| 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
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)
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.
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
> 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)