This is an archive of the discontinued Mercurial Phabricator instance.

copies: fix the changeset based algorithm regarding merge
ClosedPublic

Authored by marmoute on Mar 5 2020, 1:14 PM.

Details

Summary

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge. Older information could reach the merge through
p1, and while information from p2 was strictly fresher, it would get
overwritten anyway.

We update the situation with more details about which revision introduces rename
information. This help use making the right decision in case of merge.

We are now running a more comprehensive suite of test with include this kind of
situation. The behavior differ slightly from the filelog based in a couple of
instance. There is mostly two distinct cases:

  1. there are conflicting rename information in a merge (different rename history

on each side). In this case the filelog based implementation arbitrarily pick a
side based on the file-revision-number. So it depends on a local factor. The
changeset centric algorithm will use a deterministic approach, by picking the
information coming from the first parent of the merge. This is stable across
different clone.

  1. rename information related to file that exist in both source and destination.

The filelog based implementation do not even try to detect these, however the
changeset centric one get them for "free" (it is simpler to detect them than
not).

The new implementation focus on correctness. Performance improvement will come
later.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

marmoute created this revision.Mar 5 2020, 1:14 PM
marmoute updated this revision to Diff 20551.Mar 6 2020, 5:21 AM

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge.

Could you explain why it broke? It's hard to review this patch without really knowing what the problem or the solution is.

The new implementation focus on correctness. Performance improvement will come
later.

How much slower is it? Could you run some of those benchmarks you have run on previous patches touching this code? How do you plan to improve it?

mercurial/copies.py
277

s/had/add/?

tests/test-copies-chain-merge.t
743–744

nit: combine into (no-filelog !)?

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge.

Could you explain why it broke? It's hard to review this patch without really knowing what the problem or the solution is.

Outdated information from p1 could overwrite newer information from p2.

The new implementation focus on correctness. Performance improvement will come
later.

How much slower is it? Could you run some of those benchmarks you have run on previous patches touching this code? How do you plan to improve it?

There are two new calls that might degrade performance. This isancestor call that is easy to cache in memory. And the "ismerged" logic that is easy to cache on disk with the rest of the copy related information (the case is rare).

There is some win to have in python, but the main win will be the move the Rust algorithm (that need to be updated with the new logic). Moving to rust give a very large performance boost on the slow cases (usually over 10x, sometime 100x IIRC). That is the one I care about.

marmoute edited the summary of this revision. (Show Details)Mar 6 2020, 6:52 PM
marmoute updated this revision to Diff 20596.

series update after Martin feedback

marmoute marked 2 inline comments as done.Mar 6 2020, 7:48 PM

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge.

Could you explain why it broke? It's hard to review this patch without really knowing what the problem or the solution is.

Outdated information from p1 could overwrite newer information from p2.

The new implementation focus on correctness. Performance improvement will come
later.

How much slower is it? Could you run some of those benchmarks you have run on previous patches touching this code? How do you plan to improve it?

There are two new calls that might degrade performance. This isancestor call that is easy to cache in memory. And the "ismerged" logic that is easy to cache on disk with the rest of the copy related information (the case is rare).
There is some win to have in python, but the main win will be the move the Rust algorithm (that need to be updated with the new logic). Moving to rust give a very large performance boost on the slow cases (usually over 10x, sometime 100x IIRC). That is the one I care about.

I'll make the performance impact more concrete myself. I picked two quite arbitrary tags in the mozilla-unified repo and this is what I saw:

Before this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 5.279230 comb 5.270000 user 5.250000 sys 0.020000 (best of 3)

After this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 8.277523 comb 8.280000 user 8.170000 sys 0.110000 (best of 3)

Could you share some more benchmarking data? I know you had a set of commits that you've used before (and that you've asked me to use for benchmarking my patches to copies.py against). It's quite a significant slowdown for the case I tested above, but I'm fine with it since it fixes a bug. I'd just like to see how it behaves in other cases.

mercurial/copies.py
277

Not actually done, it seems, but not a big deal anyway.

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge.

Could you explain why it broke? It's hard to review this patch without really knowing what the problem or the solution is.

Outdated information from p1 could overwrite newer information from p2.

The new implementation focus on correctness. Performance improvement will come
later.

How much slower is it? Could you run some of those benchmarks you have run on previous patches touching this code? How do you plan to improve it?

There are two new calls that might degrade performance. This isancestor call that is easy to cache in memory. And the "ismerged" logic that is easy to cache on disk with the rest of the copy related information (the case is rare).
There is some win to have in python, but the main win will be the move the Rust algorithm (that need to be updated with the new logic). Moving to rust give a very large performance boost on the slow cases (usually over 10x, sometime 100x IIRC). That is the one I care about.

I'll make the performance impact more concrete myself. I picked two quite arbitrary tags in the mozilla-unified repo and this is what I saw:
Before this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 5.279230 comb 5.270000 user 5.250000 sys 0.020000 (best of 3)

After this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 8.277523 comb 8.280000 user 8.170000 sys 0.110000 (best of 3)

Could you share some more benchmarking data? I know you had a set of commits that you've used before (and that you've asked me to use for benchmarking my patches to copies.py against). It's quite a significant slowdown for the case I tested above, but I'm fine with it since it fixes a bug. I'd just like to see how it behaves in other cases.

We now have some automatic benchmark setup for copies tracing, however we don't have any reference repositories with the necessary data for changeset centric copy tracing. Building a reference takes some manual operation and a lots of CPU time. So I am planning to build some once the format is more finalized.

I am not too worried about the current performance number because there are multiple easy optimization. In addition the rust version of the previous algorithm proved massively more efficient, so I have good hope for this one too.

The existing reference can still be used to gather various useful pairs of revision to run manual benchmark on.

You can see them using the following command in a setup scmperf repo.

$ grep -A 3 copies repos/*.benchrepo

To setup an scmperrepo, you can use:

$ hg clone https://foss.heptapod.net/mercurial/scmperf
$ cd scmperf
$ ./script/setup-repos default.repos
marmoute updated this revision to Diff 21029.Apr 10 2020, 1:43 PM
marmoute updated this revision to Diff 21180.Apr 22 2020, 12:11 PM

Gentle ping on this. This is still ready for review.

marmoute updated this revision to Diff 21181.Apr 22 2020, 1:24 PM

Gentle ping on this. This is still ready for review.

I'm still a bit worried about the performance regression and I was hoping to get more information about that. Now is the beginning of the next cycle, so I guess it's a good time to queue this now and test it on Google users. I'll review this soon.

Gentle ping on this. This is still ready for review.

I'm still a bit worried about the performance regression and I was hoping to get more information about that. Now is the beginning of the next cycle, so I guess it's a good time to queue this now and test it on Google users. I'll review this soon.

I started poking at the performance and [spoiler] and this will be fine :-) Do not hold your breath, because there is a long road between "prototype to get numbers" and "clean implementation". If you can queue this, I would appreciate building on a more solid ground.

For example, on the worst pypy case I have been playing with:

Before the fix: ! wall 1.765737 comb 1.760000 user 1.690000 sys 0.070000 (median of 6)
After the fix:  ! wall 29.983194 comb 29.880000 user 29.780000 sys 0.100000 (median of 3)
With curent state of speedup:   ! wall 2.135641 comb 2.140000 user 2.120000 sys 0.020000 (median of 5)
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.