Patchwork [4,of,5] revset: prefetch an attribute in _generatorset.__iter__

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 19, 2014, 10:33 p.m.
Message ID <bfaab5271eb80fd28278.1411166037@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5888/
State Accepted
Commit 00c8abe64cf3dba4693db99f1c915f1692d5b95a
Headers show

Comments

Pierre-Yves David - Sept. 19, 2014, 10:33 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1411080765 25200
#      Thu Sep 18 15:52:45 2014 -0700
# Node ID bfaab5271eb80fd282784864193e0d57a1ceef89
# Parent  a2b305e889547516d7952ee8ecd67948cd8ff360
revset: prefetch an attribute in _generatorset.__iter__

Python's attribute lookup are expensible, lets do less of them.

This gives us a 7% speedup on this revset iteration (from 0.063403 to 0.059032)
Augie Fackler - Sept. 24, 2014, 3:28 p.m.
On Fri, Sep 19, 2014 at 03:33:57PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1411080765 25200
> #      Thu Sep 18 15:52:45 2014 -0700
> # Node ID bfaab5271eb80fd282784864193e0d57a1ceef89
> # Parent  a2b305e889547516d7952ee8ecd67948cd8ff360
> revset: prefetch an attribute in _generatorset.__iter__
>
> Python's attribute lookup are expensible, lets do less of them.
>
> This gives us a 7% speedup on this revset iteration (from 0.063403 to 0.059032)
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2655,16 +2655,17 @@ class _generatorset(object):
>          #
>          # Getting ride of it would provide an about 15% speed up on this
>          # iteration.
>          i = 0
>          genlist = self._genlist
> -        consume = self._consumegen()
> +        nextrev = self._consumegen().next
> +        _len = len # cache global lookup

Did this really make a difference? The caching of the __builtin__, I
mean (caching nextrev is obviously going to save some time.)

>          while True:
> -            if i < len(genlist):
> +            if i < _len(genlist):
>                  yield genlist[i]
>              else:
> -                yield consume.next()
> +                yield nextrev()
>              i += 1
>
>      def _consumegen(self):
>          cache = self._cache
>          genlist = self._genlist.append
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 24, 2014, 4:33 p.m.
On 09/24/2014 08:28 AM, Augie Fackler wrote:
> On Fri, Sep 19, 2014 at 03:33:57PM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1411080765 25200
>> #      Thu Sep 18 15:52:45 2014 -0700
>> # Node ID bfaab5271eb80fd282784864193e0d57a1ceef89
>> # Parent  a2b305e889547516d7952ee8ecd67948cd8ff360
>> revset: prefetch an attribute in _generatorset.__iter__
>>
>> Python's attribute lookup are expensible, lets do less of them.
>>
>> This gives us a 7% speedup on this revset iteration (from 0.063403 to 0.059032)
>>
>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>> --- a/mercurial/revset.py
>> +++ b/mercurial/revset.py
>> @@ -2655,16 +2655,17 @@ class _generatorset(object):
>>           #
>>           # Getting ride of it would provide an about 15% speed up on this
>>           # iteration.
>>           i = 0
>>           genlist = self._genlist
>> -        consume = self._consumegen()
>> +        nextrev = self._consumegen().next
>> +        _len = len # cache global lookup
>
> Did this really make a difference? The caching of the __builtin__, I
> mean (caching nextrev is obviously going to save some time.)

Yes, the global is catch using LOAD_GLOBAL (or something like that) that 
does a dictionnary lookup on all context abov this function. caching the 
builtin in a local free variable makes it as expensive as loading a 
local variable (lookup in an array)

I do not remember the interdependent speed boost but there was some.
Augie Fackler - Oct. 2, 2014, 7:50 p.m.
On Wed, Sep 24, 2014 at 12:33 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 09/24/2014 08:28 AM, Augie Fackler wrote:
>>
>> On Fri, Sep 19, 2014 at 03:33:57PM -0700, Pierre-Yves David wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1411080765 25200
>>> #      Thu Sep 18 15:52:45 2014 -0700
>>> # Node ID bfaab5271eb80fd282784864193e0d57a1ceef89
>>> # Parent  a2b305e889547516d7952ee8ecd67948cd8ff360
>>> revset: prefetch an attribute in _generatorset.__iter__
>>>
>>> Python's attribute lookup are expensible, lets do less of them.
>>>
>>> This gives us a 7% speedup on this revset iteration (from 0.063403 to
>>> 0.059032)
>>>
>>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>>> --- a/mercurial/revset.py
>>> +++ b/mercurial/revset.py
>>> @@ -2655,16 +2655,17 @@ class _generatorset(object):
>>>           #
>>>           # Getting ride of it would provide an about 15% speed up on
>>> this
>>>           # iteration.
>>>           i = 0
>>>           genlist = self._genlist
>>> -        consume = self._consumegen()
>>> +        nextrev = self._consumegen().next
>>> +        _len = len # cache global lookup
>>
>>
>> Did this really make a difference? The caching of the __builtin__, I
>> mean (caching nextrev is obviously going to save some time.)
>
>
> Yes, the global is catch using LOAD_GLOBAL (or something like that) that
> does a dictionnary lookup on all context abov this function. caching the
> builtin in a local free variable makes it as expensive as loading a local
> variable (lookup in an array)

Sigh.

Closing the loop on this thread: I'm ready to push these, but am
getting a rebased version from marmoute via irc and pulling from his
wip server.

>
> I do not remember the interdependent speed boost but there was some.
>
> --
> Pierre-Yves David

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2655,16 +2655,17 @@  class _generatorset(object):
         #
         # Getting ride of it would provide an about 15% speed up on this
         # iteration.
         i = 0
         genlist = self._genlist
-        consume = self._consumegen()
+        nextrev = self._consumegen().next
+        _len = len # cache global lookup
         while True:
-            if i < len(genlist):
+            if i < _len(genlist):
                 yield genlist[i]
             else:
-                yield consume.next()
+                yield nextrev()
             i += 1
 
     def _consumegen(self):
         cache = self._cache
         genlist = self._genlist.append