Patchwork [5,of,5] dirstate.status: assign members one by one instead of unpacking the tuple

login
register
mail settings
Submitter Siddharth Agarwal
Date May 28, 2014, 5:05 a.m.
Message ID <124fd417846a9105b62b.1401253521@dev1738.prn1.facebook.com>
Download mbox | patch
Permalink /patch/4879/
State Superseded
Headers show

Comments

Siddharth Agarwal - May 28, 2014, 5:05 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1401249736 25200
#      Tue May 27 21:02:16 2014 -0700
# Node ID 124fd417846a9105b62b067c2d21990a2e44467e
# Parent  9ed8b0fa79cfa5f220930c92ac79b93f6d6bd8ee
dirstate.status: assign members one by one instead of unpacking the tuple

The CPython UNPACK_SEQUENCE opcode has fast paths when the value to be unpacked
is a tuple or a list, but falls back to creating a full-fledged iterator in
general. That is much slower than simply accessing and storing the tuple
members one by one.

With this patch, hg status and hg diff regain their previous speed.

The following tests are run against a working copy with over 270,000 files.
Here, 'before' means without this or the previous patch applied.

Note that in this case `hg perfstatus` isn't representative since it doesn't
take dirstate parsing time into account.

$ time hg status  # best of 5
before: 2.03s user 1.25s system 99% cpu 3.290 total
after:  2.01s user 1.25s system 99% cpu 3.261 total

$ time hg diff    # best of 5
before: 1.32s user 0.78s system 99% cpu 2.105 total
after:  1.27s user 0.79s system 99% cpu 2.066 total
Martin Geisler - May 28, 2014, 7:26 a.m.
Siddharth Agarwal <sid0@fb.com> writes:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1401249736 25200
> #      Tue May 27 21:02:16 2014 -0700
> # Node ID 124fd417846a9105b62b067c2d21990a2e44467e
> # Parent  9ed8b0fa79cfa5f220930c92ac79b93f6d6bd8ee
> dirstate.status: assign members one by one instead of unpacking the tuple
>
> The CPython UNPACK_SEQUENCE opcode has fast paths when the value to be unpacked
> is a tuple or a list, but falls back to creating a full-fledged iterator in
> general. That is much slower than simply accessing and storing the tuple
> members one by one.
>
> With this patch, hg status and hg diff regain their previous speed.
>
> The following tests are run against a working copy with over 270,000 files.
> Here, 'before' means without this or the previous patch applied.
>
> Note that in this case `hg perfstatus` isn't representative since it doesn't
> take dirstate parsing time into account.
>
> $ time hg status  # best of 5
> before: 2.03s user 1.25s system 99% cpu 3.290 total
> after:  2.01s user 1.25s system 99% cpu 3.261 total
>
> $ time hg diff    # best of 5
> before: 1.32s user 0.78s system 99% cpu 2.105 total
> after:  1.27s user 0.79s system 99% cpu 2.066 total
>
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -823,7 +823,11 @@
>                      uadd(fn)
>                  continue
>  

Perhaps it would make sense to add a comment here explaining why tuple
unpacking is bad in this case? Otherwise someone might revert the change
to avoid the "unnecessary" t variable.

> -            state, mode, size, time = dmap[fn]
> +            t = dmap[fn]
> +            state = t[0]
> +            mode = t[1]
> +            size = t[2]
> +            time = t[3]
>  
>              if not st and state in "nma":
>                  dadd(fn)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 28, 2014, 4:37 p.m.
On 05/28/2014 12:26 AM, Martin Geisler wrote:
> Siddharth Agarwal <sid0@fb.com> writes:
>
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1401249736 25200
>> #      Tue May 27 21:02:16 2014 -0700
>> # Node ID 124fd417846a9105b62b067c2d21990a2e44467e
>> # Parent  9ed8b0fa79cfa5f220930c92ac79b93f6d6bd8ee
>> dirstate.status: assign members one by one instead of unpacking the tuple
>>
>> The CPython UNPACK_SEQUENCE opcode has fast paths when the value to be unpacked
>> is a tuple or a list, but falls back to creating a full-fledged iterator in
>> general. That is much slower than simply accessing and storing the tuple
>> members one by one.
>>
>> With this patch, hg status and hg diff regain their previous speed.
>>
>> The following tests are run against a working copy with over 270,000 files.
>> Here, 'before' means without this or the previous patch applied.
>>
>> Note that in this case `hg perfstatus` isn't representative since it doesn't
>> take dirstate parsing time into account.
>>
>> $ time hg status  # best of 5
>> before: 2.03s user 1.25s system 99% cpu 3.290 total
>> after:  2.01s user 1.25s system 99% cpu 3.261 total
>>
>> $ time hg diff    # best of 5
>> before: 1.32s user 0.78s system 99% cpu 2.105 total
>> after:  1.27s user 0.79s system 99% cpu 2.066 total
>>
>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> --- a/mercurial/dirstate.py
>> +++ b/mercurial/dirstate.py
>> @@ -823,7 +823,11 @@
>>                       uadd(fn)
>>                   continue
>>
>
> Perhaps it would make sense to add a comment here explaining why tuple
> unpacking is bad in this case? Otherwise someone might revert the change
> to avoid the "unnecessary" t variable.

s/Perhaps it would make sense to/You must definitely/
Siddharth Agarwal - May 28, 2014, 5:02 p.m.
On 05/28/2014 09:37 AM, Pierre-Yves David wrote:
>
>
> On 05/28/2014 12:26 AM, Martin Geisler wrote:
>> Siddharth Agarwal <sid0@fb.com> writes:
>>
>>> # HG changeset patch
>>> # User Siddharth Agarwal <sid0@fb.com>
>>> # Date 1401249736 25200
>>> #      Tue May 27 21:02:16 2014 -0700
>>> # Node ID 124fd417846a9105b62b067c2d21990a2e44467e
>>> # Parent  9ed8b0fa79cfa5f220930c92ac79b93f6d6bd8ee
>>> dirstate.status: assign members one by one instead of unpacking the 
>>> tuple
>>>
>>> The CPython UNPACK_SEQUENCE opcode has fast paths when the value to 
>>> be unpacked
>>> is a tuple or a list, but falls back to creating a full-fledged 
>>> iterator in
>>> general. That is much slower than simply accessing and storing the 
>>> tuple
>>> members one by one.
>>>
>>> With this patch, hg status and hg diff regain their previous speed.
>>>
>>> The following tests are run against a working copy with over 270,000 
>>> files.
>>> Here, 'before' means without this or the previous patch applied.
>>>
>>> Note that in this case `hg perfstatus` isn't representative since it 
>>> doesn't
>>> take dirstate parsing time into account.
>>>
>>> $ time hg status  # best of 5
>>> before: 2.03s user 1.25s system 99% cpu 3.290 total
>>> after:  2.01s user 1.25s system 99% cpu 3.261 total
>>>
>>> $ time hg diff    # best of 5
>>> before: 1.32s user 0.78s system 99% cpu 2.105 total
>>> after:  1.27s user 0.79s system 99% cpu 2.066 total
>>>
>>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>>> --- a/mercurial/dirstate.py
>>> +++ b/mercurial/dirstate.py
>>> @@ -823,7 +823,11 @@
>>>                       uadd(fn)
>>>                   continue
>>>
>>
>> Perhaps it would make sense to add a comment here explaining why tuple
>> unpacking is bad in this case? Otherwise someone might revert the change
>> to avoid the "unnecessary" t variable.
>
> s/Perhaps it would make sense to/You must definitely/
>

Agreed. I'll wait for more feedback and then send a V2.

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -823,7 +823,11 @@ 
                     uadd(fn)
                 continue
 
-            state, mode, size, time = dmap[fn]
+            t = dmap[fn]
+            state = t[0]
+            mode = t[1]
+            size = t[2]
+            time = t[3]
 
             if not st and state in "nma":
                 dadd(fn)