This is an archive of the discontinued Mercurial Phabricator instance.

perftweaks: further cleanup branchmap code paths

Authored by quark on Nov 23 2017, 5:27 PM.


Group Reviewers
Restricted Project
rFBHGXd619c400aac0: perftweaks: further cleanup branchmap code paths

The branchmap code is actually quite expensive (O(changelog)). So let's try
to avoid them as much as possible.

First, reading or writing on-disk caches are unnecessary since even with the
on-disk cache, it still has to pay O(changelog) headrevs cost to
incrementally update the cache, which is the same cost of rebuilding the
cache from memory.

Secondly, repo.branchmap() calls update unconditionally so the update
function should be smart to not update unnecessarily. That's the validfor

Thirdly, repo.branchmap() calls branchmap.updatecache which constructs
the "served", "immutable", "base" repoviews as an attempt to "incrementally"
build the branch cache for the "visible" repoview. There is no need to have
that many repoviews in this case. Therefore branchmap.updatecache is
replaced to a simplified version that does not construct other repoviews.

Note: there is a simple cache in index.headrevs() but that cache gets
invalidated if filteredrevs has a different address - that means different
repoviews will not benefit from index.headrevs() cache if they pass
different filteredrevs objects.

The change is gated by a new config option perftweaks.disablebranchcache2
so we can rollback if anything went wrong.

Test Plan

Run core hg tests which do not use branches (about 371 tests):

ruby -e 'puts Dir["*.t"].reject{|x|[/(hg.* branch(es)?|branch\(|--branch|branch:\s*[^d]|cache\/branch|updating to branch\s*[^d])/]}' > /tmp/tests
./ -l --extra-config-opt=extensions.perftweaks=$HOME/fb-hgext/hgext3rd/ --extra-config-opt=perftweaks.disablebranchcache=1 --extra-config-opt=perftweaks.disablebranchcache2=1 -j `nproc` `cat /tmp/tests`

Categorize them and analyse failures:

"abort: push creates new remote head X":

Failed test-exchange-obsmarkers-case-A2.t: output changed
Failed test-exchange-obsmarkers-case-A3.t: output changed
Failed test-exchange-obsmarkers-case-A5.t: output changed
Failed test-exchange-obsmarkers-case-B3.t: output changed
Failed test-exchange-obsmarkers-case-D4.t: output changed
Failed test-obsolete-checkheads.t: output changed
Failed test-push-checkheads-unpushed-D3.t: output changed
Failed test-push-checkheads-unpushed-D4.t: output changed
Failed test-push-checkheads-unpushed-D5.t: output changed
Failed test-push-checkheads-unpushed-D7.t: output changed
Failed test-bundle2-exchange.t: output changed
Failed test-phases-exchange.t: output changed

This is already broken before this change because we ignored revgen
parameter passed to branchcache.update from checkheads code path.

Harmless: config/extension/help/ui.log output changes:

Failed test-basic.t: output changed
Failed test-debugextensions.t: output changed
Failed test-help.t: output changed
Failed test-devel-warnings.t: output changed
Failed test-blackbox.t: output changed

Harmless: Change because .hg/cache does not get created:

Failed test-parseindex.t: output changed

Harmless: Branch related tests that are not filtered by regex:

Failed test-convert-cvs-branch.t: output changed
Failed test-convert-cvs.t: output changed
Failed test-convert-tagsbranch-topology.t: output changed

Harmless "Unable to find a working hg binary to extract the version from the repository tags":

Failed test-run-tests.t: output changed

Run jf integration test which catches an error caused by previous
problematic patches.

Diff Detail

rFBHGX Facebook Mercurial Extensions
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Nov 23 2017, 5:27 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 23 2017, 5:27 PM
durham requested changes to this revision.Nov 27 2017, 10:00 PM
durham added a subscriber: durham.
durham added inline comments.

Why delete default here? The old code seemed to always set a value for default.


Could we put these new checks behind a second config option as well? disablebranchcache is already set to True in many of our repos, so all these branch cache changes will get deployed as soon as the rpm ships. Since this is a somewhat subtle part of the code base, I'd feel more comfortable if we shipped it to alpha first.

This revision now requires changes to proceed.Nov 27 2017, 10:00 PM
quark added inline comments.Nov 28 2017, 7:38 PM

This happens when the repo is empty. The core hg behavior (without perftweaks) is to return {} instead of {'default': []}. So del default is more consistent with the core behavior.


Sure. Will do.

quark edited the summary of this revision. (Show Details)Nov 29 2017, 8:40 PM
quark edited the test plan for this revision. (Show Details)
quark updated this revision to Diff 3990.
durham accepted this revision.Dec 4 2017, 11:34 PM
This revision is now accepted and ready to land.Dec 4 2017, 11:34 PM
This revision was automatically updated to reflect the committed changes.