Patchwork [2,of,2,V2] bundle2: allow lazily acquiring the lock

login
register
mail settings
Submitter Durham Goode
Date Oct. 7, 2015, 1:42 a.m.
Message ID <1815d6ae1c20bd04668d.1444182140@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/10842/
State Superseded
Delegated to: Augie Fackler
Headers show

Comments

Durham Goode - Oct. 7, 2015, 1:42 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1444087194 25200
#      Mon Oct 05 16:19:54 2015 -0700
# Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
# Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
bundle2: allow lazily acquiring the lock

In the external pushrebase extension, it is valuable to be able to do some work
without taking the lock (like running expensive hooks). This enables
significantly higher commit throughput.

This patch adds an option to lazily acquire the lock. It means that all bundle2
part handlers that require writing to the repo must first call
op.gettransction(), when in this mode.
Augie Fackler - Oct. 7, 2015, 6:01 p.m.
On Tue, Oct 06, 2015 at 06:42:20PM -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1444087194 25200
> #      Mon Oct 05 16:19:54 2015 -0700
> # Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
> # Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
> bundle2: allow lazily acquiring the lock

is there any sensible way to test this?

>
> In the external pushrebase extension, it is valuable to be able to do some work
> without taking the lock (like running expensive hooks). This enables
> significantly higher commit throughput.
>
> This patch adds an option to lazily acquire the lock. It means that all bundle2
> part handlers that require writing to the repo must first call
> op.gettransction(), when in this mode.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1441,7 +1441,8 @@ def unbundle(repo, cg, heads, source, ur
>      If the push was raced as PushRaced exception is raised."""
>      r = 0
>      # need a transaction when processing a bundle2 stream
> -    wlock = lock = tr = None
> +    # [wlock, lock, tr] - needs to be an array so nested functions can modify it
> +    lockandtr = [None, None, None]
>      recordout = None
>      # quick fix for output mismatch with bundle2 in 3.4
>      captureoutput = repo.ui.configbool('experimental', 'bundle2-output-capture',
> @@ -1454,13 +1455,22 @@ def unbundle(repo, cg, heads, source, ur
>          if util.safehasattr(cg, 'params'):
>              r = None
>              try:
> -                wlock = repo.wlock()
> -                lock = repo.lock()
> -                tr = repo.transaction(source)
> -                tr.hookargs['source'] = source
> -                tr.hookargs['url'] = url
> -                tr.hookargs['bundle2'] = '1'
> -                op = bundle2.bundleoperation(repo, lambda: tr,
> +                def gettransaction():
> +                    if not lockandtr[2]:
> +                        lockandtr[0] = repo.wlock()
> +                        lockandtr[1] = repo.lock()
> +                        lockandtr[2] = repo.transaction(source)
> +                        lockandtr[2].hookargs['source'] = source
> +                        lockandtr[2].hookargs['url'] = url
> +                        lockandtr[2].hookargs['bundle2'] = '1'
> +                    return lockandtr[2]
> +
> +                # Do greedy locking by default until we're satisfied with lazy
> +                # locking.
> +                if not repo.ui.configbool('experimental', 'bundle2lazylocking'):
> +                    gettransaction()
> +
> +                op = bundle2.bundleoperation(repo, gettransaction,
>                                               captureoutput=captureoutput)
>                  try:
>                      op = bundle2.processbundle(repo, cg, op=op)
> @@ -1470,7 +1480,8 @@ def unbundle(repo, cg, heads, source, ur
>                          repo.ui.pushbuffer(error=True, subproc=True)
>                          def recordout(output):
>                              r.newpart('output', data=output, mandatory=False)
> -                tr.close()
> +                if lockandtr[2]:
> +                    lockandtr[2].close()
>              except BaseException as exc:
>                  exc.duringunbundle2 = True
>                  if captureoutput and r is not None:
> @@ -1481,10 +1492,10 @@ def unbundle(repo, cg, heads, source, ur
>                          parts.append(part)
>                  raise
>          else:
> -            lock = repo.lock()
> +            lockandtr[1] = repo.lock()
>              r = changegroup.addchangegroup(repo, cg, source, url)
>      finally:
> -        lockmod.release(tr, lock, wlock)
> +        lockmod.release(lockandtr[2], lockandtr[1], lockandtr[0])
>          if recordout is not None:
>              recordout(repo.ui.popbuffer())
>      return r
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Oct. 7, 2015, 6:23 p.m.
On 10/7/15 11:01 AM, Augie Fackler wrote:
> On Tue, Oct 06, 2015 at 06:42:20PM -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1444087194 25200
>> #      Mon Oct 05 16:19:54 2015 -0700
>> # Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
>> # Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
>> bundle2: allow lazily acquiring the lock
> is there any sensible way to test this?
>
With the pushrebase extension we have a hook that occurs before the lock 
is taken, so I have a test there.  I'm not sure how to do it in 
mercurial without some funky monkey patching in the test
Pierre-Yves David - Oct. 7, 2015, 7:04 p.m.
On 10/07/2015 11:23 AM, Durham Goode wrote:
>
>
> On 10/7/15 11:01 AM, Augie Fackler wrote:
>> On Tue, Oct 06, 2015 at 06:42:20PM -0700, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1444087194 25200
>>> #      Mon Oct 05 16:19:54 2015 -0700
>>> # Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
>>> # Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
>>> bundle2: allow lazily acquiring the lock
>> is there any sensible way to test this?
>>
> With the pushrebase extension we have a hook that occurs before the lock
> is taken, so I have a test there.  I'm not sure how to do it in
> mercurial without some funky monkey patching in the test

Just running a test with the experimental option on?
Augie Fackler - Oct. 7, 2015, 7:10 p.m.
On Wed, Oct 07, 2015 at 11:23:55AM -0700, Durham Goode wrote:
>
>
> On 10/7/15 11:01 AM, Augie Fackler wrote:
> >On Tue, Oct 06, 2015 at 06:42:20PM -0700, Durham Goode wrote:
> >># HG changeset patch
> >># User Durham Goode <durham@fb.com>
> >># Date 1444087194 25200
> >>#      Mon Oct 05 16:19:54 2015 -0700
> >># Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
> >># Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
> >>bundle2: allow lazily acquiring the lock
> >is there any sensible way to test this?
> >
> With the pushrebase extension we have a hook that occurs before the lock is
> taken, so I have a test there.  I'm not sure how to do it in mercurial
> without some funky monkey patching in the test

At a minimum, I'd like some sort of test that at least exercises the
code with this config knob enabled. Ideally we'll have some mechanism
that's easy to hook into (for things like pushrebase) which would also
make testing easier in the future.

Can I get a resend that has a trivial "things work" test, even if it
doesn't prove the lock is lazy?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 7, 2015, 7:20 p.m.
On 10/07/2015 12:10 PM, Augie Fackler wrote:
> On Wed, Oct 07, 2015 at 11:23:55AM -0700, Durham Goode wrote:
>>
>>
>> On 10/7/15 11:01 AM, Augie Fackler wrote:
>>> On Tue, Oct 06, 2015 at 06:42:20PM -0700, Durham Goode wrote:
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1444087194 25200
>>>> #      Mon Oct 05 16:19:54 2015 -0700
>>>> # Node ID 1815d6ae1c20bd04668d612f69a0d1946db0cf77
>>>> # Parent  1c8b56dd34406c2dfb2942d70ce427ef0d359f0c
>>>> bundle2: allow lazily acquiring the lock
>>> is there any sensible way to test this?
>>>
>> With the pushrebase extension we have a hook that occurs before the lock is
>> taken, so I have a test there.  I'm not sure how to do it in mercurial
>> without some funky monkey patching in the test
>
> At a minimum, I'd like some sort of test that at least exercises the
> code with this config knob enabled. Ideally we'll have some mechanism
> that's easy to hook into (for things like pushrebase) which would also
> make testing easier in the future.
>
> Can I get a resend that has a trivial "things work" test, even if it
> doesn't prove the lock is lazy?

Having a 5 lines extensions that adds a part at the begining of the 
bundle that check the lock stat should be simple.

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1441,7 +1441,8 @@  def unbundle(repo, cg, heads, source, ur
     If the push was raced as PushRaced exception is raised."""
     r = 0
     # need a transaction when processing a bundle2 stream
-    wlock = lock = tr = None
+    # [wlock, lock, tr] - needs to be an array so nested functions can modify it
+    lockandtr = [None, None, None]
     recordout = None
     # quick fix for output mismatch with bundle2 in 3.4
     captureoutput = repo.ui.configbool('experimental', 'bundle2-output-capture',
@@ -1454,13 +1455,22 @@  def unbundle(repo, cg, heads, source, ur
         if util.safehasattr(cg, 'params'):
             r = None
             try:
-                wlock = repo.wlock()
-                lock = repo.lock()
-                tr = repo.transaction(source)
-                tr.hookargs['source'] = source
-                tr.hookargs['url'] = url
-                tr.hookargs['bundle2'] = '1'
-                op = bundle2.bundleoperation(repo, lambda: tr,
+                def gettransaction():
+                    if not lockandtr[2]:
+                        lockandtr[0] = repo.wlock()
+                        lockandtr[1] = repo.lock()
+                        lockandtr[2] = repo.transaction(source)
+                        lockandtr[2].hookargs['source'] = source
+                        lockandtr[2].hookargs['url'] = url
+                        lockandtr[2].hookargs['bundle2'] = '1'
+                    return lockandtr[2]
+
+                # Do greedy locking by default until we're satisfied with lazy
+                # locking.
+                if not repo.ui.configbool('experimental', 'bundle2lazylocking'):
+                    gettransaction()
+
+                op = bundle2.bundleoperation(repo, gettransaction,
                                              captureoutput=captureoutput)
                 try:
                     op = bundle2.processbundle(repo, cg, op=op)
@@ -1470,7 +1480,8 @@  def unbundle(repo, cg, heads, source, ur
                         repo.ui.pushbuffer(error=True, subproc=True)
                         def recordout(output):
                             r.newpart('output', data=output, mandatory=False)
-                tr.close()
+                if lockandtr[2]:
+                    lockandtr[2].close()
             except BaseException as exc:
                 exc.duringunbundle2 = True
                 if captureoutput and r is not None:
@@ -1481,10 +1492,10 @@  def unbundle(repo, cg, heads, source, ur
                         parts.append(part)
                 raise
         else:
-            lock = repo.lock()
+            lockandtr[1] = repo.lock()
             r = changegroup.addchangegroup(repo, cg, source, url)
     finally:
-        lockmod.release(tr, lock, wlock)
+        lockmod.release(lockandtr[2], lockandtr[1], lockandtr[0])
         if recordout is not None:
             recordout(repo.ui.popbuffer())
     return r