This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1514

perftweaks: avoid expensive branchmap calculation during commit
ClosedPublic

Authored by quark on Nov 27 2017, 12:29 PM.
Tags
None
Subscribers
durham

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGXda791c46869b: perftweaks: avoid expensive branchmap calculation during commit
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
Lint Skipped
Unit
Unit Tests Skipped

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
167

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.

168

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

169

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?

170

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

182

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
169

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

182

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

durham added a comment.Nov 29 2017, 2:43 PM

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

hgext3rd/perftweaks.py
182

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
Closed by commit rFBHGXda791c46869b: perftweaks: avoid expensive branchmap calculation during commit (authored by quark). · Explain WhyDec 5 2017, 4:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/perftweaks.py (31 lines)

Diff 3875

hgext3rd/perftweaks.py

[perftweaks] [perftweaks]
# Whether to use faster hidden cache. It has faster cache hash calculation # Whether to use faster hidden cache. It has faster cache hash calculation
# which only check stat of a few files inside store/ directory. # which only check stat of a few files inside store/ directory.
fasthiddencache = False fasthiddencache = False
""" """
from mercurial import ( from mercurial import (
branchmap, branchmap,
commands,
dispatch, dispatch,
extensions, extensions,
localrepo,
merge, merge,
namespaces, namespaces,
phases, phases,
revlog, revlog,
scmutil, scmutil,
tags, tags,
util, util,
) )
from mercurial.extensions import wrapfunction from mercurial.extensions import wrapfunction
from mercurial.node import bin from mercurial.node import bin
import errno import errno
import os import os
testedwith = 'ships-with-fb-hgext' testedwith = 'ships-with-fb-hgext'
def extsetup(ui): def extsetup(ui):
wrapfunction(tags, '_readtagcache', _readtagcache) wrapfunction(tags, '_readtagcache', _readtagcache)
wrapfunction(merge, '_checkcollision', _checkcollision) wrapfunction(merge, '_checkcollision', _checkcollision)
wrapfunction(branchmap.branchcache, 'update', _branchmapupdate) wrapfunction(branchmap.branchcache, 'update', _branchmapupdate)
wrapfunction(branchmap, 'read', _branchmapread) wrapfunction(branchmap, 'read', _branchmapread)
wrapfunction(branchmap, 'replacecache', _branchmapreplacecache) wrapfunction(branchmap, 'replacecache', _branchmapreplacecache)
wrapfunction(branchmap, 'updatecache', _branchmapupdatecache) wrapfunction(branchmap, 'updatecache', _branchmapupdatecache)
wrapfunction(commands, '_docommit', _docommit)
if ui.configbool('perftweaks', 'preferdeltas'): if ui.configbool('perftweaks', 'preferdeltas'):
wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta) wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta)
wrapfunction(dispatch, 'runcommand', _trackdirstatesizes) wrapfunction(dispatch, 'runcommand', _trackdirstatesizes)
wrapfunction(dispatch, 'runcommand', _tracksparseprofiles) wrapfunction(dispatch, 'runcommand', _tracksparseprofiles)
wrapfunction(merge, 'update', _trackupdatesize) wrapfunction(merge, 'update', _trackupdatesize)
try: try:
# #
# Just use a shortcut path that construct the branchcache directly. # Just use a shortcut path that construct the branchcache directly.
partial = repo._branchcaches.get(repo.filtername) partial = repo._branchcaches.get(repo.filtername)
if partial is None: if partial is None:
partial = branchmap.branchcache() partial = branchmap.branchcache()
partial.update(repo, None) partial.update(repo, None)
repo._branchcaches[repo.filtername] = partial repo._branchcaches[repo.filtername] = partial
# _docommit accesses "branchheads" just to decide whether to show "(created new
# head)" or not. Accessing branchheads requires calling "headrevs()" which is
# expensive since it scans entire changelog. We can just test if "." has any
# existing children to know if there is a new head or not.
durhamUnsubmitted
Not Done

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.

durham: Maybe comment that branchheads is called *before* the commit, and therefore '.' here is the old…
def _singlebranchheads(orig, repo, branch):
durhamUnsubmitted
Not Done

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

durham: This doesn't seem to match the signature for localrepository.branchheads.
unfi = repo.unfiltered()
durhamUnsubmitted
Not Done

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?

durham: By doing the children calculation on an unfiltered set, won't we get "created new head"…
quarkAuthorUnsubmitted
Not Done

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

quark: I remembered that I did this because repo is using the "served" filter and loading that…
dotrev = unfi['.'].rev()
durhamUnsubmitted
Not Done

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

durham: Maybe rename this to 'parentrev' to make it clear that '.' is the pre-commit working copy parent
children = None
try:
children = unfi.changelog.index._children(dotrev)
except Exception:
pass
if (repo.ui.configbool('tweakdefaults', 'allowbranch', True)
or children is None):
return orig(repo, branch)
if children:
# dotrev is not a head. Return a faked head so the new rev is not part
# of it and "created new head" will be printed.
return ['\0' * 20]
durhamUnsubmitted
Not Done

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.

durham: It's technically possible that the "new" commit existed before the commit function ran (i.e. hg…
quarkAuthorUnsubmitted
Not Done

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

quark: (Also to the above unfiltered issue) I guess the correct solution is to take visibility into…
durhamUnsubmitted
Not Done

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.

durham: This can happen even if no commits are hidden. A commit might just happen to produce the same…
else:
# dotrev is a head. Return a list containing that head.
return [repo.changelog.node(dotrev)]
def _docommit(orig, ui, repo, *pats, **opts):
with extensions.wrappedfunction(localrepo.localrepository,
'branchheads', _singlebranchheads):
return orig(ui, repo, *pats, **opts)
def _branchmapwrite(orig, self, repo): def _branchmapwrite(orig, self, repo):
if repo.ui.configbool('perftweaks', 'disablebranchcache'): if repo.ui.configbool('perftweaks', 'disablebranchcache'):
# Since we don't read the branchcache, don't bother writing it. # Since we don't read the branchcache, don't bother writing it.
result = None result = None
else: else:
result = orig(self, repo) result = orig(self, repo)
if repo.ui.configbool('perftweaks', 'cachenoderevs', True): if repo.ui.configbool('perftweaks', 'cachenoderevs', True):
revs = set() revs = set()