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.

Download Raw Diff

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

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durham created this revision.Sep 20 2017, 8:03 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 20 2017, 8:03 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

durham added a child revision: D753: dirstate: move nonnormalentries to dirstatemap.Sep 20 2017, 8:03 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.

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

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

			Path	Packages
M			mercurial/dirstate.py (48 lines)

Status	Author	Revision
Closed	durham	D759 dirstate: move parents source of truth to dirstatemap
Closed	durham	D758 dirstate: move parent reading to the dirstatemap class
Closed	durham	D757 dirstate: move opendirstatefile to dirstatemap
Closed	durham	D756 dirstate: move _copymap to dirstatemap
Closed	durham	D755 dirstate: move _dirs to dirstatemap
Closed	durham	D754 dirstate: move filefoldmap to dirstatemap
Closed	durham	D753 dirstate: move nonnormalentries to dirstatemap
Closed	durham	D752 dirstate: create new dirstatemap class

Diff 1947

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()

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1947		Sep 20 2017, 8:03 PM	★	★
Diff 2	2155	rHG42b68c9c2742e08cd9461cb4da4cc2465790c574	Sep 26 2017, 6:56 AM	★	★