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.

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
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.

durham abandoned this revision.Jul 18 2017, 11:15 AM

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

PathPackages
Mhgext/rebase.py (50 lines)
Mmercurial/dirstateguard.py (10 lines)
Mtests/test-rebase-base.t (37 lines)

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 ..