Patchwork D1257: dirstate: remove excess attribute lookups for dirstate.status (issue5714)

login
register
mail settings
Submitter phabricator
Date Oct. 27, 2017, 8:14 p.m.
Message ID <differential-rev-PHID-DREV-3digpzh4ngihicyha7gb-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25293/
State Superseded
Headers show

Comments

phabricator - Oct. 27, 2017, 8:14 p.m.
durham created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  A recent refactor added a layer of abstraction to the dirstate which makes doing
  things like 'foo in dirstate' now require some extra Python attribute lookups.
  This is causing a 100ms slow down in hg status for mozilla-central.
  
  The fix is to hoist the inner dict's functions onto the main class once the lazy
  loading it complete, as well as store the actual functions before doing the
  status loop (as is done for other such functions).
  
  In my testing, it seems to address the performance regression, but we'll
  need to see the perf run results to know for sure.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/dirstate.py

CHANGE DETAILS




To: durham, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 28, 2017, 7:57 a.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Probably the cached attributes need to be deleted at clear(), where new
  dict is assigned to self._map.

REPOSITORY
  rHG Mercurial

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

To: durham, #hg-reviewers, yuja
Cc: mercurial-devel
phabricator - Oct. 28, 2017, 7:36 p.m.
durham added a comment.


  Fixed by using _map.clear() instead of replacing the _map.

REPOSITORY
  rHG Mercurial

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

To: durham, #hg-reviewers, yuja
Cc: mercurial-devel
phabricator - Oct. 29, 2017, 3:30 a.m.
yuja accepted this revision.
yuja added a comment.
This revision is now accepted and ready to land.


  Queued, thanks.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -1053,6 +1053,9 @@ 
         removed, deleted, clean = [], [], []
 
         dmap = self._map
+        dmap.preload()
+        dcontains = dmap.__contains__
+        dget = dmap.__getitem__
         ladd = lookup.append            # aka "unsure"
         madd = modified.append
         aadd = added.append
@@ -1074,7 +1077,7 @@ 
         full = listclean or match.traversedir is not None
         for fn, st in self.walk(match, subrepos, listunknown, listignored,
                                 full=full).iteritems():
-            if fn not in dmap:
+            if not dcontains(fn):
                 if (listignored or mexact(fn)) and dirignore(fn):
                     if listignored:
                         iadd(fn)
@@ -1089,7 +1092,7 @@ 
             # a list, but falls back to creating a full-fledged iterator in
             # general. That is much slower than simply accessing and storing the
             # tuple members one by one.
-            t = dmap[fn]
+            t = dget(fn)
             state = t[0]
             mode = t[1]
             size = t[2]
@@ -1247,6 +1250,10 @@ 
     def keys(self):
         return self._map.keys()
 
+    def preload(self):
+        """Loads the underlying data, if it's not already loaded"""
+        self._map
+
     def nonnormalentries(self):
         '''Compute the nonnormal dirstate entries from the dmap'''
         try:
@@ -1373,6 +1380,13 @@ 
         if not self._dirtyparents:
             self.setparents(*p)
 
+        # Avoid excess attribute lookups by fast pathing certain checks
+        self.__contains__ = self._map.__contains__
+        self.__getitem__ = self._map.__getitem__
+        self.__setitem__ = self._map.__setitem__
+        self.__delitem__ = self._map.__delitem__
+        self.get = self._map.get
+
     def write(self, st, now):
         st.write(parsers.pack_dirstate(self._map, self.copymap,
                                        self.parents(), now))