This is an archive of the discontinued Mercurial Phabricator instance.

Differential D135

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

Authored by durham on Jul 18 2017, 11:09 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG609606d21765: rebase: use one dirstateguard for when using rebase.singletransaction
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.
(grafted from 9e3dc3a1638b9754b58a0cb26aaa75d868058109)
(grafted from 7d38b41d2266d9a02a15c64229fae0da5738dcec)

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.Jul 18 2017, 11:09 AM
martinvonz added a subscriber: martinvonz.Jul 18 2017, 11:31 AM
martinvonz added inline comments.
hgext/rebase.py
483

As in the previous patch, this can be None here and in the cases below

durham updated this revision to Diff 274.Jul 18 2017, 2:14 PM
durham marked an inline comment as done.Jul 18 2017, 2:15 PM
martinvonz added inline comments.Jul 18 2017, 4:41 PM
mercurial/util.py
612–617

Are these necessary? Looks like the nullcontextmanager() is now only used where a plain contextmanager is expected. That should also mean that you should be able to replace this class by:

@contextlib.contextmanager
def nullcontextmanager():
    yield
durham updated this revision to Diff 338.Jul 20 2017, 4:33 AM
durham marked an inline comment as done.Jul 20 2017, 4:34 AM
martinvonz accepted this revision.Jul 20 2017, 11:54 AM

Looks good, but we're in code freeze, so it will have to wait until after.

This revision is now accepted and ready to land.Jul 20 2017, 11:54 AM
Closed by commit rHG609606d21765: rebase: use one dirstateguard for when using rebase.singletransaction (authored by durham). · Explain WhyAug 1 2017, 4:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

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

Diff 483

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 = None
martinvonzUnsubmitted
Done

As in the previous patch, this can be None here and in the cases below

martinvonz: As in the previous patch, this can be None here and in the cases below
if ui.configbool('rebase', 'singletransaction'):
dsguard = dirstateguard.dirstateguard(repo, 'rebase')
with util.acceptintervention(dsguard):
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)
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
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 = None tr = None
if ui.configbool('rebase', 'singletransaction'): dsguard = None
singletr = ui.configbool('rebase', 'singletransaction')
if singletr:
tr = repo.transaction('rebase') tr = repo.transaction('rebase')
with util.acceptintervention(tr): with util.acceptintervention(tr):
if singletr:
dsguard = dirstateguard.dirstateguard(repo, 'rebase')
with util.acceptintervention(dsguard):
rbsrt._performrebase(tr) rbsrt._performrebase(tr)
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: %s") msg = (_("can't close already inactivated backup: %s")
% self._backupname) % self._backupname)
raise error.Abort(msg) raise error.Abort(msg)
self._repo.dirstate.clearbackup(self._repo.currenttransaction(), self._repo.dirstate.clearbackup(self._repo.currenttransaction(),
self._backupname) self._backupname)

mercurial/util.py

yield yield
tr.close() tr.close()
except error.InterventionRequired: except error.InterventionRequired:
tr.close() tr.close()
raise raise
finally: finally:
tr.release() tr.release()
@contextlib.contextmanager
def nullcontextmanager():
yield
class _lrucachenode(object): class _lrucachenode(object):
"""A node in a doubly linked list. """A node in a doubly linked list.
Holds a reference to nodes on either side as well as a key-value Holds a reference to nodes on either side as well as a key-value
pair for the dictionary entry. pair for the dictionary entry.
""" """
__slots__ = (u'next', u'prev', u'key', u'value') __slots__ = (u'next', u'prev', u'key', u'value')
def __init__(self): def __init__(self):
martinvonzUnsubmitted
Done

Are these necessary? Looks like the nullcontextmanager() is now only used where a plain contextmanager is expected. That should also mean that you should be able to replace this class by:

@contextlib.contextmanager
def nullcontextmanager():
    yield
martinvonz: Are these necessary? Looks like the nullcontextmanager() is now only used where a plain…
self.next = None self.next = None
self.prev = None self.prev = None
self.key = _notset self.key = _notset
self.value = None self.value = None
def markempty(self): def markempty(self):
"""Mark the node as emptied.""" """Mark the node as emptied."""

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