Patchwork [V2] localrepo: avoid unnecessary conversion from node to rev

login
register
mail settings
Submitter Stanislau Hlebik
Date Feb. 7, 2017, 8:52 a.m.
Message ID <8614d5d15d6fe47c3bcf.1486457533@devvm1840.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18338/
State Rejected
Headers show

Comments

Stanislau Hlebik - Feb. 7, 2017, 8:52 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1486457478 28800
#      Tue Feb 07 00:51:18 2017 -0800
# Node ID 8614d5d15d6fe47c3bcfdf79823f68f18c7741ae
# Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
localrepo: avoid unnecessary conversion from node to rev

changelog.heads() first calls headrevs then converts them to nodes.
localrepo.heads() then sorts them using self.changelog.rev function and makes
useless conversion back to revs. Instead let's call changelog.headrevs() from
localrepo.heads(), sort the output and then convert to nodes. Because headrevs
does not support start parameter this optimization only works if start is None.
Jun Wu - Feb. 7, 2017, 9:11 p.m.
V1 was accepted and in hg-committed [1] already. I don't know the official
recommended workflows but I used to just push follow-up fixes.

In general, it's a good practice to use hg-committed instead of the hg repo
for development, to avoid potential conflicts with other changes.

[1]: https://www.mercurial-scm.org/repo/hg-committed/rev/1791be8a95c5

Excerpts from Stanislau Hlebik's message of 2017-02-07 00:52:13 -0800:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1486457478 28800
> #      Tue Feb 07 00:51:18 2017 -0800
> # Node ID 8614d5d15d6fe47c3bcfdf79823f68f18c7741ae
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> localrepo: avoid unnecessary conversion from node to rev
> 
> changelog.heads() first calls headrevs then converts them to nodes.
> localrepo.heads() then sorts them using self.changelog.rev function and makes
> useless conversion back to revs. Instead let's call changelog.headrevs() from
> localrepo.heads(), sort the output and then convert to nodes. Because headrevs
> does not support start parameter this optimization only works if start is None.
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1852,6 +1852,10 @@
>                                    listsubrepos)
>  
>      def heads(self, start=None):
> +        if start is None:
> +            headrevs = reversed(self.changelog.headrevs())
> +            return [self.changelog.node(rev) for rev in headrevs]
> +
>          heads = self.changelog.heads(start)
>          # sort the output in rev descending order
>          return sorted(heads, key=self.changelog.rev, reverse=True)
Yuya Nishihara - Feb. 10, 2017, 1:48 p.m.
On Tue, 7 Feb 2017 13:11:23 -0800, Jun Wu wrote:
> V1 was accepted and in hg-committed [1] already. I don't know the official
> recommended workflows but I used to just push follow-up fixes.

It's at the bottom of the draft stack, so I think follow-ups would be
preferred.

> Excerpts from Stanislau Hlebik's message of 2017-02-07 00:52:13 -0800:
> > # HG changeset patch
> > # User Stanislau Hlebik <stash@fb.com>
> > # Date 1486457478 28800
> > #      Tue Feb 07 00:51:18 2017 -0800
> > # Node ID 8614d5d15d6fe47c3bcfdf79823f68f18c7741ae
> > # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> > localrepo: avoid unnecessary conversion from node to rev
> > 
> > changelog.heads() first calls headrevs then converts them to nodes.
> > localrepo.heads() then sorts them using self.changelog.rev function and makes
> > useless conversion back to revs. Instead let's call changelog.headrevs() from
> > localrepo.heads(), sort the output and then convert to nodes. Because headrevs
> > does not support start parameter this optimization only works if start is None.
> > 
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -1852,6 +1852,10 @@
> >                                    listsubrepos)
> >  
> >      def heads(self, start=None):
> > +        if start is None:
> > +            headrevs = reversed(self.changelog.headrevs())
> > +            return [self.changelog.node(rev) for rev in headrevs]

Also, can you check if self.changelog has no visible cost? I'm a bit afraid that
this patch could actually made heads() slow because of repeated self.changelog
calls.
Augie Fackler - Feb. 10, 2017, 9:28 p.m.
On Fri, Feb 10, 2017 at 10:48:10PM +0900, Yuya Nishihara wrote:
> On Tue, 7 Feb 2017 13:11:23 -0800, Jun Wu wrote:
> > V1 was accepted and in hg-committed [1] already. I don't know the official
> > recommended workflows but I used to just push follow-up fixes.
>
> It's at the bottom of the draft stack, so I think follow-ups would be
> preferred.

+1, I'd rather not shake up the entire stack at this point.

>
> > Excerpts from Stanislau Hlebik's message of 2017-02-07 00:52:13 -0800:
> > > # HG changeset patch
> > > # User Stanislau Hlebik <stash@fb.com>
> > > # Date 1486457478 28800
> > > #      Tue Feb 07 00:51:18 2017 -0800
> > > # Node ID 8614d5d15d6fe47c3bcfdf79823f68f18c7741ae
> > > # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> > > localrepo: avoid unnecessary conversion from node to rev
> > >
> > > changelog.heads() first calls headrevs then converts them to nodes.
> > > localrepo.heads() then sorts them using self.changelog.rev function and makes
> > > useless conversion back to revs. Instead let's call changelog.headrevs() from
> > > localrepo.heads(), sort the output and then convert to nodes. Because headrevs
> > > does not support start parameter this optimization only works if start is None.
> > >
> > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > --- a/mercurial/localrepo.py
> > > +++ b/mercurial/localrepo.py
> > > @@ -1852,6 +1852,10 @@
> > >                                    listsubrepos)
> > >
> > >      def heads(self, start=None):
> > > +        if start is None:
> > > +            headrevs = reversed(self.changelog.headrevs())
> > > +            return [self.changelog.node(rev) for rev in headrevs]
>
> Also, can you check if self.changelog has no visible cost? I'm a bit afraid that
> this patch could actually made heads() slow because of repeated self.changelog
> calls.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - Feb. 11, 2017, 6:36 p.m.
> On Feb 10, 2017, at 05:48, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Tue, 7 Feb 2017 13:11:23 -0800, Jun Wu wrote:
>> V1 was accepted and in hg-committed [1] already. I don't know the official
>> recommended workflows but I used to just push follow-up fixes.
> 
> It's at the bottom of the draft stack, so I think follow-ups would be
> preferred.
> 
>> Excerpts from Stanislau Hlebik's message of 2017-02-07 00:52:13 -0800:
>>> # HG changeset patch
>>> # User Stanislau Hlebik <stash@fb.com>
>>> # Date 1486457478 28800
>>> #      Tue Feb 07 00:51:18 2017 -0800
>>> # Node ID 8614d5d15d6fe47c3bcfdf79823f68f18c7741ae
>>> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
>>> localrepo: avoid unnecessary conversion from node to rev
>>> 
>>> changelog.heads() first calls headrevs then converts them to nodes.
>>> localrepo.heads() then sorts them using self.changelog.rev function and makes
>>> useless conversion back to revs. Instead let's call changelog.headrevs() from
>>> localrepo.heads(), sort the output and then convert to nodes. Because headrevs
>>> does not support start parameter this optimization only works if start is None.
>>> 
>>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>> --- a/mercurial/localrepo.py
>>> +++ b/mercurial/localrepo.py
>>> @@ -1852,6 +1852,10 @@
>>>                                   listsubrepos)
>>> 
>>>     def heads(self, start=None):
>>> +        if start is None:
>>> +            headrevs = reversed(self.changelog.headrevs())
>>> +            return [self.changelog.node(rev) for rev in headrevs]
> 
> Also, can you check if self.changelog has no visible cost? I'm a bit afraid that
> this patch could actually made heads() slow because of repeated self.changelog
> calls.

Repeated lookups of self.changelog will incur noticeable overhead in loops like this. Please follow up with a change to cache the changelog in a local variable.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1852,6 +1852,10 @@ 
                                   listsubrepos)
 
     def heads(self, start=None):
+        if start is None:
+            headrevs = reversed(self.changelog.headrevs())
+            return [self.changelog.node(rev) for rev in headrevs]
+
         heads = self.changelog.heads(start)
         # sort the output in rev descending order
         return sorted(heads, key=self.changelog.rev, reverse=True)