This is an archive of the discontinued Mercurial Phabricator instance.

Differential D752

dirstate: create new dirstatemap class
ClosedPublic

Authored by durham on Sep 20 2017, 8:03 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGb36881c68569: dirstate: create new dirstatemap class
rHG42b68c9c2742: dirstate: create new dirstatemap class
Summary

This is part of a larger refactor to move the dirstate storage logic to a
separate class, so it's easier to rewrite the dirstate storage layer without
having to rewrite all the algorithms as well.

Step one it to create the class, and replace dirstate._map with it. The
abstraction bleeds through in a few places where the main dirstate class has to
access self._map._map, but those will be cleaned up in future patches.

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.Sep 20 2017, 8:03 PM
durham added a comment.Sep 20 2017, 8:04 PM

This series is the first 8 commits of a 20 patch series. At the end all the storage for the dirstate goes through the dirstatemap class.

indygreg accepted this revision.Sep 24 2017, 11:07 AM
indygreg added a subscriber: indygreg.

Looks good. Nits on API can be addressed as a follow-up, if needed.

mercurial/dirstate.py
1345

Do we want this inherit from collections.MutableMapping?

A good reason to say "no" is KISS, especially if we'll be having multiple backends.

1349–1350

Given that iteritems() no longer exists in Python 3, should we change this to items() and standardize on the modern convention?

This revision is now accepted and ready to land.Sep 24 2017, 11:07 AM
indygreg added a comment.Sep 24 2017, 11:34 AM

In the spirit of full disclosure, I reviewed this series after a long-haul flight. I'm a little jet lagged and am not an expert in dirstate. This series seems like a pretty straightforward refactor to me, which is why I felt comfortable giving review. But if there be dragons in this code, an extra set of eyeballs may be warranted.

durham added inline comments.Sep 26 2017, 6:40 AM
mercurial/dirstate.py
1345

For now I'd say no, until we've done at least one other backend implementation and have seen that MutableMapping is still appropriate. In particular, we might not want to allow iteration without a matcher, to discourage O(working copy) operations.

1349–1350

Do we do that anywhere else in the code base already? My initial tendency is to just follow existing patterns, especially during a refactor, to minimize churn.

Closed by commit rHG42b68c9c2742: dirstate: create new dirstatemap class (authored by durham). · Explain WhySep 29 2017, 2:43 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Sep 29 2017, 4:03 PM
martinvonz added inline comments.
mercurial/dirstate.py
1345

I was wondering if all this delegation would be noticeably costly, but I couldn't think of a case where it is (no significant change in "hg files -0 | xargs -0 touch; time ~/hg/hg st", for example)

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (48 lines)

Diff 2155

mercurial/dirstate.py

return os.fstat(tmpfd).st_mtime return os.fstat(tmpfd).st_mtime
finally: finally:
os.close(tmpfd) os.close(tmpfd)
vfs.unlink(tmpname) vfs.unlink(tmpname)
def nonnormalentries(dmap): def nonnormalentries(dmap):
'''Compute the nonnormal dirstate entries from the dmap''' '''Compute the nonnormal dirstate entries from the dmap'''
try: try:
return parsers.nonnormalotherparententries(dmap) return parsers.nonnormalotherparententries(dmap._map)
except AttributeError: except AttributeError:
nonnorm = set() nonnorm = set()
otherparent = set() otherparent = set()
for fname, e in dmap.iteritems(): for fname, e in dmap.iteritems():
if e[0] != 'n' or e[3] == -1: if e[0] != 'n' or e[3] == -1:
nonnorm.add(fname) nonnorm.add(fname)
if e[0] == 'n' and e[2] == -2: if e[0] == 'n' and e[2] == -2:
otherparent.add(fname) otherparent.add(fname)
@propertycache @propertycache
def _filefoldmap(self): def _filefoldmap(self):
try: try:
makefilefoldmap = parsers.make_file_foldmap makefilefoldmap = parsers.make_file_foldmap
except AttributeError: except AttributeError:
pass pass
else: else:
return makefilefoldmap(self._map, util.normcasespec, return makefilefoldmap(self._map._map, util.normcasespec,
util.normcasefallback) util.normcasefallback)
f = {} f = {}
normcase = util.normcase normcase = util.normcase
for name, s in self._map.iteritems(): for name, s in self._map.iteritems():
if s[0] != 'r': if s[0] != 'r':
f[normcase(name)] = name f[normcase(name)] = name
f['.'] = '.' # prevents useless util.fspath() invocation f['.'] = '.' # prevents useless util.fspath() invocation
raise error.Abort(_('working directory state appears damaged!')) raise error.Abort(_('working directory state appears damaged!'))
except IOError as err: except IOError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise
return [nullid, nullid] return [nullid, nullid]
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return util.dirs(self._map, 'r') return util.dirs(self._map._map, 'r')
def dirs(self): def dirs(self):
return self._dirs return self._dirs
@rootcache('.hgignore') @rootcache('.hgignore')
def _ignore(self): def _ignore(self):
files = self._ignorefiles() files = self._ignorefiles()
if not files: if not files:
if self._pendingmode is not None and self._pendingmode != mode: if self._pendingmode is not None and self._pendingmode != mode:
fp.close() fp.close()
raise error.Abort(_('working directory state may be ' raise error.Abort(_('working directory state may be '
'changed parallelly')) 'changed parallelly'))
self._pendingmode = mode self._pendingmode = mode
return fp return fp
def _read(self): def _read(self):
self._map = {} self._map = dirstatemap()
self._copymap = {} self._copymap = {}
# ignore HG_PENDING because identity is used only for writing # ignore HG_PENDING because identity is used only for writing
self._identity = util.filestat.frompath( self._identity = util.filestat.frompath(
self._opener.join(self._filename)) self._opener.join(self._filename))
try: try:
fp = self._opendirstatefile() fp = self._opendirstatefile()
try: try:
st = fp.read() st = fp.read()
# all over 10,000 files, the size of a dirstate entry is 85 # all over 10,000 files, the size of a dirstate entry is 85
# bytes. The cost of resizing is significantly higher than the cost # bytes. The cost of resizing is significantly higher than the cost
# of filling in a larger presized dict, so subtract 20% from the # of filling in a larger presized dict, so subtract 20% from the
# size. # size.
# #
# This heuristic is imperfect in many ways, so in a future dirstate # This heuristic is imperfect in many ways, so in a future dirstate
# format update it makes sense to just record the number of entries # format update it makes sense to just record the number of entries
# on write. # on write.
self._map = parsers.dict_new_presized(len(st) / 71) self._map._map = parsers.dict_new_presized(len(st) / 71)
# Python's garbage collector triggers a GC each time a certain number # Python's garbage collector triggers a GC each time a certain number
# of container objects (the number being defined by # of container objects (the number being defined by
# gc.get_threshold()) are allocated. parse_dirstate creates a tuple # gc.get_threshold()) are allocated. parse_dirstate creates a tuple
# for each file in the dirstate. The C version then immediately marks # for each file in the dirstate. The C version then immediately marks
# them as not to be tracked by the collector. However, this has no # them as not to be tracked by the collector. However, this has no
# effect on when GCs are triggered, only on what objects the GC looks # effect on when GCs are triggered, only on what objects the GC looks
# into. This means that O(number of files) GCs are unavoidable. # into. This means that O(number of files) GCs are unavoidable.
# Depending on when in the process's lifetime the dirstate is parsed, # Depending on when in the process's lifetime the dirstate is parsed,
# this can get very expensive. As a workaround, disable GC while # this can get very expensive. As a workaround, disable GC while
# 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._map, self._copymap, st)
if not self._dirtypl: if not self._dirtypl:
self._pl = p self._pl = p
def invalidate(self): def invalidate(self):
'''Causes the next access to reread the dirstate. '''Causes the next access to reread the dirstate.
This is different from localrepo.invalidatedirstate() because it always This is different from localrepo.invalidatedirstate() because it always
rereads the dirstate. Use localrepo.invalidatedirstate() if you want to rereads the dirstate. Use localrepo.invalidatedirstate() if you want to
- version provided via command arguments - version provided via command arguments
''' '''
if self._checkcase: if self._checkcase:
return self._normalize(path, isknown, ignoremissing) return self._normalize(path, isknown, ignoremissing)
return path return path
def clear(self): def clear(self):
self._map = {} self._map = dirstatemap()
self._nonnormalset = set() self._nonnormalset = set()
self._otherparentset = set() self._otherparentset = set()
if "_dirs" in self.__dict__: if "_dirs" in self.__dict__:
delattr(self, "_dirs") delattr(self, "_dirs")
self._copymap = {} self._copymap = {}
self._pl = [nullid, nullid] self._pl = [nullid, nullid]
self._lastnormaltime = 0 self._lastnormaltime = 0
self._updatedfiles.clear() self._updatedfiles.clear()
# multiple of n seconds # multiple of n seconds
clock = time.time() clock = time.time()
start = int(clock) - (int(clock) % delaywrite) start = int(clock) - (int(clock) % delaywrite)
end = start + delaywrite end = start + delaywrite
time.sleep(end - clock) time.sleep(end - clock)
now = end # trust our estimate that the end is near now now = end # trust our estimate that the end is near now
break break
st.write(parsers.pack_dirstate(self._map, self._copymap, self._pl, now)) st.write(parsers.pack_dirstate(self._map._map, self._copymap, self._pl,
now))
self._nonnormalset, self._otherparentset = nonnormalentries(self._map) self._nonnormalset, self._otherparentset = nonnormalentries(self._map)
st.close() st.close()
self._lastnormaltime = 0 self._lastnormaltime = 0
self._dirty = self._dirtypl = False self._dirty = self._dirtypl = False
def _dirignore(self, f): def _dirignore(self, f):
if f == '.': if f == '.':
return False return False
badfn(ff, badtype(kind)) badfn(ff, badtype(kind))
if nf in dmap: if nf in dmap:
results[nf] = None results[nf] = None
except OSError as inst: # nf not found on disk - it is dirstate only except OSError as inst: # nf not found on disk - it is dirstate only
if nf in dmap: # does it exactly match a missing file? if nf in dmap: # does it exactly match a missing file?
results[nf] = None results[nf] = None
else: # does it match a missing directory? else: # does it match a missing directory?
if alldirs is None: if alldirs is None:
alldirs = util.dirs(dmap) alldirs = util.dirs(dmap._map)
if nf in alldirs: if nf in alldirs:
if matchedir: if matchedir:
matchedir(nf) matchedir(nf)
notfoundadd(nf) notfoundadd(nf)
else: else:
badfn(ff, encoding.strtolocal(inst.strerror)) badfn(ff, encoding.strtolocal(inst.strerror))
# Case insensitive filesystems cannot rely on lstat() failing to detect # Case insensitive filesystems cannot rely on lstat() failing to detect
# changes of dirstate out after restoring from backup file # changes of dirstate out after restoring from backup file
self.invalidate() self.invalidate()
filename = self._actualfilename(tr) filename = self._actualfilename(tr)
self._opener.rename(backupname, filename, checkambig=True) self._opener.rename(backupname, filename, checkambig=True)
def clearbackup(self, tr, backupname): def clearbackup(self, tr, backupname):
'''Clear backup file''' '''Clear backup file'''
self._opener.unlink(backupname) self._opener.unlink(backupname)
class dirstatemap(object):
indygregUnsubmitted
Not Done

Do we want this inherit from collections.MutableMapping?

A good reason to say "no" is KISS, especially if we'll be having multiple backends.

indygreg: Do we want this inherit from `collections.MutableMapping`? A good reason to say "no" is KISS…
durhamAuthorUnsubmitted
Not Done

For now I'd say no, until we've done at least one other backend implementation and have seen that MutableMapping is still appropriate. In particular, we might not want to allow iteration without a matcher, to discourage O(working copy) operations.

durham: For now I'd say no, until we've done at least one other backend implementation and have seen…
martinvonzUnsubmitted
Not Done

I was wondering if all this delegation would be noticeably costly, but I couldn't think of a case where it is (no significant change in "hg files -0 | xargs -0 touch; time ~/hg/hg st", for example)

martinvonz: I was wondering if all this delegation would be noticeably costly, but I couldn't think of a…
def __init__(self):
self._map = {}
def iteritems(self):
return self._map.iteritems()
indygregUnsubmitted
Not Done

Given that iteritems() no longer exists in Python 3, should we change this to items() and standardize on the modern convention?

indygreg: Given that `iteritems()` no longer exists in Python 3, should we change this to `items()` and…
durhamAuthorUnsubmitted
Not Done

Do we do that anywhere else in the code base already? My initial tendency is to just follow existing patterns, especially during a refactor, to minimize churn.

durham: Do we do that anywhere else in the code base already? My initial tendency is to just follow…
def __iter__(self):
return iter(self._map)
def get(self, key, default=None):
return self._map.get(key, default)
def __contains__(self, key):
return key in self._map
def __setitem__(self, key, value):
self._map[key] = value
def __getitem__(self, key):
return self._map[key]
def __delitem__(self, key):
del self._map[key]
def keys(self):
return self._map.keys()