Patchwork [04,of,22] obsstore: minor optimization for the obsolete revset

login
register
mail settings
Submitter Jun Wu
Date June 4, 2017, 11:59 p.m.
Message ID <e1d6126ed614db6ec664.1496620756@x1c>
Download mbox | patch
Permalink /patch/21181/
State Accepted
Headers show

Comments

Jun Wu - June 4, 2017, 11:59 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1496457147 25200
#      Fri Jun 02 19:32:27 2017 -0700
# Node ID e1d6126ed614db6ec664c79afcb3a141c9a0dfa4
# Parent  d492628229c58f8417a8b5925a614e26a16465af
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r e1d6126ed614
obsstore: minor optimization for the obsolete revset

Use local variables in a loop.
Gregory Szorc - June 6, 2017, 6:05 a.m.
On Sun, Jun 4, 2017 at 4:59 PM, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1496457147 25200
> #      Fri Jun 02 19:32:27 2017 -0700
> # Node ID e1d6126ed614db6ec664c79afcb3a141c9a0dfa4
> # Parent  d492628229c58f8417a8b5925a614e26a16465af
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> e1d6126ed614
> obsstore: minor optimization for the obsolete revset
>

Queued 4-7.

I wish this one had perf numbers in the commit message. But the patch looks
correct and avoiding attribute lookups in loops like this is a good
practice.


>
> Use local variables in a loop.
>
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -1278,10 +1278,8 @@ def clearobscaches(repo):
>  def _computeobsoleteset(repo):
>      """the set of obsolete revisions"""
> -    obs = set()
>      getnode = repo.changelog.node
>      notpublic = repo._phasecache.getrevset(repo, (phases.draft,
> phases.secret))
> -    for r in notpublic:
> -        if getnode(r) in repo.obsstore.successors:
> -            obs.add(r)
> +    isobs = repo.obsstore.successors.__contains__
> +    obs = set(r for r in notpublic if isobs(getnode(r)))
>      return obs
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - June 6, 2017, 8:10 p.m.
On Mon, Jun 5, 2017 at 11:05 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> On Sun, Jun 4, 2017 at 4:59 PM, Jun Wu <quark@fb.com> wrote:
>>
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1496457147 25200
>> #      Fri Jun 02 19:32:27 2017 -0700
>> # Node ID e1d6126ed614db6ec664c79afcb3a141c9a0dfa4
>> # Parent  d492628229c58f8417a8b5925a614e26a16465af
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
>> e1d6126ed614
>> obsstore: minor optimization for the obsolete revset
>
>
> Queued 4-7.
>
> I wish this one had perf numbers in the commit message. But the patch looks
> correct and avoiding attribute lookups in loops like this is a good
> practice.
>
>>
>>
>> Use local variables in a loop.
>>
>> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
>> --- a/mercurial/obsolete.py
>> +++ b/mercurial/obsolete.py
>> @@ -1278,10 +1278,8 @@ def clearobscaches(repo):
>>  def _computeobsoleteset(repo):
>>      """the set of obsolete revisions"""
>> -    obs = set()
>>      getnode = repo.changelog.node
>>      notpublic = repo._phasecache.getrevset(repo, (phases.draft,
>> phases.secret))
>> -    for r in notpublic:
>> -        if getnode(r) in repo.obsstore.successors:
>> -            obs.add(r)
>> +    isobs = repo.obsstore.successors.__contains__
>> +    obs = set(r for r in notpublic if isobs(getnode(r)))
>>      return obs

I'll inline "obs" in flight.

>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - June 6, 2017, 8:30 p.m.
On Tue, Jun 6, 2017 at 1:10 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Mon, Jun 5, 2017 at 11:05 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>> On Sun, Jun 4, 2017 at 4:59 PM, Jun Wu <quark@fb.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1496457147 25200
>>> #      Fri Jun 02 19:32:27 2017 -0700
>>> # Node ID e1d6126ed614db6ec664c79afcb3a141c9a0dfa4
>>> # Parent  d492628229c58f8417a8b5925a614e26a16465af
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
>>> e1d6126ed614
>>> obsstore: minor optimization for the obsolete revset
>>
>>
>> Queued 4-7.
>>
>> I wish this one had perf numbers in the commit message. But the patch looks
>> correct and avoiding attribute lookups in loops like this is a good
>> practice.
>>
>>>
>>>
>>> Use local variables in a loop.
>>>
>>> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
>>> --- a/mercurial/obsolete.py
>>> +++ b/mercurial/obsolete.py
>>> @@ -1278,10 +1278,8 @@ def clearobscaches(repo):
>>>  def _computeobsoleteset(repo):
>>>      """the set of obsolete revisions"""
>>> -    obs = set()
>>>      getnode = repo.changelog.node
>>>      notpublic = repo._phasecache.getrevset(repo, (phases.draft,
>>> phases.secret))
>>> -    for r in notpublic:
>>> -        if getnode(r) in repo.obsstore.successors:
>>> -            obs.add(r)
>>> +    isobs = repo.obsstore.successors.__contains__
>>> +    obs = set(r for r in notpublic if isobs(getnode(r)))
>>>      return obs
>
> I'll inline "obs" in flight.

Apparently not. It became public while I was reviewing.

>
>>>
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>>
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
Pierre-Yves David - June 7, 2017, 1:50 p.m.
On 06/06/2017 07:05 AM, Gregory Szorc wrote:
> On Sun, Jun 4, 2017 at 4:59 PM, Jun Wu <quark@fb.com> wrote:
>
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1496457147 25200
>> #      Fri Jun 02 19:32:27 2017 -0700
>> # Node ID e1d6126ed614db6ec664c79afcb3a141c9a0dfa4
>> # Parent  d492628229c58f8417a8b5925a614e26a16465af
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
>> e1d6126ed614
>> obsstore: minor optimization for the obsolete revset
>>
>
> Queued 4-7.
>
> I wish this one had perf numbers in the commit message. But the patch looks
> correct and avoiding attribute lookups in loops like this is a good
> practice.

I can't find the changeset again, but I had a local version of that with 
timing data. I think it moved the loop from 45ms to 6ms or something 
similarly ridiculous improvement that python can offer.

(and yes +1 for patch 4-7)

Cheers,

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -1278,10 +1278,8 @@  def clearobscaches(repo):
 def _computeobsoleteset(repo):
     """the set of obsolete revisions"""
-    obs = set()
     getnode = repo.changelog.node
     notpublic = repo._phasecache.getrevset(repo, (phases.draft, phases.secret))
-    for r in notpublic:
-        if getnode(r) in repo.obsstore.successors:
-            obs.add(r)
+    isobs = repo.obsstore.successors.__contains__
+    obs = set(r for r in notpublic if isobs(getnode(r)))
     return obs