Patchwork [5,of,5,STABLE,v2] revset: use _diffset in orderedlazyset.__sub__

login
register
mail settings
Submitter Gregory Szorc
Date Sept. 8, 2014, 9:59 p.m.
Message ID <d2c3dd725cf9b64b564f.1410213587@gps-mbp.local>
Download mbox | patch
Permalink /patch/5732/
State Rejected
Headers show

Comments

Gregory Szorc - Sept. 8, 2014, 9:59 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1410200370 25200
#      Mon Sep 08 11:19:30 2014 -0700
# Node ID d2c3dd725cf9b64b564f13096102f61f6b0d06ba
# Parent  b8226fb4800578b0d7b05f255f4c26140e458256
revset: use _diffset in orderedlazyset.__sub__

On a clone of the Firefox repository, the following revsets from the
revset benchmarks demonstrated a statistically significant change.

revset #25: roots((0::) - (0::tip))
wall 447.441994 comb 447.420000 user 447.240000 sys 0.180000 (best of 3)
wall  14.762341 comb  14.770000 user  14.730000 sys 0.040000 (best of 3)

This revset is very similar to one that runs as part of phase boundary
advancement during clone/pull. This change thus makes clones of large
repositories (like Firefox) much faster.

Interestingly, a related revset (roots((tip~100::) - (tip~100::tip)))
showed no statistically significant change.
Augie Fackler - Sept. 9, 2014, 4:36 p.m.
On Mon, Sep 08, 2014 at 02:59:47PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1410200370 25200
> #      Mon Sep 08 11:19:30 2014 -0700
> # Node ID d2c3dd725cf9b64b564f13096102f61f6b0d06ba
> # Parent  b8226fb4800578b0d7b05f255f4c26140e458256
> revset: use _diffset in orderedlazyset.__sub__

This series looks good to me, but did you intend this for stable?

(I'll leave these flagged, and will consider dropping them into default
- this seems like it might be too invasive for stable.)

>
> On a clone of the Firefox repository, the following revsets from the
> revset benchmarks demonstrated a statistically significant change.
>
> revset #25: roots((0::) - (0::tip))
> wall 447.441994 comb 447.420000 user 447.240000 sys 0.180000 (best of 3)
> wall  14.762341 comb  14.770000 user  14.730000 sys 0.040000 (best of 3)
>
> This revset is very similar to one that runs as part of phase boundary
> advancement during clone/pull. This change thus makes clones of large
> repositories (like Firefox) much faster.
>
> Interestingly, a related revset (roots((tip~100::) - (tip~100::tip)))
> showed no statistically significant change.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2414,10 +2414,14 @@ class orderedlazyset(_orderedsetmixin, l
>          return orderedlazyset(self, x.__contains__,
>                  ascending=self._ascending)
>
>      def __sub__(self, x):
> -        return orderedlazyset(self, lambda r: r not in x,
> -                ascending=self._ascending)
> +        kwargs = {}
> +        if self.isascending() and x.isascending():
> +            kwargs['ascending'] = True
> +        if self.isdescending() and x.isdescending():
> +            kwargs['ascending'] = False
> +        return _diffset(self, x, **kwargs)
>
>      def __add__(self, x):
>          kwargs = {}
>          if self.isascending() and x.isascending():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Gregory Szorc - Sept. 9, 2014, 4:47 p.m.
On 9/9/14 9:36 AM, Augie Fackler wrote:
> On Mon, Sep 08, 2014 at 02:59:47PM -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1410200370 25200
>> #      Mon Sep 08 11:19:30 2014 -0700
>> # Node ID d2c3dd725cf9b64b564f13096102f61f6b0d06ba
>> # Parent  b8226fb4800578b0d7b05f255f4c26140e458256
>> revset: use _diffset in orderedlazyset.__sub__
>
> This series looks good to me, but did you intend this for stable?
>
> (I'll leave these flagged, and will consider dropping them into default
> - this seems like it might be too invasive for stable.)

The clone time regression for the Firefox repo in 3.0 is a *major* 
regression. It appears Mercurial is hung for several minutes. I would 
like to see this fix rolled out ASAP so the window of impacted hg 
versions is as narrow as possible. If this is accepted in stable, I 
would even go so far as to ask for an early 3.0.2 release to further 
reduce the window by a few weeks.

I have the benchmarks running now, but I believe b8226fb48005 (the 
gnarliest patch in the series) isn't required to fix the regression and 
omitting it would introduce no revset benchmark regressions. Would that 
make stable inclusion easier?
Gregory Szorc - Sept. 9, 2014, 5:30 p.m.
On 9/9/14 9:47 AM, Gregory Szorc wrote:
> On 9/9/14 9:36 AM, Augie Fackler wrote:
>> On Mon, Sep 08, 2014 at 02:59:47PM -0700, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1410200370 25200
>>> #      Mon Sep 08 11:19:30 2014 -0700
>>> # Node ID d2c3dd725cf9b64b564f13096102f61f6b0d06ba
>>> # Parent  b8226fb4800578b0d7b05f255f4c26140e458256
>>> revset: use _diffset in orderedlazyset.__sub__
>>
>> This series looks good to me, but did you intend this for stable?
>>
>> (I'll leave these flagged, and will consider dropping them into default
>> - this seems like it might be too invasive for stable.)
>
> The clone time regression for the Firefox repo in 3.0 is a *major*
> regression. It appears Mercurial is hung for several minutes. I would
> like to see this fix rolled out ASAP so the window of impacted hg
> versions is as narrow as possible. If this is accepted in stable, I
> would even go so far as to ask for an early 3.0.2 release to further
> reduce the window by a few weeks.
>
> I have the benchmarks running now, but I believe b8226fb48005 (the
> gnarliest patch in the series) isn't required to fix the regression and
> omitting it would introduce no revset benchmark regressions. Would that
> make stable inclusion easier?

b8226fb48005 (manual iterator advancement) has a regression on revset 
#24. So I guess it would need to be considered for stable?

Result by revset
================

Revision: ['30088', '30102']
0) Revision: revset: more optimally handle __sub__ (issue4352)
1) Revision: revset: use _diffset in orderedlazyset.__sub__


revset #0: all()
0) wall 0.019646 comb 0.020000 user 0.020000 sys 0.000000 (best of 111)
1) wall 0.019443 comb 0.010000 user 0.010000 sys 0.000000 (best of 112)

revset #1: draft()
0) wall 0.139500 comb 0.140000 user 0.140000 sys 0.000000 (best of 64)
1) wall 0.138517 comb 0.140000 user 0.140000 sys 0.000000 (best of 65)

revset #2: ::tip
0) wall 1.075278 comb 1.070000 user 1.070000 sys 0.000000 (best of 9)
1) wall 1.077900 comb 1.080000 user 1.080000 sys 0.000000 (best of 9)

revset #3: draft() and ::tip
0) wall 0.350342 comb 0.350000 user 0.350000 sys 0.000000 (best of 26)
1) wall 0.349941 comb 0.350000 user 0.350000 sys 0.000000 (best of 26)

revset #4: ::tip and draft()
0) wall 0.353476 comb 0.360000 user 0.360000 sys 0.000000 (best of 26)
1) wall 0.348810 comb 0.350000 user 0.350000 sys 0.000000 (best of 26)

revset #5: 0::tip
0) wall 0.428219 comb 0.430000 user 0.430000 sys 0.000000 (best of 21)
1) wall 0.415304 comb 0.410000 user 0.410000 sys 0.000000 (best of 22)

revset #6: roots(0::tip)
0) wall 0.812258 comb 0.800000 user 0.790000 sys 0.010000 (best of 12)
1) wall 0.810720 comb 0.800000 user 0.790000 sys 0.010000 (best of 12)

revset #7: author(lmoscovicz)
0) wall 12.452664 comb 12.440000 user 12.360000 sys 0.080000 (best of 3)
1) wall 12.406843 comb 12.400000 user 12.330000 sys 0.070000 (best of 3)

revset #8: author(mpm)
0) wall 11.961316 comb 11.960000 user 11.890000 sys 0.070000 (best of 3)
1) wall 12.309132 comb 12.300000 user 12.220000 sys 0.080000 (best of 3)

revset #9: author(lmoscovicz) or author(mpm)
0) wall 31.568731 comb 31.560000 user 31.400000 sys 0.160000 (best of 3)
1) wall 30.947259 comb 30.940000 user 30.780000 sys 0.160000 (best of 3)

revset #10: author(mpm) or author(lmoscovicz)
0) wall 31.804026 comb 31.800000 user 31.630000 sys 0.170000 (best of 3)
1) wall 32.038068 comb 32.040000 user 31.870000 sys 0.170000 (best of 3)

revset #11: tip:0
0) wall 0.150554 comb 0.150000 user 0.150000 sys 0.000000 (best of 59)
1) wall 0.147571 comb 0.150000 user 0.150000 sys 0.000000 (best of 61)

revset #12: max(tip:0)
0) wall 0.029270 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)
1) wall 0.029079 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)

revset #13: min(0:tip)
0) wall 0.028872 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)
1) wall 0.028999 comb 0.020000 user 0.020000 sys 0.000000 (best of 100)

revset #14: 0::
0) wall 0.703585 comb 0.700000 user 0.690000 sys 0.010000 (best of 13)
1) wall 0.697673 comb 0.690000 user 0.680000 sys 0.010000 (best of 13)

revset #15: min(0::)
0) wall 0.000553 comb 0.000000 user 0.000000 sys 0.000000 (best of 3576)
1) wall 0.000553 comb 0.000000 user 0.000000 sys 0.000000 (best of 3571)

revset #16: roots((tip~100::) - (tip~100::tip))
0) wall 0.252185 comb 0.250000 user 0.250000 sys 0.000000 (best of 36)
1) wall 0.248275 comb 0.250000 user 0.250000 sys 0.000000 (best of 37)

revset #17: ::p1(p1(tip))::
0) wall 2.262527 comb 2.260000 user 2.250000 sys 0.010000 (best of 5)
1) wall 2.294079 comb 2.280000 user 2.270000 sys 0.010000 (best of 4)

revset #18: public()
0) wall 0.149696 comb 0.150000 user 0.150000 sys 0.000000 (best of 60)
1) wall 0.151007 comb 0.150000 user 0.150000 sys 0.000000 (best of 60)

revset #19: :10000 and public()
0) wall 0.037054 comb 0.040000 user 0.040000 sys 0.000000 (best of 100)
1) wall 0.037962 comb 0.040000 user 0.040000 sys 0.000000 (best of 100)

revset #20: draft()
0) wall 0.136598 comb 0.140000 user 0.140000 sys 0.000000 (best of 65)
1) wall 0.140949 comb 0.140000 user 0.140000 sys 0.000000 (best of 64)

revset #21: :10000 and draft()
0) wall 0.036853 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)
1) wall 0.037166 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)

revset #22: max(::(tip~20) - obsolete())
0) wall 1.078498 comb 1.080000 user 1.070000 sys 0.010000 (best of 9)
1) wall 1.070492 comb 1.080000 user 1.070000 sys 0.010000 (best of 9)

revset #23: roots((0:tip)::)
0) wall 1.760132 comb 1.760000 user 1.750000 sys 0.010000 (best of 6)
1) wall 1.796107 comb 1.790000 user 1.780000 sys 0.010000 (best of 6)

revset #24: (not public() - obsolete())
0) wall 0.378735 comb 0.380000 user 0.380000 sys 0.000000 (best of 24)
1) wall 0.485378 comb 0.490000 user 0.490000 sys 0.000000 (best of 19)

revset #25: roots((0::) - (0::tip))
0) wall 462.575024 comb 462.460000 user 462.150000 sys 0.310000 (best of 3)
1) wall 16.052030 comb 16.050000 user 16.000000 sys 0.050000 (best of 3)

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2414,10 +2414,14 @@  class orderedlazyset(_orderedsetmixin, l
         return orderedlazyset(self, x.__contains__,
                 ascending=self._ascending)
 
     def __sub__(self, x):
-        return orderedlazyset(self, lambda r: r not in x,
-                ascending=self._ascending)
+        kwargs = {}
+        if self.isascending() and x.isascending():
+            kwargs['ascending'] = True
+        if self.isdescending() and x.isdescending():
+            kwargs['ascending'] = False
+        return _diffset(self, x, **kwargs)
 
     def __add__(self, x):
         kwargs = {}
         if self.isascending() and x.isascending():