This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9023

branchmap: add a cache validation cache, avoid expensive re-hash on every use
ClosedPublic

Authored by spectral on Sep 15 2020, 6:48 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG89f0d9f87701: branchmap: add a cache validation cache, avoid expensive re-hash on every use
Summary

In a pathological hg log case, we end up executing the branchmap validity
checking twice per commit displayed. Or maybe we always do, and I just noticed
because it's really slow in this repo for some reason.

Before:

Time (mean ± σ):      9.816 s ±  0.071 s    [User: 9.435 s, System: 0.392 s]

Range (min … max):    9.709 s …  9.920 s

After:

Time (mean ± σ):      8.671 s ±  0.078 s    [User: 8.309 s, System: 0.392 s]

Range (min … max):    8.594 s …  8.816 s

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

spectral created this revision.Sep 15 2020, 6:48 PM
pulkit added a subscriber: pulkit.Sep 16 2020, 3:29 AM

Looks good to me apart from the probable unrelated change.

hg
1 ↗(On Diff #22646)

Unrelated change?

marmoute added a subscriber: marmoute.Sep 16 2020, 6:52 AM

Did you measure the performance impact on other operation than your target operation ? I am curious about the impact of the @property here.

spectral updated this revision to Diff 22658.Sep 16 2020, 3:14 PM
spectral added a comment.Sep 16 2020, 3:14 PM
In D9023#135215, @marmoute wrote:

Did you measure the performance impact on other operation than your target operation ? I am curious about the impact of the @property here.

I did not. Do you have recommendations for a representative sample of repos/commands that might show an issue? Or perhaps I can switch to a different mechanism? There's very few locations that set this, switching them to instead call a 'set_filteredrevs' method would likely be trivial.

In D9023#135204, @pulkit wrote:

Looks good to me apart from the probable unrelated change.

*sigh* I keep doing that... sorry! Updated.

martinvonz accepted this revision.Sep 21 2020, 4:22 PM
martinvonz added a subscriber: martinvonz.
In D9023#135241, @spectral wrote:
In D9023#135215, @marmoute wrote:

Did you measure the performance impact on other operation than your target operation ? I am curious about the impact of the @property here.

I did not. Do you have recommendations for a representative sample of repos/commands that might show an issue? Or perhaps I can switch to a different mechanism? There's very few locations that set this, switching them to instead call a 'set_filteredrevs' method would likely be trivial.

I can't think of any myself and this is a clear win in my hg repo, so I'll queue this as is. We can always fix any slowness caused by @property in a follow-up if we can measure any.

This revision is now accepted and ready to land.Sep 21 2020, 4:22 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/changelog.py (14 lines)
Mmercurial/scmutil.py (16 lines)

Diff 22751

mercurial/changelog.py

# tend to be small and don't delta well with each. So disable delta # tend to be small and don't delta well with each. So disable delta
# chains. # chains.
self._storedeltachains = False self._storedeltachains = False
self._realopener = opener self._realopener = opener
self._delayed = False self._delayed = False
self._delaybuf = None self._delaybuf = None
self._divert = False self._divert = False
self.filteredrevs = frozenset() self._filteredrevs = frozenset()
self._filteredrevs_hashcache = {}
self._copiesstorage = opener.options.get(b'copies-storage') self._copiesstorage = opener.options.get(b'copies-storage')
@property
def filteredrevs(self):
return self._filteredrevs
@filteredrevs.setter
def filteredrevs(self, val):
# Ensure all updates go through this function
assert isinstance(val, frozenset)
self._filteredrevs = val
self._filteredrevs_hashcache = {}
def delayupdate(self, tr): def delayupdate(self, tr):
"""delay visibility of index updates to other readers""" """delay visibility of index updates to other readers"""
if not self._delayed: if not self._delayed:
if len(self) == 0: if len(self) == 0:
self._divert = True self._divert = True
if self._realopener.exists(self.indexfile + b'.a'): if self._realopener.exists(self.indexfile + b'.a'):
self._realopener.unlink(self.indexfile + b'.a') self._realopener.unlink(self.indexfile + b'.a')

mercurial/scmutil.py

tipnode changing. tipnode changing.
This function hashes all the revs filtered from the view and returns This function hashes all the revs filtered from the view and returns
that SHA-1 digest. that SHA-1 digest.
""" """
cl = repo.changelog cl = repo.changelog
if not cl.filteredrevs: if not cl.filteredrevs:
return None return None
key = None key = cl._filteredrevs_hashcache.get(maxrev)
if not key:
revs = sorted(r for r in cl.filteredrevs if r <= maxrev) revs = sorted(r for r in cl.filteredrevs if r <= maxrev)
if revs: if revs:
s = hashutil.sha1() s = hashutil.sha1()
for rev in revs: for rev in revs:
s.update(b'%d;' % rev) s.update(b'%d;' % rev)
key = s.digest() key = s.digest()
cl._filteredrevs_hashcache[maxrev] = key
return key return key
def walkrepos(path, followsym=False, seen_dirs=None, recurse=False): def walkrepos(path, followsym=False, seen_dirs=None, recurse=False):
'''yield every hg repository under path, always recursively. '''yield every hg repository under path, always recursively.
The recurse flag will only control recursion into repo working dirs''' The recurse flag will only control recursion into repo working dirs'''
def errhandler(err): def errhandler(err):