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

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1257

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

Authored by durham on Oct 27 2017, 4:14 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHGffeea2406276: dirstate: remove excess attribute lookups for dirstate.status (issue5714)
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.

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

durham created this revision.Oct 27 2017, 4:14 PM
yuja accepted this revision.Oct 28 2017, 3:46 AM
This revision is now accepted and ready to land.Oct 28 2017, 3:46 AM
yuja requested changes to this revision.Oct 28 2017, 3:57 AM

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

This revision now requires changes to proceed.Oct 28 2017, 3:57 AM
durham updated this revision to Diff 3147.Oct 28 2017, 3:36 PM
durham added a comment.Oct 28 2017, 3:36 PM

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

yuja accepted this revision.Oct 28 2017, 11:30 PM

Queued, thanks.

This revision is now accepted and ready to land.Oct 28 2017, 11:30 PM
Closed by commit rHGffeea2406276: dirstate: remove excess attribute lookups for dirstate.status (issue5714) (authored by durham). · Explain WhyOct 28 2017, 11:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (22 lines)

Diff 3150

mercurial/dirstate.py

files that have definitely not been modified since the files that have definitely not been modified since the
dirstate was written dirstate was written
''' '''
listignored, listclean, listunknown = ignored, clean, unknown listignored, listclean, listunknown = ignored, clean, unknown
lookup, modified, added, unknown, ignored = [], [], [], [], [] lookup, modified, added, unknown, ignored = [], [], [], [], []
removed, deleted, clean = [], [], [] removed, deleted, clean = [], [], []
dmap = self._map dmap = self._map
dmap.preload()
dcontains = dmap.__contains__
dget = dmap.__getitem__
ladd = lookup.append # aka "unsure" ladd = lookup.append # aka "unsure"
madd = modified.append madd = modified.append
aadd = added.append aadd = added.append
uadd = unknown.append uadd = unknown.append
iadd = ignored.append iadd = ignored.append
radd = removed.append radd = removed.append
dadd = deleted.append dadd = deleted.append
cadd = clean.append cadd = clean.append
mexact = match.exact mexact = match.exact
dirignore = self._dirignore dirignore = self._dirignore
checkexec = self._checkexec checkexec = self._checkexec
copymap = self._map.copymap copymap = self._map.copymap
lastnormaltime = self._lastnormaltime lastnormaltime = self._lastnormaltime
# We need to do full walks when either # We need to do full walks when either
# - we're listing all clean files, or # - we're listing all clean files, or
# - match.traversedir does something, because match.traversedir should # - match.traversedir does something, because match.traversedir should
# be called for every dir in the working dir # be called for every dir in the working dir
full = listclean or match.traversedir is not None full = listclean or match.traversedir is not None
for fn, st in self.walk(match, subrepos, listunknown, listignored, for fn, st in self.walk(match, subrepos, listunknown, listignored,
full=full).iteritems(): full=full).iteritems():
if fn not in dmap: if not dcontains(fn):
if (listignored or mexact(fn)) and dirignore(fn): if (listignored or mexact(fn)) and dirignore(fn):
if listignored: if listignored:
iadd(fn) iadd(fn)
else: else:
uadd(fn) uadd(fn)
continue continue
# This is equivalent to 'state, mode, size, time = dmap[fn]' but not # This is equivalent to 'state, mode, size, time = dmap[fn]' but not
# written like that for performance reasons. dmap[fn] is not a # written like that for performance reasons. dmap[fn] is not a
# Python tuple in compiled builds. The CPython UNPACK_SEQUENCE # Python tuple in compiled builds. The CPython UNPACK_SEQUENCE
# opcode has fast paths when the value to be unpacked is a tuple or # opcode has fast paths when the value to be unpacked is a tuple or
# a list, but falls back to creating a full-fledged iterator in # a list, but falls back to creating a full-fledged iterator in
# general. That is much slower than simply accessing and storing the # general. That is much slower than simply accessing and storing the
# tuple members one by one. # tuple members one by one.
t = dmap[fn] t = dget(fn)
state = t[0] state = t[0]
mode = t[1] mode = t[1]
size = t[2] size = t[2]
time = t[3] time = t[3]
if not st and state in "nma": if not st and state in "nma":
dadd(fn) dadd(fn)
elif state == 'n': elif state == 'n':
@propertycache @propertycache
def copymap(self): def copymap(self):
self.copymap = {} self.copymap = {}
self._map self._map
return self.copymap return self.copymap
def clear(self): def clear(self):
self._map = {} self._map.clear()
self.copymap = {} self.copymap.clear()
self.setparents(nullid, nullid) self.setparents(nullid, nullid)
def iteritems(self): def iteritems(self):
return self._map.iteritems() return self._map.iteritems()
def __len__(self): def __len__(self):
return len(self._map) return len(self._map)
return self._map[key] return self._map[key]
def __delitem__(self, key): def __delitem__(self, key):
del self._map[key] del self._map[key]
def keys(self): def keys(self):
return self._map.keys() return self._map.keys()
def preload(self):
"""Loads the underlying data, if it's not already loaded"""
self._map
def nonnormalentries(self): def nonnormalentries(self):
'''Compute the nonnormal dirstate entries from the dmap''' '''Compute the nonnormal dirstate entries from the dmap'''
try: try:
return parsers.nonnormalotherparententries(self._map) return parsers.nonnormalotherparententries(self._map)
except AttributeError: except AttributeError:
nonnorm = set() nonnorm = set()
otherparent = set() otherparent = set()
for fname, e in self._map.iteritems(): for fname, e in self._map.iteritems():
# parsing the dirstate. # parsing the dirstate.
# #
# (we cannot decorate the function directly since it is in a C module) # (we cannot decorate the function directly since it is in a C module)
parse_dirstate = util.nogc(parsers.parse_dirstate) parse_dirstate = util.nogc(parsers.parse_dirstate)
p = parse_dirstate(self._map, self.copymap, st) p = parse_dirstate(self._map, self.copymap, st)
if not self._dirtyparents: if not self._dirtyparents:
self.setparents(*p) 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): def write(self, st, now):
st.write(parsers.pack_dirstate(self._map, self.copymap, st.write(parsers.pack_dirstate(self._map, self.copymap,
self.parents(), now)) self.parents(), now))
st.close() st.close()
self._dirtyparents = False self._dirtyparents = False
self.nonnormalset, self.otherparentset = self.nonnormalentries() self.nonnormalset, self.otherparentset = self.nonnormalentries()
@propertycache @propertycache