Patchwork [3,of,3,V2] histedit: add histedit.singletransaction config option

login
register
mail settings
Submitter Durham Goode
Date March 10, 2017, 11:58 p.m.
Message ID <34b22c430d074e57d529.1489190285@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/19090/
State Accepted
Headers show

Comments

Durham Goode - March 10, 2017, 11:58 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1489189949 28800
#      Fri Mar 10 15:52:29 2017 -0800
# Node ID 34b22c430d074e57d5293c75349d4651e80dfdce
# Parent  4046a8f85f7c6b025b986aad9104b73ba2680493
histedit: add histedit.singletransaction config option

This adds an option (which defaults to False) to run entire histedits in a
single transaction. This results in 20-25% faster histedits in large repos where
transaction startup cost is expensive.

I didn't want to enable this by default because it has some unfortunate side
effects. For instance, if a pretxncommit hook throws midway through the
histedit, it will rollback the entire histedit and lose any progress the user
had made. Same if the user aborts editting a commit message. It's still worth
turning this on for large repos, but probably not for normal sized repos.

Long term, once we have inmemory merging, we could do the entire histedit in
memory, without a transaction, then we could selectively rollback just parts of
it in the event of an exception.

Tested it by running the tests with
`--extra-config-opt=histedit.singletransaction=True`. The only failure was
related to the hook rollback issue I mention above.
Yuya Nishihara - March 20, 2017, 5:53 a.m.
On Fri, 10 Mar 2017 15:58:05 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1489189949 28800
> #      Fri Mar 10 15:52:29 2017 -0800
> # Node ID 34b22c430d074e57d5293c75349d4651e80dfdce
> # Parent  4046a8f85f7c6b025b986aad9104b73ba2680493
> histedit: add histedit.singletransaction config option

> +By default, histedit will close the transaction after each action. For
> +performance purposes, you can configure histedit to use a single transaction
> +across the entire histedit. WARNING: This setting introduces a significant risk
> +of losing the work you've done in a histedit if the histedit aborts
> +unexpectedly::

This seems scary enough, and the code looks good. Queued, thanks.

> +    try:
> +        # Don't use singletransaction by default since it rolls the entire
> +        # transaction back if an unexpected exception happens (like a
> +        # pretxncommit hook throws, or the user aborts the commit msg editor).
> +        if ui.configbool("histedit", "singletransaction", False):
> +            # Don't use a 'with' for the transaction, since actions may close
> +            # and reopen a transaction. For example, if the action executes an
> +            # external process it may choose to commit the transaction first.
> +            state.tr = repo.transaction('histedit')

I think not using 'with' is good, but have no idea how the transaction can be
reopen.
> +    except error.InterventionRequired:
> +        if state.tr is not None:
> +            state.tr.close()
> +        raise
> +    except Exception:
> +        if state.tr is not None:
> +            state.tr.abort()
> +        raise

Maybe this could be "finally: tr.release()", but I'm not sure which is more
correct.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -168,6 +168,15 @@  the drop to be implicit for missing comm
   [histedit]
   dropmissing = True
 
+By default, histedit will close the transaction after each action. For
+performance purposes, you can configure histedit to use a single transaction
+across the entire histedit. WARNING: This setting introduces a significant risk
+of losing the work you've done in a histedit if the histedit aborts
+unexpectedly::
+
+  [histedit]
+  singletransaction = True
+
 """
 
 from __future__ import absolute_import
@@ -269,6 +278,7 @@  class histeditstate(object):
         self.lock = lock
         self.wlock = wlock
         self.backupfile = None
+        self.tr = None
         if replacements is None:
             self.replacements = []
         else:
@@ -1105,18 +1115,45 @@  def _continuehistedit(ui, repo, state):
 
     total = len(state.actions)
     pos = 0
-    while state.actions:
-        state.write()
-        actobj = state.actions[0]
-        pos += 1
-        ui.progress(_("editing"), pos, actobj.torule(),
-                    _('changes'), total)
-        ui.debug('histedit: processing %s %s\n' % (actobj.verb,\
-                                                   actobj.torule()))
-        parentctx, replacement_ = actobj.run()
-        state.parentctxnode = parentctx.node()
-        state.replacements.extend(replacement_)
-        state.actions.pop(0)
+    state.tr = None
+
+    # Force an initial state file write, so the user can run --abort/continue
+    # even if there's an exception before the first transaction serialize.
+    state.write()
+    try:
+        # Don't use singletransaction by default since it rolls the entire
+        # transaction back if an unexpected exception happens (like a
+        # pretxncommit hook throws, or the user aborts the commit msg editor).
+        if ui.configbool("histedit", "singletransaction", False):
+            # Don't use a 'with' for the transaction, since actions may close
+            # and reopen a transaction. For example, if the action executes an
+            # external process it may choose to commit the transaction first.
+            state.tr = repo.transaction('histedit')
+
+        while state.actions:
+            state.write(tr=state.tr)
+            actobj = state.actions[0]
+            pos += 1
+            ui.progress(_("editing"), pos, actobj.torule(),
+                        _('changes'), total)
+            ui.debug('histedit: processing %s %s\n' % (actobj.verb,\
+                                                       actobj.torule()))
+            parentctx, replacement_ = actobj.run()
+            state.parentctxnode = parentctx.node()
+            state.replacements.extend(replacement_)
+            state.actions.pop(0)
+
+        if state.tr is not None:
+            state.tr.close()
+    except error.InterventionRequired:
+        if state.tr is not None:
+            state.tr.close()
+        raise
+    except Exception:
+        if state.tr is not None:
+            state.tr.abort()
+        raise
+
     state.write()
     ui.progress(_("editing"), None)