This is an archive of the discontinued Mercurial Phabricator instance.

perftweaks: avoid expensive branchmap calculation during commit

Authored by quark on Nov 27 2017, 12:29 PM.



The commit code path reads branchheads just to show whether the new commit
is creating a new head or not.

439     | _docommit                 
111      \ branchheads              
109       | branchmap               
109       | _branchmapupdatecache   
109       | closure                 
109       | _branchmapupdate        

Disabling the branch calculation saves about 100ms during commit in a large

Test Plan

Added a test.

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 27 2017, 12:29 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 27 2017, 12:29 PM
durham requested changes to this revision.Nov 27 2017, 10:21 PM
durham added a subscriber: durham.
durham added inline comments.

Maybe comment that branchheads is called *before* the commit, and therefore '.' here is the old working copy parent, and that the result of this function is used to test if the old parent was already a head and if the new commit wasn't a head already.


This doesn't seem to match the signature for localrepository.branchheads.


By doing the children calculation on an unfiltered set, won't we get "created new head" messages when the old child is a hidden commit? I don't think we'd get a message in that case before?


Maybe rename this to 'parentrev' to make it clear that '.' is the pre-commit working copy parent


It's technically possible that the "new" commit existed before the commit function ran (i.e. hg commit produced the exact same hash as already existed), and was a head at the time. In that case, returning a fake result here will cause it to print "created new head" when no commit was actually produced. If there's no easy way to fix it, maybe that's ok since it should be super rare.

This revision now requires changes to proceed.Nov 27 2017, 10:21 PM
quark added a comment.Nov 28 2017, 7:46 PM

I'm currently leaning towards getting rid of the message to avoid thinking about the unfiltered/already-exist corner cases.

Practically, since the DAG is easily accessible (sl), it seems fine to not "warn" about new heads. I can imagine the original problem of multiple heads is the branch name then becomes ambiguous. But it's less a problem since we don't use named branches.


I remembered that I did this because repo is using the "served" filter and loading that repoview is expensive.


(Also to the above unfiltered issue) I guess the correct solution is to take visibility into consideration.

I'm totally on board with getting rid of the message.


This can happen even if no commits are hidden. A commit might just happen to produce the same hash as one that already exists and trigger this case.

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 3991.
durham accepted this revision.Dec 4 2017, 11:35 PM
This revision is now accepted and ready to land.Dec 4 2017, 11:35 PM
This revision was automatically updated to reflect the committed changes.