Page MenuHomePhabricator

perftweaks: avoid expensive branchmap calculation during commit
ClosedPublic

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

Details

Summary

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

439     | _docommit                           commands.py:1505
111      \ branchheads                        localrepo.py:2169
109       | branchmap                         localrepo.py:953
109       | _branchmapupdatecache             perftweaks.py:146
109       | closure                           extensions.py:343
109       | _branchmapupdate                  perftweaks.py:107

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

Test Plan

Added a test.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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.
hgext3rd/perftweaks.py
171

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.

172

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

173

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?

174

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

186

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.

hgext3rd/perftweaks.py
173

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

186

(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.

hgext3rd/perftweaks.py
186

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.