Patchwork D6151: branchmap: remove the dict interface from the branchcache class (API)

login
register
mail settings
Submitter phabricator
Date March 19, 2019, 11:30 a.m.
Message ID <differential-rev-PHID-DREV-lm7o6yx7dsgddxrskmqw-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39327/
State Superseded
Headers show

Comments

phabricator - March 19, 2019, 11:30 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The current branchmap computation involves reading the whole branchmap from
  disk, validating all the nodes even if they are not required. This leads to a
  lot of time on repos which have large branchmap or a lot of branches. On large
  repos, this can validate around 1000's of nodes.
  
  On some operations, like finding whether a branch exists or not, we don't need
  to validate all the nodes. Or updating heads for a single branch.
  
  Before this patch, branchcache class was having dict interface and it was hard
  to keep track of reads.
  
  This patch removes the dict interface. Upcoming patches will implement lazy
  loading and validation of data and implement better API's.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6151

AFFECTED FILES
  mercurial/branchmap.py
  mercurial/localrepo.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - March 24, 2019, 1:08 a.m.
>      def copy(self):
>          """return an deep copy of the branchcache object"""
> -        return type(self)(
> -            self, self.tipnode, self.tiprev, self.filteredhash,
> +        return branchcache(
> +            self.entries, self.tipnode, self.tiprev, self.filteredhash,
>              self._closednodes)

In order to support subclassing, a `branchcache` type has to be resolved
dynamically. IIRC, that's the point of `type(self)` use here.
phabricator - March 24, 2019, 1:10 a.m.
yuja added a comment.


  >   def copy(self):
  >       """return an deep copy of the branchcache object"""
  > 
  > - return type(self)(
  > - self, self.tipnode, self.tiprev, self.filteredhash, +        return branchcache( +            self.entries, self.tipnode, self.tiprev, self.filteredhash, self._closednodes)
  
  In order to support subclassing, a `branchcache` type has to be resolved
  dynamically. IIRC, that's the point of `type(self)` use here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6151

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1556,7 +1556,7 @@ 
         return scmutil.revsymbol(self, key).node()
 
     def lookupbranch(self, key):
-        if key in self.branchmap():
+        if key in self.branchmap().entries:
             return key
 
         return scmutil.revsymbol(self, key).branch()
@@ -2730,7 +2730,7 @@ 
         if branch is None:
             branch = self[None].branch()
         branches = self.branchmap()
-        if branch not in branches:
+        if branch not in branches.entries:
             return []
         # the cache returns heads ordered lowest to highest
         bheads = list(reversed(branches.branchheads(branch, closed=closed)))
diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -127,7 +127,7 @@ 
         self._per_filter.clear()
 
 
-class branchcache(dict):
+class branchcache(object):
     """A dict like object that hold branches heads cache.
 
     This cache is used to avoid costly computations to determine all the
@@ -151,7 +151,6 @@ 
 
     def __init__(self, entries=(), tipnode=nullid, tiprev=nullrev,
                  filteredhash=None, closednodes=None):
-        super(branchcache, self).__init__(entries)
         self.tipnode = tipnode
         self.tiprev = tiprev
         self.filteredhash = filteredhash
@@ -162,6 +161,25 @@ 
             self._closednodes = set()
         else:
             self._closednodes = closednodes
+        self.entries = dict(entries)
+
+    def __iter__(self):
+        return iter(self.entries)
+
+    def __setitem__(self, key, value):
+        self.entries[key] = value
+
+    def __getitem__(self, key):
+        return self.entries[key]
+
+    def setdefault(self, *args):
+        return self.entries.setdefault(*args)
+
+    def iteritems(self):
+        return self.entries.iteritems()
+
+    def itervalues(self):
+        return self.entries.itervalues()
 
     @classmethod
     def fromfile(cls, repo):
@@ -271,8 +289,8 @@ 
 
     def copy(self):
         """return an deep copy of the branchcache object"""
-        return type(self)(
-            self, self.tipnode, self.tiprev, self.filteredhash,
+        return branchcache(
+            self.entries, self.tipnode, self.tiprev, self.filteredhash,
             self._closednodes)
 
     def write(self, repo):
@@ -295,7 +313,7 @@ 
             f.close()
             repo.ui.log('branchcache',
                         'wrote %s branch cache with %d labels and %d nodes\n',
-                        repo.filtername, len(self), nodecount)
+                        repo.filtername, len(self.entries), nodecount)
         except (IOError, OSError, error.Abort) as inst:
             # Abort may be raised by read only opener, so log and continue
             repo.ui.debug("couldn't write branch cache: %s\n" %
@@ -351,7 +369,7 @@ 
             # cache key are not valid anymore
             self.tipnode = nullid
             self.tiprev = nullrev
-            for heads in self.values():
+            for heads in self.itervalues():
                 tiprev = max(cl.rev(node) for node in heads)
                 if tiprev > self.tiprev:
                     self.tipnode = cl.node(tiprev)