This is an archive of the discontinued Mercurial Phabricator instance.

Differential D63

rebase: use one dirstateguard for when using rebase.singletransaction
AbandonedPublic

Authored by durham on Jul 12 2017, 1:56 PM.

Download Raw Diff

Details

Reviewers

martinvonz

Group Reviewers

Restricted Project

Summary

This was previously landed as 2519994d25ca but backed out in b63351f6a2 because
it broke hooks mid-rebase and caused conflict resolution data loss in the event
of unexpected exceptions. This new version adds the behavior back but behind a
config flag, since the performance improvement is notable in large repositories.

The old commit message was:

Recently we switched rebases to run the entire rebase inside a single
transaction, which dramatically improved the speed of rebases in repos with
large working copies. Let's also move the dirstate into a single dirstateguard
to get the same benefits. This let's us avoid serializing the dirstate after
each commit.

In a large repo, rebasing 27 commits is sped up by about 20%.

I believe the test changes are because us touching the dirstate gave the
transaction something to actually rollback.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durham created this revision.Jul 12 2017, 1:56 PM

Herald added a subscriber: mercurial-devel. · View Herald TranscriptJul 12 2017, 1:56 PM

durham added reviewers: Restricted Project, martinvonz.Jul 12 2017, 1:58 PM

durham added 1 blocking reviewer(s): martinvonz.

durham removed 1 blocking reviewer(s): martinvonz.Jul 12 2017, 2:00 PM

martinvonz accepted this revision.Jul 12 2017, 4:27 PM

martinvonz added inline comments.

tests/test-rebase-base.t
400	I haven't seen this style of starting a comment with '-' elsewhere, so it feels more confusing than helpful. I can see the point of doing it to indicate intra-testcase comments (as opposed to the per-testcase comments like on line 382), but only if we try to do it somewhat consistently. Should I drop it in flight or do you want us to start using this style?

This revision is now accepted and ready to land.Jul 12 2017, 4:27 PM

phillco added a subscriber: phillco.Jul 12 2017, 4:31 PM

phillco added inline comments.

tests/test-rebase-base.t
400	It'd be nice to have such a style, I think.

martinvonz added inline comments.Jul 12 2017, 4:47 PM

tests/test-rebase-base.t
400	I'd be fine with that, but I don't want to decide it right now, so I've dropped the '- ' in flight and queued this.

Update sent as https://phab.mercurial-scm.org/D134 . Not sure how to get it to update this diff instead, so abandoning.

Revision Contents
Changeset List

			Path	Packages
M			hgext/rebase.py (50 lines)
M			mercurial/dirstateguard.py (10 lines)
M			tests/test-rebase-base.t (37 lines)

	Status	Author	Revision
	Abandoned	durham	D63 rebase: use one dirstateguard for when using rebase.singletransaction
	Abandoned	durham	D62 rebase: add config to move rebase into a single transaction

Diff 88

hgext/rebase.py

	commitmsg = 'Collapsed revision'			commitmsg = 'Collapsed revision'
	for rebased in self.state:			for rebased in self.state:
	if rebased not in self.skipped and\			if rebased not in self.skipped and\
	self.state[rebased] > nullmerge:			self.state[rebased] > nullmerge:
	commitmsg += '\n* %s' % repo[rebased].description()			commitmsg += '\n* %s' % repo[rebased].description()
	editopt = True			editopt = True
	editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)			editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
	revtoreuse = max(self.state)			revtoreuse = max(self.state)

				dsguard = util.nullcontextmanager()
				if ui.configbool('rebase', 'singletransaction'):
				dsguard = dirstateguard.dirstateguard(repo, 'rebase')
				with dsguard:
				try:
	newnode = concludenode(repo, revtoreuse, p1, self.external,			newnode = concludenode(repo, revtoreuse, p1, self.external,
	commitmsg=commitmsg,			commitmsg=commitmsg,
	extrafn=_makeextrafn(self.extrafns),			extrafn=_makeextrafn(self.extrafns),
	editor=editor,			editor=editor,
	keepbranches=self.keepbranchesf,			keepbranches=self.keepbranchesf,
	date=self.date)			date=self.date)
				except error.InterventionRequired:
				dsguard.close()
				raise

	if newnode is None:			if newnode is None:
	newrev = self.dest			newrev = self.dest
	else:			else:
	newrev = repo[newnode].rev()			newrev = repo[newnode].rev()
	for oldrev in self.state.iterkeys():			for oldrev in self.state.iterkeys():
	if self.state[oldrev] > nullmerge:			if self.state[oldrev] > nullmerge:
	self.state[oldrev] = newrev			self.state[oldrev] = newrev

	return retcode			return retcode
	else:			else:
	dest, rebaseset = _definesets(ui, repo, destf, srcf, basef, revf,			dest, rebaseset = _definesets(ui, repo, destf, srcf, basef, revf,
	destspace=destspace)			destspace=destspace)
	retcode = rbsrt._preparenewrebase(dest, rebaseset)			retcode = rbsrt._preparenewrebase(dest, rebaseset)
	if retcode is not None:			if retcode is not None:
	return retcode			return retcode

				tr = util.nullcontextmanager()
				dsguard = util.nullcontextmanager()

	singletr = ui.configbool('rebase', 'singletransaction')			singletr = ui.configbool('rebase', 'singletransaction')
	if singletr:			if singletr:
	tr = repo.transaction('rebase')			tr = repo.transaction('rebase')
	else:
	tr = util.nullcontextmanager()
	with tr:			with tr:
				if singletr:
				dsguard = dirstateguard.dirstateguard(repo, 'rebase')
				with dsguard:
	try:			try:
	rbsrt._performrebase(tr if singletr else None)			rbsrt._performrebase(tr if singletr else None)
	except error.InterventionRequired:			except error.InterventionRequired:
				dsguard.close()
	tr.close()			tr.close()
	raise			raise

	rbsrt._finishrebase()			rbsrt._finishrebase()

	def _definesets(ui, repo, destf=None, srcf=None, basef=None, revf=None,			def _definesets(ui, repo, destf=None, srcf=None, basef=None, revf=None,
	destspace=None):			destspace=None):
	"""use revisions argument to define destination and rebase set			"""use revisions argument to define destination and rebase set
	"""			"""
	if revf is None:			if revf is None:
	(max(destancestors),			(max(destancestors),
	', '.join(str(p) for p in sorted(parents))))			', '.join(str(p) for p in sorted(parents))))

	def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None,			def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None,
	keepbranches=False, date=None):			keepbranches=False, date=None):
	'''Commit the wd changes with parents p1 and p2. Reuse commit info from rev			'''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
	but also store useful information in extra.			but also store useful information in extra.
	Return node of committed revision.'''			Return node of committed revision.'''
				dsguard = util.nullcontextmanager()
				if not repo.ui.configbool('rebase', 'singletransaction'):
	dsguard = dirstateguard.dirstateguard(repo, 'rebase')			dsguard = dirstateguard.dirstateguard(repo, 'rebase')
	try:			with dsguard:
	repo.setparents(repo[p1].node(), repo[p2].node())			repo.setparents(repo[p1].node(), repo[p2].node())
	ctx = repo[rev]			ctx = repo[rev]
	if commitmsg is None:			if commitmsg is None:
	commitmsg = ctx.description()			commitmsg = ctx.description()
	keepbranch = keepbranches and repo[p1].branch() != ctx.branch()			keepbranch = keepbranches and repo[p1].branch() != ctx.branch()
	extra = {'rebase_source': ctx.hex()}			extra = {'rebase_source': ctx.hex()}
	if extrafn:			if extrafn:
	extrafn(ctx, extra)			extrafn(ctx, extra)

	destphase = max(ctx.phase(), phases.draft)			destphase = max(ctx.phase(), phases.draft)
	overrides = {('phases', 'new-commit'): destphase}			overrides = {('phases', 'new-commit'): destphase}
	with repo.ui.configoverride(overrides, 'rebase'):			with repo.ui.configoverride(overrides, 'rebase'):
	if keepbranch:			if keepbranch:
	repo.ui.setconfig('ui', 'allowemptycommit', True)			repo.ui.setconfig('ui', 'allowemptycommit', True)
	# Commit might fail if unresolved files exist			# Commit might fail if unresolved files exist
	if date is None:			if date is None:
	date = ctx.date()			date = ctx.date()
	newnode = repo.commit(text=commitmsg, user=ctx.user(),			newnode = repo.commit(text=commitmsg, user=ctx.user(),
	date=date, extra=extra, editor=editor)			date=date, extra=extra, editor=editor)

	repo.dirstate.setbranch(repo[newnode].branch())			repo.dirstate.setbranch(repo[newnode].branch())
	dsguard.close()
	return newnode			return newnode
	finally:
	release(dsguard)

	def rebasenode(repo, rev, p1, base, state, collapse, dest):			def rebasenode(repo, rev, p1, base, state, collapse, dest):
	'Rebase a single revision rev on top of p1 using base as merge ancestor'			'Rebase a single revision rev on top of p1 using base as merge ancestor'
	# Merge phase			# Merge phase
	# Update to destination and merge it with local			# Update to destination and merge it with local
	if repo['.'].rev() != p1:			if repo['.'].rev() != p1:
	repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))			repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
	mergemod.update(repo, p1, False, True)			mergemod.update(repo, p1, False, True)

mercurial/dirstateguard.py

	def __del__(self):			def __del__(self):
	if self._active: # still active			if self._active: # still active
	# this may occur, even if this class is used correctly:			# this may occur, even if this class is used correctly:
	# for example, releasing other resources like transaction			# for example, releasing other resources like transaction
	# may raise exception before ``dirstateguard.release`` in			# may raise exception before ``dirstateguard.release`` in
	# ``release(tr, ....)``.			# ``release(tr, ....)``.
	self._abort()			self._abort()

				def __enter__(self):
				return self

				def __exit__(self, exc_type, exc_val, exc_tb):
				try:
				if exc_type is None:
				self.close()
				finally:
				self.release()

	def close(self):			def close(self):
	if not self._active: # already inactivated			if not self._active: # already inactivated
	msg = (_("can't close already inactivated backup: dirstate%s")			msg = (_("can't close already inactivated backup: dirstate%s")
	% self._suffix)			% self._suffix)
	raise error.Abort(msg)			raise error.Abort(msg)

	self._repo.dirstate.clearbackup(self._repo.currenttransaction(),			self._repo.dirstate.clearbackup(self._repo.currenttransaction(),
	self._suffix)			self._suffix)

tests/test-rebase-base.t

	+---o 3: B			+---o 3: B
	\| \|/			\| \|/
	\| o 2: Z			\| o 2: Z
	\| \|			\| \|
	o \| 1: C			o \| 1: C
	/			/
	o 0: A			o 0: A

				Rebasing using a single transaction

				$ hg init singletr && cd singletr
				$ cat >> .hg/hgrc <<EOF
				> [rebase]
				> singletransaction=True
				> EOF
				$ hg debugdrawdag <<'EOF'
				> Z
				> \|
				> \| D
				> \| \|
				> \| C
				> \| \|
				> Y B
				> \|/
				> A
				> EOF
				- We should only see two status stored messages. One from the start, one from
				martinvonzUnsubmitted Not Done I haven't seen this style of starting a comment with '-' elsewhere, so it feels more confusing than helpful. I can see the point of doing it to indicate intra-testcase comments (as opposed to the per-testcase comments like on line 382), but only if we try to do it somewhat consistently. Should I drop it in flight or do you want us to start using this style? martinvonz: I haven't seen this style of starting a comment with '-' elsewhere, so it feels more confusing…
				phillcoUnsubmitted Not Done It'd be nice to have such a style, I think. phillco: It'd be nice to have such a style, I think.
				martinvonzUnsubmitted Not Done I'd be fine with that, but I don't want to decide it right now, so I've dropped the '- ' in flight and queued this. martinvonz: I'd be fine with that, but I don't want to decide it right now, so I've dropped the '- ' in…
				- the end.
				$ hg rebase --debug -b D -d Z \| grep 'status stored'
				rebase status stored
				rebase status stored
				$ hg tglog
				o 5: D
				\|
				o 4: C
				\|
				o 3: B
				\|
				o 2: Z
				\|
				o 1: Y
				\|
				o 0: A

				$ cd ..

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	88			Jul 12 2017, 1:56 PM	★	★