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
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 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?
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?
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