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

login
register
mail settings
Submitter Durham Goode
Date March 9, 2017, 12:33 a.m.
Message ID <92225c3757e833d117c9.1489019620@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/19043/
State Superseded
Headers show

Comments

Durham Goode - March 9, 2017, 12:33 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1489019264 28800
#      Wed Mar 08 16:27:44 2017 -0800
# Node ID 92225c3757e833d117c91d063cb8d42f6f355eef
# Parent  265484e77411893d910f2eff4efe02cfe6abd5b7
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.
Durham Goode - March 9, 2017, 12:38 a.m.
On 3/8/17 4:33 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1489019264 28800
> #      Wed Mar 08 16:27:44 2017 -0800
> # Node ID 92225c3757e833d117c91d063cb8d42f6f355eef
> # Parent  265484e77411893d910f2eff4efe02cfe6abd5b7
> 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.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -168,6 +168,13 @@ the drop to be implicit for missing comm
>    [histedit]
>    dropmissing = True
>
> +By default, histedit will close the transaction after each action, so if there
> +are any errors the latest work is preserved. For performance purposes, you can
> +configure histedit to use a single transaction across the entire histedit::
> +
> +  [histedit]
> +  singletransaction = True
> +
>  """
>
>  from __future__ import absolute_import
> @@ -269,6 +276,7 @@ class histeditstate(object):
>          self.lock = lock
>          self.wlock = wlock
>          self.backupfile = None
> +        self.tr = None
>          if replacements is None:
>              self.replacements = []
>          else:

The bottom half of this patch is easier to read with diff -w below (but 
thunderbird tried to word wrap, so I've truncated some of the long lines 
for this paste):

@@ -1105,8 +1113,23 @@ def _continuehistedit(ui, repo, state):

      total = len(state.actions)
      pos = 0
+    state.tr = None
+
+    # Force an initial state file write, so the user can run
+    # even if there's an exception before the first transaction
+    state.write()
+    try:
+        # Don't use singletransaction by default since it rolls the
+        # transaction back if an unexpected exception happens (like a
+        # pretxncommit hook throws, or the user aborts the commit msg
+        if ui.configbool("histedit", "singletransaction", False):
+            # Don't use a 'with' for the transaction, since actions
+            # and reopen a transaction. For example, if the action + 
         # external process it may choose to commit the transaction
+            state.tr = repo.transaction('histedit')
+
      while state.actions:
-        state.write()
+            state.write(tr=state.tr)
          actobj = state.actions[0]
          pos += 1
          ui.progress(_("editing"), pos, actobj.torule(),
@@ -1117,6 +1140,18 @@ def _continuehistedit(ui, repo, state):
          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)
Augie Fackler - March 10, 2017, 2:27 a.m.
On Wed, Mar 08, 2017 at 04:33:40PM -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1489019264 28800
> #      Wed Mar 08 16:27:44 2017 -0800
> # Node ID 92225c3757e833d117c91d063cb8d42f6f355eef
> # Parent  265484e77411893d910f2eff4efe02cfe6abd5b7
> 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.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -168,6 +168,13 @@ the drop to be implicit for missing comm
>    [histedit]
>    dropmissing = True
>
> +By default, histedit will close the transaction after each action, so if there
> +are any errors the latest work is preserved. For performance purposes, you can
> +configure histedit to use a single transaction across the entire histedit::

I feel like this isn't up-front enough about the risks around hooks
causing you to lose your entire histedit. Can we strengthen this
warning somehow? (Or am I being too worried?)

> +
> +  [histedit]
> +  singletransaction = True
> +
>  """
>
>  from __future__ import absolute_import
> @@ -269,6 +276,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 +1113,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)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - March 10, 2017, 11:09 p.m.
On 3/9/17 6:27 PM, Augie Fackler wrote:
> On Wed, Mar 08, 2017 at 04:33:40PM -0800, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1489019264 28800
>> #      Wed Mar 08 16:27:44 2017 -0800
>> # Node ID 92225c3757e833d117c91d063cb8d42f6f355eef
>> # Parent  265484e77411893d910f2eff4efe02cfe6abd5b7
>> 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.
>>
>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>> --- a/hgext/histedit.py
>> +++ b/hgext/histedit.py
>> @@ -168,6 +168,13 @@ the drop to be implicit for missing comm
>>    [histedit]
>>    dropmissing = True
>>
>> +By default, histedit will close the transaction after each action, so if there
>> +are any errors the latest work is preserved. For performance purposes, you can
>> +configure histedit to use a single transaction across the entire histedit::
>
> I feel like this isn't up-front enough about the risks around hooks
> causing you to lose your entire histedit. Can we strengthen this
> warning somehow? (Or am I being too worried?)

I can make this scarier and will resend.

>
>> +
>> +  [histedit]
>> +  singletransaction = True
>> +
>>  """
>>
>>  from __future__ import absolute_import
>> @@ -269,6 +276,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 +1113,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)
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Kw35ACwQdtSmYBSygm5sAEbpqm7vAqZmKufRgYrnhx4&s=ioG-vJXRj143_KFzwwzhsEuXZj189uFsznV_3UPnRqY&e=

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -168,6 +168,13 @@  the drop to be implicit for missing comm
   [histedit]
   dropmissing = True
 
+By default, histedit will close the transaction after each action, so if there
+are any errors the latest work is preserved. For performance purposes, you can
+configure histedit to use a single transaction across the entire histedit::
+
+  [histedit]
+  singletransaction = True
+
 """
 
 from __future__ import absolute_import
@@ -269,6 +276,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 +1113,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)