Patchwork revset: use a baseset in _notpublic()

login
register
mail settings
Submitter Pierre-Yves David
Date June 18, 2015, 5:04 p.m.
Message ID <32d7dee9420183af58bd.1434647095@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/9704/
State Accepted
Commit 833fa28cd9494e966ee0299d3685e6080812b5d1
Headers show

Comments

Pierre-Yves David - June 18, 2015, 5:04 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1433991507 25200
#      Wed Jun 10 19:58:27 2015 -0700
# Node ID 32d7dee9420183af58bd5ce150908425e708de77
# Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
revset: use a baseset in _notpublic()

The '_notpublic()' internal revset was "returning" a set. That was wrong. We now
return a 'baseset' as appropriate. This has no effect on performance in most case,
because we do the exact same operation than what the combination with a
'fullreposet' was doing. This as a small effect on some operation when combined
with other set, because we now apply the filtering in all cases. I think the
correctness is worth the impact on some corner cases. The optimizer should take
care of these corner cases anyway.

revset #0: not public()
   plain         min           max           first         last          reverse
0) 0.000465      0.000491      0.000495      0.000500      0.000494      0.000479
1) 0.000484      0.000503      0.000498      0.000505      0.000504      0.000491

revset #1: (tip~1000::) - public()
   plain         min           max           first         last          reverse
0) 0.002765      0.001742      0.002767      0.001730      0.002761      0.002782
1) 0.002847      0.001777      0.002776      0.001741      0.002764      0.002858

revset #2: not public() and branch("default")
   plain         min           max           first         last          reverse
0) 0.012104      0.011138      0.011189      0.011138      0.011166      0.011578
1) 0.011387  94% 0.011738 105% 0.014220 127% 0.011223      0.011184      0.012077

revset #3: (not public() - obsolete())
   plain         min           max           first         last          reverse
0) 0.000583      0.000556      0.000552      0.000555      0.000552      0.000610
1) 0.000613 105% 0.000559      0.000557      0.000573      0.000558      0.000613

revset #4: head() - public()
   plain         min           max           first         last          reverse
0) 0.010869      0.010800      0.011547      0.010843      0.010891      0.010891
1) 0.011031      0.011497 106% 0.011087      0.011100      0.011100      0.011085
Augie Fackler - June 22, 2015, 2:12 p.m.
On Thu, Jun 18, 2015 at 10:04:55AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1433991507 25200
> #      Wed Jun 10 19:58:27 2015 -0700
> # Node ID 32d7dee9420183af58bd5ce150908425e708de77
> # Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
> revset: use a baseset in _notpublic()

Queued, thanks.

>
> The '_notpublic()' internal revset was "returning" a set. That was wrong. We now
> return a 'baseset' as appropriate. This has no effect on performance in most case,
> because we do the exact same operation than what the combination with a
> 'fullreposet' was doing. This as a small effect on some operation when combined
> with other set, because we now apply the filtering in all cases. I think the
> correctness is worth the impact on some corner cases. The optimizer should take
> care of these corner cases anyway.
>
> revset #0: not public()
>    plain         min           max           first         last          reverse
> 0) 0.000465      0.000491      0.000495      0.000500      0.000494      0.000479
> 1) 0.000484      0.000503      0.000498      0.000505      0.000504      0.000491
>
> revset #1: (tip~1000::) - public()
>    plain         min           max           first         last          reverse
> 0) 0.002765      0.001742      0.002767      0.001730      0.002761      0.002782
> 1) 0.002847      0.001777      0.002776      0.001741      0.002764      0.002858
>
> revset #2: not public() and branch("default")
>    plain         min           max           first         last          reverse
> 0) 0.012104      0.011138      0.011189      0.011138      0.011166      0.011578
> 1) 0.011387  94% 0.011738 105% 0.014220 127% 0.011223      0.011184      0.012077
>
> revset #3: (not public() - obsolete())
>    plain         min           max           first         last          reverse
> 0) 0.000583      0.000556      0.000552      0.000555      0.000552      0.000610
> 1) 0.000613 105% 0.000559      0.000557      0.000573      0.000558      0.000613
>
> revset #4: head() - public()
>    plain         min           max           first         last          reverse
> 0) 0.010869      0.010800      0.011547      0.010843      0.010891      0.010891
> 1) 0.011031      0.011497 106% 0.011087      0.011100      0.011100      0.011085
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -1536,12 +1536,12 @@ def _notpublic(repo, subset, x):
>      repo._phasecache.loadphaserevs(repo) # ensure phase's sets are loaded
>      if repo._phasecache._phasesets:
>          s = set()
>          for u in repo._phasecache._phasesets[1:]:
>              s.update(u)
> -        # XXX we should turn this into a baseset instead of a set, smartset may
> -        # do some optimisations from the fact this is a baseset.
> +        s = baseset(s - repo.changelog.filteredrevs)
> +        s.sort()
>          return subset & s
>      else:
>          phase = repo._phasecache.phase
>          target = phases.public
>          condition = lambda r: phase(repo, r) != target
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - June 22, 2015, 5:01 p.m.
On 06/22/2015 07:12 AM, Augie Fackler wrote:
> On Thu, Jun 18, 2015 at 10:04:55AM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1433991507 25200
>> #      Wed Jun 10 19:58:27 2015 -0700
>> # Node ID 32d7dee9420183af58bd5ce150908425e708de77
>> # Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
>> revset: use a baseset in _notpublic()
>
> Queued, thanks.

Matt already took this one (on saturday or something)
Augie Fackler - June 22, 2015, 5:02 p.m.
> On Jun 22, 2015, at 13:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 06/22/2015 07:12 AM, Augie Fackler wrote:
>> On Thu, Jun 18, 2015 at 10:04:55AM -0700, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1433991507 25200
>>> #      Wed Jun 10 19:58:27 2015 -0700
>>> # Node ID 32d7dee9420183af58bd5ce150908425e708de77
>>> # Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
>>> revset: use a baseset in _notpublic()
>> 
>> Queued, thanks.
> 
> Matt already took this one (on saturday or something)

So I noticed when trying to import. :)

> 
> 
> -- 
> Pierre-Yves David
Pierre-Yves David - June 22, 2015, 5:03 p.m.
On 06/22/2015 10:02 AM, Augie Fackler wrote:
>
>> On Jun 22, 2015, at 13:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 06/22/2015 07:12 AM, Augie Fackler wrote:
>>> On Thu, Jun 18, 2015 at 10:04:55AM -0700, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1433991507 25200
>>>> #      Wed Jun 10 19:58:27 2015 -0700
>>>> # Node ID 32d7dee9420183af58bd5ce150908425e708de77
>>>> # Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
>>>> revset: use a baseset in _notpublic()
>>>
>>> Queued, thanks.
>>
>> Matt already took this one (on saturday or something)
>
> So I noticed when trying to import. :)

The tools you use should probably be reading obsmarkers for main to 
catch these case.
Augie Fackler - June 22, 2015, 5:04 p.m.
> On Jun 22, 2015, at 13:03, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 06/22/2015 10:02 AM, Augie Fackler wrote:
>> 
>>> On Jun 22, 2015, at 13:01, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>> 
>>> 
>>> 
>>> On 06/22/2015 07:12 AM, Augie Fackler wrote:
>>>> On Thu, Jun 18, 2015 at 10:04:55AM -0700, Pierre-Yves David wrote:
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>>> # Date 1433991507 25200
>>>>> #      Wed Jun 10 19:58:27 2015 -0700
>>>>> # Node ID 32d7dee9420183af58bd5ce150908425e708de77
>>>>> # Parent  e0d29cae67f9ad7324fc4818bed2248aeef0af83
>>>>> revset: use a baseset in _notpublic()
>>>> 
>>>> Queued, thanks.
>>> 
>>> Matt already took this one (on saturday or something)
>> 
>> So I noticed when trying to import. :)
> 
> The tools you use should probably be reading obsmarkers for main to catch these case.

I'm close to writing a bot that closes the loop with the mailing list when someone ninja-queues something.

Email is very much the source of truth for me.

> 
> 
> -- 
> Pierre-Yves David

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1536,12 +1536,12 @@  def _notpublic(repo, subset, x):
     repo._phasecache.loadphaserevs(repo) # ensure phase's sets are loaded
     if repo._phasecache._phasesets:
         s = set()
         for u in repo._phasecache._phasesets[1:]:
             s.update(u)
-        # XXX we should turn this into a baseset instead of a set, smartset may
-        # do some optimisations from the fact this is a baseset.
+        s = baseset(s - repo.changelog.filteredrevs)
+        s.sort()
         return subset & s
     else:
         phase = repo._phasecache.phase
         target = phases.public
         condition = lambda r: phase(repo, r) != target