This is an archive of the discontinued Mercurial Phabricator instance.

Differential D62

rebase: add config to move rebase into a single transaction
AbandonedPublic

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

Details

Reviewers
martinvonz
phillco
Group Reviewers
Restricted Project
Summary

This was previously landed as cf8ad0e6c0e4 but backed out in a5abaa81fa because
it broke hook 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 next patch adds a test covering this config.

The old commit message was:

Previously, rebasing would open several transaction over the course of rebasing
several commits. Opening a transaction can have notable overhead (like copying
the dirstate) which can add up when rebasing many commits.

This patch adds a single large transaction around the actual commit rebase
operation, with a catch for intervention which serializes the current state if
we need to drop back to the terminal for user intervention. Amazingly, almost
all the tests seem to pass.

On large repos with large working copies, this can speed up rebasing 7 commits
by 25%. I'd expect the percentage to be a bit larger for rebasing even more
commits.

There are minor test changes because we're rolling back the entire transaction
during unexpected exceptions instead of just stopping mid-rebase, so there's no
more backup bundle. It also leave an unknown file in the working copy, since our
clean up 'hg update' doesn't delete unknown files.

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
phillco added a subscriber: phillco.Jul 12 2017, 3:37 PM

lgtm

mercurial/util.py
592–594

Thanks, I was looking for something like this on one of my branches!

phillco accepted this revision as: phillco.Jul 12 2017, 3:51 PM

Actual LGTM.

Though, it's interesting, it looks like I have the ability to accept as hg. I presume I'll need to keep unchecking that so I don't accidentally pretend to be a core reviewer.

This revision is now accepted and ready to land.Jul 12 2017, 3:51 PM
martinvonz accepted this revision.Jul 12 2017, 4:16 PM
martinvonz added inline comments.
mercurial/util.py
592

The body looks more like a "nulltransaction" (I think close(), release(), abort() are part of transaction's interface), but this is fine with me anyway.

martinvonz added inline comments.Jul 12 2017, 4:54 PM
hgext/rebase.py
721

Won't this and tr's context manager together double-close the transaction?

durham added inline comments.Jul 13 2017, 5:22 PM
hgext/rebase.py
721

The context manage will only call tr.release(),since it's an exception exit path.

durham added a comment.Jul 13 2017, 5:23 PM
In D62#790, @phillco wrote:

Though, it's interesting, it looks like I have the ability to accept as hg. I presume I'll need to keep unchecking that so I don't accidentally pretend to be a core reviewer.

Anyone can accept as #hg, but only reviewers can accept as hg-reviewers, which is a blocking reviewer on all new diffs (but not this one because it was created before that herald rule was added I think)

martinvonz added inline comments.Jul 13 2017, 6:26 PM
hgext/rebase.py
721

Ah, right.

Anyway, once D66 is in, you should be able to simplify this to:

tr = None
if singletr = ui.configbool('rebase', 'singletransaction'):
    tr = repo.transaction('rebase')
with util.acceptintervention(tr):
    rbsrt._performrebase(tr)

So it seems best to wait for that to land.

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

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

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (26 lines)
Mmercurial/util.py (14 lines)

Diff 87

hgext/rebase.py

[self.dest], [self.dest],
inclusive=True) inclusive=True)
self.external = externalparent(self.repo, self.state, self.external = externalparent(self.repo, self.state,
self.destancestors) self.destancestors)
if dest.closesbranch() and not self.keepbranchesf: if dest.closesbranch() and not self.keepbranchesf:
self.ui.status(_('reopening closed branch head %s\n') % dest) self.ui.status(_('reopening closed branch head %s\n') % dest)
def _performrebase(self): def _performrebase(self, tr):
repo, ui, opts = self.repo, self.ui, self.opts repo, ui, opts = self.repo, self.ui, self.opts
if self.keepbranchesf: if self.keepbranchesf:
# insert _savebranch at the start of extrafns so if # insert _savebranch at the start of extrafns so if
# there's a user-provided extrafn it can clobber branch if # there's a user-provided extrafn it can clobber branch if
# desired # desired
self.extrafns.insert(0, _savebranch) self.extrafns.insert(0, _savebranch)
if self.collapsef: if self.collapsef:
branches = set() branches = set()
pos += 1 pos += 1
ui.status(_('rebasing %s\n') % desc) ui.status(_('rebasing %s\n') % desc)
ui.progress(_("rebasing"), pos, ("%d:%s" % (rev, ctx)), ui.progress(_("rebasing"), pos, ("%d:%s" % (rev, ctx)),
_('changesets'), total) _('changesets'), total)
p1, p2, base = defineparents(repo, rev, self.dest, p1, p2, base = defineparents(repo, rev, self.dest,
self.state, self.state,
self.destancestors, self.destancestors,
self.obsoletenotrebased) self.obsoletenotrebased)
self.storestatus() self.storestatus(tr=tr)
storecollapsemsg(repo, self.collapsemsg) storecollapsemsg(repo, self.collapsemsg)
if len(repo[None].parents()) == 2: if len(repo[None].parents()) == 2:
repo.ui.debug('resuming interrupted rebase\n') repo.ui.debug('resuming interrupted rebase\n')
else: else:
try: try:
ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
'rebase') 'rebase')
stats = rebasenode(repo, rev, p1, base, self.state, stats = rebasenode(repo, rev, p1, base, self.state,
Configuration Options: Configuration Options:
You can make rebase require a destination if you set the following config You can make rebase require a destination if you set the following config
option:: option::
[commands] [commands]
rebase.requiredest = True rebase.requiredest = True
By default, rebase will close the transaction after each commit. For
performance purposes, you can configure rebase to use a single transaction
across the entire rebase. WARNING: This setting introduces a significant
risk of losing the work you've done in a rebase if the rebase aborts
unexpectedly::
[rebase]
singletransaction = True
Return Values: Return Values:
Returns 0 on success, 1 if nothing to rebase or there are Returns 0 on success, 1 if nothing to rebase or there are
unresolved conflicts. unresolved conflicts.
""" """
rbsrt = rebaseruntime(repo, ui, opts) rbsrt = rebaseruntime(repo, ui, opts)
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
rbsrt._performrebase() singletr = ui.configbool('rebase', 'singletransaction')
if singletr:
tr = repo.transaction('rebase')
else:
tr = util.nullcontextmanager()
with tr:
try:
rbsrt._performrebase(tr if singletr else None)
except error.InterventionRequired:
tr.close()
martinvonzUnsubmitted
Not Done

Won't this and tr's context manager together double-close the transaction?

martinvonz: Won't this and tr's context manager together double-close the transaction?
durhamAuthorUnsubmitted
Not Done

The context manage will only call tr.release(),since it's an exception exit path.

durham: The context manage will only call tr.release(),since it's an exception exit path.
martinvonzUnsubmitted
Not Done

Ah, right.

Anyway, once D66 is in, you should be able to simplify this to:

tr = None
if singletr = ui.configbool('rebase', 'singletransaction'):
    tr = repo.transaction('rebase')
with util.acceptintervention(tr):
    rbsrt._performrebase(tr)

So it seems best to wait for that to land.

martinvonz: Ah, right. Anyway, once D66 is in, you should be able to simplify this to: tr = None if…
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:
revf = [] revf = []

mercurial/util.py

['b', 'a'] ['b', 'a']
''' '''
def __setitem__(self, key, value): def __setitem__(self, key, value):
if key in self: if key in self:
del self[key] del self[key]
super(sortdict, self).__setitem__(key, value) super(sortdict, self).__setitem__(key, value)
class nullcontextmanager(object):
martinvonzUnsubmitted
Not Done

The body looks more like a "nulltransaction" (I think close(), release(), abort() are part of transaction's interface), but this is fine with me anyway.

martinvonz: The body looks more like a "nulltransaction" (I think close(), release(), abort() are part of…
"""A no-op context manager.
"""
phillcoUnsubmitted
Not Done

Thanks, I was looking for something like this on one of my branches!

phillco: Thanks, I was looking for something like this on one of my branches!
def __enter__(self):
return self
def __exit__(self, exc_type, exc_val, exc_tb):
pass
def close(self):
pass
def release(self):
pass
def abort(self):
pass
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')