This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5291

branchmap: build the revbranchcache._namesreverse() only when required
ClosedPublic

Authored by pulkit on Nov 21 2018, 9:17 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG50a64c321c1e: branchmap: build the revbranchcache._namesreverse() only when required
Summary

On big repositories with a lot of named branches and that also increasing over
time, building of this dict can be expensive and shows up in profile.

For our internal repository, this saves ~0.05 seconds.

Thanks to Yuya for suggesting using util.propertycache() and
util.clearcachedproperty().

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Nov 21 2018, 9:17 AM
yuja added a subscriber: yuja.Nov 21 2018, 9:39 AM

+ if self._namesreverse is None:
+ self._namesreverse = dict((b, r) for r, b in enumerate(self._names))

if b in self._namesreverse:
    branchidx = self._namesreverse[b]
else:

@@ -467,6 +469,8 @@

def setdata(self, branch, rev, node, close):
    """add new data information to the cache"""

+ if self._namesreverse is None:
+ self._namesreverse = dict((b, r) for r, b in enumerate(self._names))

Maybe _namesreverse can be a propertycache, instead of duplicating it.
del self.__dict__[r'...'] can be used to discard the cache.

pulkit added a comment.Nov 22 2018, 4:44 AM
In D5291#78794, @yuja wrote:

+ if self._namesreverse is None:
+ self._namesreverse = dict((b, r) for r, b in enumerate(self._names))

if b in self._namesreverse:
    branchidx = self._namesreverse[b]
else:

@@ -467,6 +469,8 @@

def setdata(self, branch, rev, node, close):
    """add new data information to the cache"""

+ if self._namesreverse is None:
+ self._namesreverse = dict((b, r) for r, b in enumerate(self._names))

Maybe _namesreverse can be a propertycache, instead of duplicating it.
del self.__dict__[r'...'] can be used to discard the cache.

Hm, I am not sure if I feel good about doing del self.__dict__[r'...'] thing.

yuja added a comment.Nov 22 2018, 8:32 AM
> Maybe _namesreverse can be a propertycache, instead of duplicating it.
>  `del self.__dict__[r'...']` can be used to discard the cache.
Hm, I am not sure if I feel good about doing `del self.__dict__[r'...']` thing.

It's pretty common in Mercurial codebase. That said, I found a better way.
util.clearcachedproperty(self, '...').

pulkit edited the summary of this revision. (Show Details)Nov 22 2018, 9:00 AM
pulkit updated this revision to Diff 12586.
Closed by commit rHG50a64c321c1e: branchmap: build the revbranchcache._namesreverse() only when required (authored by pulkit). · Explain WhyNov 23 2018, 10:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/branchmap.py (7 lines)

Diff 12586

mercurial/branchmap.py

stringutil.forcebytestr(inst)) stringutil.forcebytestr(inst))
# remember number of good records on disk # remember number of good records on disk
self._rbcrevslen = min(len(self._rbcrevs) // _rbcrecsize, self._rbcrevslen = min(len(self._rbcrevs) // _rbcrecsize,
len(repo.changelog)) len(repo.changelog))
if self._rbcrevslen == 0: if self._rbcrevslen == 0:
self._names = [] self._names = []
self._rbcnamescount = len(self._names) # number of names read at self._rbcnamescount = len(self._names) # number of names read at
# _rbcsnameslen # _rbcsnameslen
self._namesreverse = dict((b, r) for r, b in enumerate(self._names))
def _clear(self): def _clear(self):
self._rbcsnameslen = 0 self._rbcsnameslen = 0
del self._names[:] del self._names[:]
self._rbcnamescount = 0 self._rbcnamescount = 0
self._namesreverse.clear()
self._rbcrevslen = len(self._repo.changelog) self._rbcrevslen = len(self._repo.changelog)
self._rbcrevs = bytearray(self._rbcrevslen * _rbcrecsize) self._rbcrevs = bytearray(self._rbcrevslen * _rbcrecsize)
util.clearcachedproperty(self, '_namesreverse')
@util.propertycache
def _namesreverse(self):
return dict((b, r) for r, b in enumerate(self._names))
def branchinfo(self, rev): def branchinfo(self, rev):
"""Return branch name and close flag for rev, using and updating """Return branch name and close flag for rev, using and updating
persistent cache.""" persistent cache."""
changelog = self._repo.changelog changelog = self._repo.changelog
rbcrevidx = rev * _rbcrecsize rbcrevidx = rev * _rbcrecsize
# avoid negative index, changelog.read(nullrev) is fast without cache # avoid negative index, changelog.read(nullrev) is fast without cache