This is an archive of the discontinued Mercurial Phabricator instance.

dirstate: avoid reading the map when possible (issue5713) (issue5717)
ClosedPublic

Authored by durham on Oct 26 2017, 7:25 PM.

Download Raw Diff

Details

Reviewers

lothiraldan
yuja

Group Reviewers

hg-reviewers

Commits

rHG6e66033f91cc: dirstate: avoid reading the map when possible (issue5713) (issue5717)

Summary

Before the recent refactor, we would not load the entire map until it was
accessed. As part of the refactor, that got lost and even just trying to load
the dirstate parents would load the whole map. This caused a perf regression
(issue5713) and a regression with static http serving (issue5717).

Making it lazy loaded again fixes both.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durham created this revision.Oct 26 2017, 7:25 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 26 2017, 7:25 PM

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

This makes Windows mostly happy again, but now I'm getting the following diff. Any ideas?

diff --git a/tests/test-static-http.t b/tests/test-static-http.t

a/tests/test-static-http.t

+++ b/tests/test-static-http.t
@@ -221,34 +221,31 @@

$ cat server.log | sed -n -e 's|.*GET \(/[^ ]*\).*|\1|p' | sort -u
/.hg/bookmarks
/.hg/bookmarks.current

/.hg/cache/hgtagsfnodes1 /.hg/requires /.hg/store/00changelog.i /.hg/store/00manifest.i /.hg/store/data/%7E2ehgsub.i /.hg/store/data/%7E2ehgsubstate.i /.hg/store/data/a.i

+ /.hg/cache/hgtagsfnodes1 (glob)

/notarepo/.hg/00changelog.i
/notarepo/.hg/requires
/remote-with-names/.hg/bookmarks
/remote-with-names/.hg/bookmarks.current

/remote-with-names/.hg/cache/branch2-served
/remote-with-names/.hg/cache/hgtagsfnodes1
/remote-with-names/.hg/cache/tags2-served /remote-with-names/.hg/localtags /remote-with-names/.hg/requires /remote-with-names/.hg/store/00changelog.i /remote-with-names/.hg/store/00manifest.i /remote-with-names/.hg/store/data/%7E2ehgtags.i /remote-with-names/.hg/store/data/foo.i

+ /remote-with-names/.hg/cache/branch2-base (glob)
+ /remote-with-names/.hg/cache/branch2-immutable (glob)
+ /remote-with-names/.hg/cache/branch2-served (glob)
+ /remote-with-names/.hg/cache/hgtagsfnodes1 (glob)
+ /remote-with-names/.hg/cache/rbc-names-v1 (glob)
+ /remote-with-names/.hg/cache/tags2-served (glob)

/remote/.hg/bookmarks
/remote/.hg/bookmarks.current

/remote/.hg/cache/branch2-base
/remote/.hg/cache/branch2-immutable
/remote/.hg/cache/branch2-served
/remote/.hg/cache/hgtagsfnodes1
/remote/.hg/cache/rbc-names-v1
/remote/.hg/cache/tags2-served /remote/.hg/localtags /remote/.hg/requires /remote/.hg/store/00changelog.i

@@ -257,6 +254,12 @@

/remote/.hg/store/data/%7E2ehgtags.i
/remote/.hg/store/data/bar.i
/remote/.hg/store/data/quux.i

+ /remote/.hg/cache/branch2-base (glob)
+ /remote/.hg/cache/branch2-immutable (glob)
+ /remote/.hg/cache/branch2-served (glob)
+ /remote/.hg/cache/hgtagsfnodes1 (glob)
+ /remote/.hg/cache/rbc-names-v1 (glob)
+ /remote/.hg/cache/tags2-served (glob)

/remotempty/.hg/bookmarks
/remotempty/.hg/bookmarks.current
/remotempty/.hg/requires

@@ -264,9 +267,9 @@

/remotempty/.hg/store/00manifest.i
/sub/.hg/bookmarks
/sub/.hg/bookmarks.current

/sub/.hg/cache/hgtagsfnodes1 /sub/.hg/requires /sub/.hg/store/00changelog.i /sub/.hg/store/00manifest.i /sub/.hg/store/data/%7E2ehgtags.i /sub/.hg/store/data/test.i

+ /sub/.hg/cache/hgtagsfnodes1 (glob)

Try that again without the reformatting

diff --git a/tests/test-static-http.t b/tests/test-static-http.t
--- a/tests/test-static-http.t
+++ b/tests/test-static-http.t
@@ -221,34 +221,31 @@
   $ cat server.log | sed -n -e 's|.*GET \(/[^ ]*\).*|\1|p' | sort -u
   /.hg/bookmarks
   /.hg/bookmarks.current
-  /.hg/cache/hgtagsfnodes1
   /.hg/requires
   /.hg/store/00changelog.i
   /.hg/store/00manifest.i
   /.hg/store/data/%7E2ehgsub.i
   /.hg/store/data/%7E2ehgsubstate.i
   /.hg/store/data/a.i
+  /.hg/cache/hgtagsfnodes1 (glob)
   /notarepo/.hg/00changelog.i
   /notarepo/.hg/requires
   /remote-with-names/.hg/bookmarks
   /remote-with-names/.hg/bookmarks.current
-  /remote-with-names/.hg/cache/branch2-served
-  /remote-with-names/.hg/cache/hgtagsfnodes1
-  /remote-with-names/.hg/cache/tags2-served
   /remote-with-names/.hg/localtags
   /remote-with-names/.hg/requires
   /remote-with-names/.hg/store/00changelog.i
   /remote-with-names/.hg/store/00manifest.i
   /remote-with-names/.hg/store/data/%7E2ehgtags.i
   /remote-with-names/.hg/store/data/foo.i
+  /remote-with-names/.hg/cache/branch2-base (glob)
+  /remote-with-names/.hg/cache/branch2-immutable (glob)
+  /remote-with-names/.hg/cache/branch2-served (glob)
+  /remote-with-names/.hg/cache/hgtagsfnodes1 (glob)
+  /remote-with-names/.hg/cache/rbc-names-v1 (glob)
+  /remote-with-names/.hg/cache/tags2-served (glob)
   /remote/.hg/bookmarks
   /remote/.hg/bookmarks.current
-  /remote/.hg/cache/branch2-base
-  /remote/.hg/cache/branch2-immutable
-  /remote/.hg/cache/branch2-served
-  /remote/.hg/cache/hgtagsfnodes1
-  /remote/.hg/cache/rbc-names-v1
-  /remote/.hg/cache/tags2-served
   /remote/.hg/localtags
   /remote/.hg/requires
   /remote/.hg/store/00changelog.i
@@ -257,6 +254,12 @@
   /remote/.hg/store/data/%7E2ehgtags.i
   /remote/.hg/store/data/bar.i
   /remote/.hg/store/data/quux.i
+  /remote/.hg/cache/branch2-base (glob)
+  /remote/.hg/cache/branch2-immutable (glob)
+  /remote/.hg/cache/branch2-served (glob)
+  /remote/.hg/cache/hgtagsfnodes1 (glob)
+  /remote/.hg/cache/rbc-names-v1 (glob)
+  /remote/.hg/cache/tags2-served (glob)
   /remotempty/.hg/bookmarks
   /remotempty/.hg/bookmarks.current
   /remotempty/.hg/requires
@@ -264,9 +267,9 @@
   /remotempty/.hg/store/00manifest.i
   /sub/.hg/bookmarks
   /sub/.hg/bookmarks.current
-  /sub/.hg/cache/hgtagsfnodes1
   /sub/.hg/requires
   /sub/.hg/store/00changelog.i
   /sub/.hg/store/00manifest.i
   /sub/.hg/store/data/%7E2ehgtags.i
   /sub/.hg/store/data/test.i
+  /sub/.hg/cache/hgtagsfnodes1 (glob)

It looks good to me.

I did some quick and dirty performance check and I see a performance improvement. On the mozilla-central repository, without this change, I get these results:

$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 154 ms +- 2 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 154 ms +- 2 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 156 ms +- 3 ms

And with this changeset I get better results:

$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 119 ms +- 3 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 120 ms +- 3 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 123 ms +- 7 ms

Where perf is https://perf.readthedocs.io/en/latest/

(I don't read the patch content yet.)

I just made static-http repo not see dirstate because of the issue5717.
I don't know the issue5713, but a fewer loading over HTTP should be generally better.

Here are some measures 0865d25e8a8a:

$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 109 ms +- 3 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 110 ms +- 2 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 111 ms +- 3 ms

And here are the mesaures on c36c3fa7d35b:

$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 166 ms +- 5 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 165 ms +- 3 ms
$ python -m perf command /home/lothiraldan/project/mercurial/mercurial/hg id -r .
.....................
command: Mean +- std dev: 166 ms +- 5 ms

So it's seems that this patch solves most of the performance regression, but with so many changesets since c36c3fa7d35b I'm not sure we can do better.

@mharbison72 your test output looks like the sort order changed? Does it consistently repro before and after this patch? The sort order is from the sort -u in the test, so I can't imagine this affecting it.

durham added a child revision: D1257: dirstate: remove excess attribute lookups for dirstate.status (issue5714).Oct 27 2017, 4:15 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

Queued up to this patch, thanks.

mercurial/dirstate.py
132	Perhaps this can be moved to init() since calling dirstatemap() doesn't do any IO now.
1209	Nit: dirstatemap.read() could be a private function.

Closed by commit rHG6e66033f91cc: dirstate: avoid reading the map when possible (issue5713) (issue5717) (authored by durham). · Explain WhyOct 28 2017, 4:09 AM

This revision was automatically updated to reflect the committed changes.

After rereading the numbers, I realize that with this patch we are still 10ms (around 10%) slower than before. The numbers before 0865d25e8a8a were [109, 110, 111]ms and with this patch we are at [119, 120, 123]ms.

We should take a look at it.

@durham It did turn out to be a sort issue: some paths had '\' and were (partially?) converted to '/' by the test runner _after_ the sort. Piping through sed to fix the paths beforehand reduces it to this:

--- c:/Users/Matt/projects/hg/tests/test-static-http.t
+++ c:/Users/Matt/projects/hg/tests/test-static-http.t.err
@@ -232,8 +232,11 @@
   /notarepo/.hg/requires
   /remote-with-names/.hg/bookmarks
   /remote-with-names/.hg/bookmarks.current
+  /remote-with-names/.hg/cache/branch2-base
+  /remote-with-names/.hg/cache/branch2-immutable
   /remote-with-names/.hg/cache/branch2-served
   /remote-with-names/.hg/cache/hgtagsfnodes1
+  /remote-with-names/.hg/cache/rbc-names-v1
   /remote-with-names/.hg/cache/tags2-served
   /remote-with-names/.hg/localtags
   /remote-with-names/.hg/requires

Not sure why yet, but this patch isn't the problem.

Revision Contents
Changeset List

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

Status	Author	Revision
Closed	durham	D1257 dirstate: remove excess attribute lookups for dirstate.status (issue5714)
Closed	durham	D1253 dirstate: avoid reading the map when possible (issue5713) (issue5717)
Closed	durham	D1252 dirstate: move clear onto dirstatemap class

Diff 3129

mercurial/dirstate.py

	that modify the dirstate parent.			that modify the dirstate parent.
	'''			'''
	return self._parentwriters > 0			return self._parentwriters > 0

	@propertycache			@propertycache
	def _map(self):			def _map(self):
	'''Return the dirstate contents as a map from filename to			'''Return the dirstate contents as a map from filename to
	(state, mode, size, time).'''			(state, mode, size, time).'''
	self._read()			self._map = dirstatemap(self._ui, self._opener, self._root)
				yujaUnsubmitted Not Done Perhaps this can be moved to init() since calling dirstatemap() doesn't do any IO now. yuja: Perhaps this can be moved to __init__() since calling dirstatemap() doesn't do any IO now.
	return self._map			return self._map

	@property			@property
	def _sparsematcher(self):			def _sparsematcher(self):
	"""The matcher for the sparse checkout.			"""The matcher for the sparse checkout.

	The working directory may not include every file from a manifest. The			The working directory may not include every file from a manifest. The
	matcher obtained by this property will match a path if it is to be			matcher obtained by this property will match a path if it is to be
	# replacing the underlying file			# replacing the underlying file
	ce = self._filecache['_branch']			ce = self._filecache['_branch']
	if ce:			if ce:
	ce.refresh()			ce.refresh()
	except: # re-raises			except: # re-raises
	f.discard()			f.discard()
	raise			raise

	def _read(self):
	self._map = dirstatemap(self._ui, self._opener, self._root)
	self._map.read()

	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
	check whether the dirstate has changed before rereading it.'''			check whether the dirstate has changed before rereading it.'''

	for a in ("_map", "_branch", "_ignore"):			for a in ("_map", "_branch", "_ignore"):

	class dirstatemap(object):			class dirstatemap(object):
	def __init__(self, ui, opener, root):			def __init__(self, ui, opener, root):
	self._ui = ui			self._ui = ui
	self._opener = opener			self._opener = opener
	self._root = root			self._root = root
	self._filename = 'dirstate'			self._filename = 'dirstate'

	self._map = {}
	self.copymap = {}
	self._parents = None			self._parents = None
	self._dirtyparents = False			self._dirtyparents = False

	# for consistent view between _pl() and _read() invocations			# for consistent view between _pl() and _read() invocations
	self._pendingmode = None			self._pendingmode = None

				@propertycache
				def _map(self):
				self._map = {}
				self.read()
				yujaUnsubmitted Not Done Nit: dirstatemap.read() could be a private function. yuja: Nit: dirstatemap.read() could be a private function.
				return self._map

				@propertycache
				def copymap(self):
				self.copymap = {}
				self._map
				return self.copymap

	def clear(self):			def clear(self):
	self._map = {}			self._map = {}
	self.copymap = {}			self.copymap = {}
	self.setparents(nullid, nullid)			self.setparents(nullid, nullid)

	def iteritems(self):			def iteritems(self):
	return self._map.iteritems()			return self._map.iteritems()

	@propertycache			@propertycache
	def otherparentset(self):			def otherparentset(self):
	nonnorm, otherparents = self.nonnormalentries()			nonnorm, otherparents = self.nonnormalentries()
	self.nonnormalset = nonnorm			self.nonnormalset = nonnorm
	return otherparents			return otherparents

	@propertycache			@propertycache
	def identity(self):			def identity(self):
	self.read()			self._map
	return self.identity			return self.identity

	@propertycache			@propertycache
	def dirfoldmap(self):			def dirfoldmap(self):
	f = {}			f = {}
	normcase = util.normcase			normcase = util.normcase
	for name in self.dirs:			for name in self.dirs:
	f[normcase(name)] = name			f[normcase(name)] = name
	return f			return f

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3129		Oct 26 2017, 7:25 PM	★	★
Diff 2	3146	rHG6e66033f91ccfb7666ec0a25d19954673269c61a	Oct 26 2017, 7:15 PM	★	★