Patchwork [3,of,3,diffset-extra] revset: use _diffset in spanset.__sub__

login
register
mail settings
Submitter Gregory Szorc
Date Sept. 8, 2014, 10 p.m.
Message ID <b12f25dad733d8842472.1410213652@gps-mbp.local>
Download mbox | patch
Permalink /patch/5735/
State Deferred
Headers show

Comments

Gregory Szorc - Sept. 8, 2014, 10 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1410200439 25200
#      Mon Sep 08 11:20:39 2014 -0700
# Node ID b12f25dad733d8842472bfb64f8fa80347cad2f0
# Parent  3a75a1f6937b15a6edc5e7a5c838c8d4cdf9dc73
revset: use _diffset in spanset.__sub__

This patch results in the following statistically significant changes in
revset benchmarks for a clone of the Firefox repository.

revset #8: author(mpm)
wall 11.409557 comb 11.410000 user 11.360000 sys 0.050000 (best of 3)
wall 12.045913 comb 12.040000 user 12.000000 sys 0.040000 (best of 3)

revset #9: author(lmoscovicz) or author(mpm)
wall 30.106085 comb 30.100000 user 29.980000 sys 0.120000 (best of 3)
wall 36.646229 comb 36.640000 user 36.490000 sys 0.150000 (best of 3)

revset #10: author(mpm) or author(lmoscovicz)
wall 29.797047 comb 29.800000 user 29.690000 sys 0.110000 (best of 3)
wall 35.162670 comb 35.170000 user 35.010000 sys 0.160000 (best of 3)

revset #22: max(::(tip~20) - obsolete())
wall 0.966111 comb 0.970000 user 0.960000 sys 0.010000 (best of 10)
wall 1.167777 comb 1.170000 user 1.160000 sys 0.010000 (best of 8)

revset #24: (not public() - obsolete())
wall 0.365095 comb 0.360000 user 0.360000 sys 0.000000 (best of 25)
wall 0.219435 comb 0.220000 user 0.220000 sys 0.000000 (best of 42)

Since all but one of these is a regression, there are merits for
rejecting this patch.
Gregory Szorc - Sept. 8, 2014, 10:06 p.m.
I isolated the performance regression down to this usage of _diffset. I 
suspect, the baseset optimization is likely pulling its weight here. We 
should probably move that into _diffset.

Also, this patch series is really for observation only. The patches have 
no obvious positive changes (aside from revset #24 speed-up in this 
patch). I'm not sure if this is due to lack of test coverage or just the 
patches not being appropriate.

Given how often I seem to be stumbling across slowdowns in revset 
performance, I'm inclined to believe the revsetbenchmarks.txt coverage 
is at least partially lacking. I encourage others to add revset "death 
tests" to improve matters.

On 9/8/14 3:00 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1410200439 25200
> #      Mon Sep 08 11:20:39 2014 -0700
> # Node ID b12f25dad733d8842472bfb64f8fa80347cad2f0
> # Parent  3a75a1f6937b15a6edc5e7a5c838c8d4cdf9dc73
> revset: use _diffset in spanset.__sub__
>
> This patch results in the following statistically significant changes in
> revset benchmarks for a clone of the Firefox repository.
>
> revset #8: author(mpm)
> wall 11.409557 comb 11.410000 user 11.360000 sys 0.050000 (best of 3)
> wall 12.045913 comb 12.040000 user 12.000000 sys 0.040000 (best of 3)
>
> revset #9: author(lmoscovicz) or author(mpm)
> wall 30.106085 comb 30.100000 user 29.980000 sys 0.120000 (best of 3)
> wall 36.646229 comb 36.640000 user 36.490000 sys 0.150000 (best of 3)
>
> revset #10: author(mpm) or author(lmoscovicz)
> wall 29.797047 comb 29.800000 user 29.690000 sys 0.110000 (best of 3)
> wall 35.162670 comb 35.170000 user 35.010000 sys 0.160000 (best of 3)
>
> revset #22: max(::(tip~20) - obsolete())
> wall 0.966111 comb 0.970000 user 0.960000 sys 0.010000 (best of 10)
> wall 1.167777 comb 1.170000 user 1.160000 sys 0.010000 (best of 8)
>
> revset #24: (not public() - obsolete())
> wall 0.365095 comb 0.360000 user 0.360000 sys 0.000000 (best of 25)
> wall 0.219435 comb 0.220000 user 0.220000 sys 0.000000 (best of 42)
>
> Since all but one of these is a regression, there are merits for
> rejecting this patch.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2873,14 +2873,14 @@ class spanset(_orderedsetmixin):
>           else:
>               return orderedlazyset(self, x.__contains__, ascending=False)
>
>       def __sub__(self, x):
> -        if isinstance(x, baseset):
> -            x = x.set()
> -        if self._start <= self._end:
> -            return orderedlazyset(self, lambda r: r not in x)
> -        else:
> -            return orderedlazyset(self, lambda r: r not in x, ascending=False)
> +        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():
>

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2873,14 +2873,14 @@  class spanset(_orderedsetmixin):
         else:
             return orderedlazyset(self, x.__contains__, ascending=False)
 
     def __sub__(self, x):
-        if isinstance(x, baseset):
-            x = x.set()
-        if self._start <= self._end:
-            return orderedlazyset(self, lambda r: r not in x)
-        else:
-            return orderedlazyset(self, lambda r: r not in x, ascending=False)
+        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():