Patchwork scmutil: changed revrange code not to use append

login
register
mail settings
Submitter Lucas Moscovicz
Date Feb. 24, 2014, 10:37 p.m.
Message ID <be21c07d47d8b8ca1581.1393281478@dev1037.prn2.facebook.com>
Download mbox | patch
Permalink /patch/3751/
State Superseded
Commit 165b117ffc1e0597331b7b3f9a0b666c8749df38
Headers show

Comments

Lucas Moscovicz - Feb. 24, 2014, 10:37 p.m.
# HG changeset patch
# User Lucas Moscovicz <lmoscovicz@fb.com>
# Date 1393262852 28800
#      Mon Feb 24 09:27:32 2014 -0800
# Node ID be21c07d47d8b8ca158181b2ed491138780358d9
# Parent  dbb13949e05d8c576672ea0a49a978048c77ddf7
scmutil: changed revrange code not to use append

Removed one call to the append method
Pierre-Yves David - Feb. 24, 2014, 10:40 p.m.
On 02/24/2014 02:37 PM, Lucas Moscovicz wrote:
> # HG changeset patch
> # User Lucas Moscovicz <lmoscovicz@fb.com>
> # Date 1393262852 28800
> #      Mon Feb 24 09:27:32 2014 -0800
> # Node ID be21c07d47d8b8ca158181b2ed491138780358d9
> # Parent  dbb13949e05d8c576672ea0a49a978048c77ddf7
> scmutil: changed revrange code not to use append
>
> Removed one call to the append method
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -517,7 +517,7 @@
>                   if rev in seen:
>                       continue
>                   seen.add(rev)
> -                l.append(rev)
> +                l = l + [rev]

Highly concerned by the performance impact of this. Can you elaborate on 
what and why is going on here?
Sean Farley - Feb. 24, 2014, 10:57 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 02/24/2014 02:37 PM, Lucas Moscovicz wrote:
>> # HG changeset patch
>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>> # Date 1393262852 28800
>> #      Mon Feb 24 09:27:32 2014 -0800
>> # Node ID be21c07d47d8b8ca158181b2ed491138780358d9
>> # Parent  dbb13949e05d8c576672ea0a49a978048c77ddf7
>> scmutil: changed revrange code not to use append
>>
>> Removed one call to the append method
>>
>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>> --- a/mercurial/scmutil.py
>> +++ b/mercurial/scmutil.py
>> @@ -517,7 +517,7 @@
>>                   if rev in seen:
>>                       continue
>>                   seen.add(rev)
>> -                l.append(rev)
>> +                l = l + [rev]
>
> Highly concerned by the performance impact of this. Can you elaborate on 
> what and why is going on here?

Pierre-Yves: Matt asked Lucas to remove calls to append so that the new
lazy implementations didn't need to implement an append.

Lucas: I agree with Pierre-Yves and wonder if you couldn't change the
'l' variable to a set instead of a list?
Lucas Moscovicz - Feb. 24, 2014, 10:58 p.m.
On 2/24/14, 2:57 PM, "Sean Farley" <sean.michael.farley@gmail.com> wrote:

>
>Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 02/24/2014 02:37 PM, Lucas Moscovicz wrote:
>>> # HG changeset patch
>>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>>> # Date 1393262852 28800
>>> #      Mon Feb 24 09:27:32 2014 -0800
>>> # Node ID be21c07d47d8b8ca158181b2ed491138780358d9
>>> # Parent  dbb13949e05d8c576672ea0a49a978048c77ddf7
>>> scmutil: changed revrange code not to use append
>>>
>>> Removed one call to the append method
>>>
>>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>>> --- a/mercurial/scmutil.py
>>> +++ b/mercurial/scmutil.py
>>> @@ -517,7 +517,7 @@
>>>                   if rev in seen:
>>>                       continue
>>>                   seen.add(rev)
>>> -                l.append(rev)
>>> +                l = l + [rev]
>>
>> Highly concerned by the performance impact of this. Can you elaborate
>>on 
>> what and why is going on here?
>
>Pierre-Yves: Matt asked Lucas to remove calls to append so that the new
>lazy implementations didn't need to implement an append.

Yes, and also in later patches the implementation of the + operator is
going to be lazy as well so it shouldn¹t have any negative performance
impact.

>
>Lucas: I agree with Pierre-Yves and wonder if you couldn't change the
>'l' variable to a set instead of a list?
Lucas Moscovicz - Feb. 24, 2014, 11:04 p.m.
On 2/24/14, 2:57 PM, "Sean Farley" <sean.michael.farley@gmail.com> wrote:

>
>Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 02/24/2014 02:37 PM, Lucas Moscovicz wrote:
>>> # HG changeset patch
>>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>>> # Date 1393262852 28800
>>> #      Mon Feb 24 09:27:32 2014 -0800
>>> # Node ID be21c07d47d8b8ca158181b2ed491138780358d9
>>> # Parent  dbb13949e05d8c576672ea0a49a978048c77ddf7
>>> scmutil: changed revrange code not to use append
>>>
>>> Removed one call to the append method
>>>
>>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>>> --- a/mercurial/scmutil.py
>>> +++ b/mercurial/scmutil.py
>>> @@ -517,7 +517,7 @@
>>>                   if rev in seen:
>>>                       continue
>>>                   seen.add(rev)
>>> -                l.append(rev)
>>> +                l = l + [rev]
>>
>> Highly concerned by the performance impact of this. Can you elaborate
>>on 
>> what and why is going on here?
>
>Pierre-Yves: Matt asked Lucas to remove calls to append so that the new
>lazy implementations didn't need to implement an append.
>
>Lucas: I agree with Pierre-Yves and wonder if you couldn't change the
>'l' variable to a set instead of a list?

Sorry I didn¹t see this part of the question.
I expect the l variable to be a baseset typed class always so when I need
it to behave like a set I can just use the .set() method to get it.

Converting it into a set at this point would probably rearrange the order
of the revisions and in many cases lead to return the values in the wrong
order.

>

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -517,7 +517,7 @@ 
                 if rev in seen:
                     continue
                 seen.add(rev)
-                l.append(rev)
+                l = l + [rev]
                 continue
         except error.RepoLookupError:
             pass