Patchwork [6,of,8] hidden: remove _consistencyblockers()

login
register
mail settings
Submitter via Mercurial-devel
Date May 28, 2017, 6:15 a.m.
Message ID <f8b3d943ff21a2b906f2.1495952134@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/20971/
State Changes Requested
Headers show

Comments

via Mercurial-devel - May 28, 2017, 6:15 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1495950919 25200
#      Sat May 27 22:55:19 2017 -0700
# Node ID f8b3d943ff21a2b906f2806476a637aac93763c4
# Parent  8b91ef8f7f051165c098f3ebfc871e90afada070
hidden: remove  _consistencyblockers()

Roughly speaking, we currently do this to reveal hidden ancestors of
visible revisions:

 1. Iterate over all visible non-public revisions and see if they have
    hidden parents

 2. For each revision found in step (1) walk the chain of hidden
    commits and reveal it

We can simplify that by skipping step (1) and doing step (2) from all
visible non-public revisions instead.

This doesn't seem to have much impact on "perfvolatilesets".

Before:
! obsolete
! wall 0.004616 comb 0.000000 user 0.000000 sys 0.000000 (best of 570)
! visible
! wall 0.008235 comb 0.010000 user 0.010000 sys 0.000000 (best of 326)

After:
! obsolete
! wall 0.004727 comb 0.010000 user 0.010000 sys 0.000000 (best of 543)
! visible
! wall 0.008371 comb 0.000000 user 0.000000 sys 0.000000 (best of 324)
Pierre-Yves David - May 29, 2017, 4:50 p.m.
On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495950919 25200
> #      Sat May 27 22:55:19 2017 -0700
> # Node ID f8b3d943ff21a2b906f2806476a637aac93763c4
> # Parent  8b91ef8f7f051165c098f3ebfc871e90afada070
> hidden: remove  _consistencyblockers()
>
> Roughly speaking, we currently do this to reveal hidden ancestors of
> visible revisions:
>
>  1. Iterate over all visible non-public revisions and see if they have
>     hidden parents
>
>  2. For each revision found in step (1) walk the chain of hidden
>     commits and reveal it
>
> We can simplify that by skipping step (1) and doing step (2) from all
> visible non-public revisions instead.
>
> This doesn't seem to have much impact on "perfvolatilesets".
>
> Before:
> ! obsolete
> ! wall 0.004616 comb 0.000000 user 0.000000 sys 0.000000 (best of 570)
> ! visible
> ! wall 0.008235 comb 0.010000 user 0.010000 sys 0.000000 (best of 326)
>
> After:
> ! obsolete
> ! wall 0.004727 comb 0.010000 user 0.010000 sys 0.000000 (best of 543)
> ! visible
> ! wall 0.008371 comb 0.000000 user 0.000000 sys 0.000000 (best of 324)
>
> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -44,20 +44,6 @@
>          anchors.update(rev(t[0]) for t in tags.values() if t[0] in nodemap)
>      return anchors
>
> -def _consistencyblocker(pfunc, hideable, revs):
> -    """return non-hideable changeset blocking hideable one
> -
> -    For consistency, we cannot actually hide a changeset if one of it children
> -    are visible, this function find such children.
> -    """
> -    blockers = set()
> -    for r in revs:
> -        for p in pfunc(r):
> -            if p != nullrev and p in hideable:
> -                blockers.add(r)
> -                break
> -    return blockers
> -
>  def _revealancestors(hidden, pfunc, revs):
>      """reveals contiguous chains of hidden ancestors of 'revs' by removing them
>      from 'hidden'
> @@ -89,16 +75,12 @@
>          mutablephases = (phases.draft, phases.secret)
>          mutable = repo._phasecache.getrevset(repo, mutablephases)
>
> -        visible = mutable - hidden
> -        blockers = _consistencyblocker(pfunc, hidden, visible)
> -
> -        # check if we have wd parents, bookmarks or tags pointing to hidden
> -        # changesets and remove those.
> -        blockers |= (hidden & anchorrevs(repo))
> -        if blockers:
> +        visible = set(mutable - hidden)

I've not sure why we create a set from the operation result. That looks 
like a wasteful double allocation. Am I missing something?
via Mercurial-devel - May 29, 2017, 4:55 p.m.
On May 29, 2017 9:50 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495950919 25200
> #      Sat May 27 22:55:19 2017 -0700
> # Node ID f8b3d943ff21a2b906f2806476a637aac93763c4
> # Parent  8b91ef8f7f051165c098f3ebfc871e90afada070
> hidden: remove  _consistencyblockers()
>
> Roughly speaking, we currently do this to reveal hidden ancestors of
> visible revisions:
>
>  1. Iterate over all visible non-public revisions and see if they have
>     hidden parents
>
>  2. For each revision found in step (1) walk the chain of hidden
>     commits and reveal it
>
> We can simplify that by skipping step (1) and doing step (2) from all
> visible non-public revisions instead.
>
> This doesn't seem to have much impact on "perfvolatilesets".
>
> Before:
> ! obsolete
> ! wall 0.004616 comb 0.000000 user 0.000000 sys 0.000000 (best of 570)
> ! visible
> ! wall 0.008235 comb 0.010000 user 0.010000 sys 0.000000 (best of 326)
>
> After:
> ! obsolete
> ! wall 0.004727 comb 0.010000 user 0.010000 sys 0.000000 (best of 543)
> ! visible
> ! wall 0.008371 comb 0.000000 user 0.000000 sys 0.000000 (best of 324)
>
> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -44,20 +44,6 @@
>          anchors.update(rev(t[0]) for t in tags.values() if t[0] in
> nodemap)
>      return anchors
>
> -def _consistencyblocker(pfunc, hideable, revs):
> -    """return non-hideable changeset blocking hideable one
> -
> -    For consistency, we cannot actually hide a changeset if one of it
> children
> -    are visible, this function find such children.
> -    """
> -    blockers = set()
> -    for r in revs:
> -        for p in pfunc(r):
> -            if p != nullrev and p in hideable:
> -                blockers.add(r)
> -                break
> -    return blockers
> -
>  def _revealancestors(hidden, pfunc, revs):
>      """reveals contiguous chains of hidden ancestors of 'revs' by
> removing them
>      from 'hidden'
> @@ -89,16 +75,12 @@
>          mutablephases = (phases.draft, phases.secret)
>          mutable = repo._phasecache.getrevset(repo, mutablephases)
>
> -        visible = mutable - hidden
> -        blockers = _consistencyblocker(pfunc, hidden, visible)
> -
> -        # check if we have wd parents, bookmarks or tags pointing to
> hidden
> -        # changesets and remove those.
> -        blockers |= (hidden & anchorrevs(repo))
> -        if blockers:
> +        visible = set(mutable - hidden)
>

I've not sure why we create a set from the operation result. That looks
like a wasteful double allocation. Am I missing something?


as A
via Mercurial-devel - May 29, 2017, 5:07 p.m.
On May 29, 2017 9:55 AM, "Martin von Zweigbergk" <martinvonz@google.com>
wrote:



On May 29, 2017 9:50 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495950919 25200
> #      Sat May 27 22:55:19 2017 -0700
> # Node ID f8b3d943ff21a2b906f2806476a637aac93763c4
> # Parent  8b91ef8f7f051165c098f3ebfc871e90afada070
> hidden: remove  _consistencyblockers()
>
> Roughly speaking, we currently do this to reveal hidden ancestors of
> visible revisions:
>
>  1. Iterate over all visible non-public revisions and see if they have
>     hidden parents
>
>  2. For each revision found in step (1) walk the chain of hidden
>     commits and reveal it
>
> We can simplify that by skipping step (1) and doing step (2) from all
> visible non-public revisions instead.
>
> This doesn't seem to have much impact on "perfvolatilesets".
>
> Before:
> ! obsolete
> ! wall 0.004616 comb 0.000000 user 0.000000 sys 0.000000 (best of 570)
> ! visible
> ! wall 0.008235 comb 0.010000 user 0.010000 sys 0.000000 (best of 326)
>
> After:
> ! obsolete
> ! wall 0.004727 comb 0.010000 user 0.010000 sys 0.000000 (best of 543)
> ! visible
> ! wall 0.008371 comb 0.000000 user 0.000000 sys 0.000000 (best of 324)
>
> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -44,20 +44,6 @@
>          anchors.update(rev(t[0]) for t in tags.values() if t[0] in
> nodemap)
>      return anchors
>
> -def _consistencyblocker(pfunc, hideable, revs):
> -    """return non-hideable changeset blocking hideable one
> -
> -    For consistency, we cannot actually hide a changeset if one of it
> children
> -    are visible, this function find such children.
> -    """
> -    blockers = set()
> -    for r in revs:
> -        for p in pfunc(r):
> -            if p != nullrev and p in hideable:
> -                blockers.add(r)
> -                break
> -    return blockers
> -
>  def _revealancestors(hidden, pfunc, revs):
>      """reveals contiguous chains of hidden ancestors of 'revs' by
> removing them
>      from 'hidden'
> @@ -89,16 +75,12 @@
>          mutablephases = (phases.draft, phases.secret)
>          mutable = repo._phasecache.getrevset(repo, mutablephases)
>
> -        visible = mutable - hidden
> -        blockers = _consistencyblocker(pfunc, hidden, visible)
> -
> -        # check if we have wd parents, bookmarks or tags pointing to
> hidden
> -        # changesets and remove those.
> -        blockers |= (hidden & anchorrevs(repo))
> -        if blockers:
> +        visible = set(mutable - hidden)
>

I've not sure why we create a set from the operation result. That looks
like a wasteful double allocation. Am I missing something?


as A
Jun Wu - June 3, 2017, 5:38 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-06-03 10:05:06 -0700:
> [...]
> If all current code paths use the full set (and don't care about order), I

I think we are talking about phase revsets.

The order matters. If you look at 17b5cda5a84a, note that "s.sort()" was
removed to improve performance. 

> think it makes sense to return a plain set. I'd accept a patch that changes
> that. I realize it conflicts with your vision, but it sounds like a pretty
> small patch, so I'm not concerned about the possible churn of taking such a
> patch that you later back out when you make things lazier.
Pierre-Yves David - June 3, 2017, 7:38 p.m.
On 06/03/2017 07:38 PM, Jun Wu wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-06-03 10:05:06 -0700:
>> [...]
>> If all current code paths use the full set (and don't care about order), I
>
> I think we are talking about phase revsets.

No. we are talking about the low level set computation, like in 
'_computeobsoleteset' and 'computehidden'. The revsets are out of the 
scope for that discussion.

The function aboves performs unordered operation on python 'set' object 
and return a python 'set' object.

The phase logic itself is computing and cache python 'set' object. 
Having the phase logic explicitly covert that set to a smartset before 
making it available to this low level function is not necessary and end 
up being harmful for performance.

I just made a small series that allow to skip the smartset conversion 
for these function. It pass tests and reveals a quite significant impact 
from the smartset usage:

In mozilla-central greg development repository:
   before
     obsolete: 0.000885,
     visible:  0.002548
   after
     obsolete: 0.576 ms (-35%)
     visible:  1.098 ms (-57%)

On my Mercurial Core repository:
   before
     obsolete: 1.271 ms
     visible:  3.424 ms
   after
     obsolete: 0.950 ms (-25%)
     visible:  1.882 ms (-45%)

That series is just making existing data available to function that can 
use them. That would be fairly easy to update in the future if we end up 
needing to do so.

In addition, Having a smartset eventually required Martin to do a 
conversion to a set to have an object with the set of feature he needed. 
Since they are intended for revset usage, not as a replacement for 
python 'set'.

Cheers,
Jun Wu - June 3, 2017, 8:08 p.m.
Excerpts from Pierre-Yves David's message of 2017-06-03 21:38:47 +0200:
> 
> On 06/03/2017 07:38 PM, Jun Wu wrote:
> > Excerpts from Martin von Zweigbergk's message of 2017-06-03 10:05:06 -0700:
> >> [...]
> >> If all current code paths use the full set (and don't care about order), I
> >
> > I think we are talking about phase revsets.
> 
> No. we are talking about the low level set computation, like in 
> '_computeobsoleteset' and 'computehidden'. The revsets are out of the 
> scope for that discussion.

I understand we want to continue use Python sets in obsolete.py. I didn't
say that's a problem.

> The function aboves performs unordered operation on python 'set' object 
> and return a python 'set' object.
> 
> The phase logic itself is computing and cache python 'set' object. 
> Having the phase logic explicitly covert that set to a smartset before 
> making it available to this low level function is not necessary and end 
> up being harmful for performance.
> 
> I just made a small series that allow to skip the smartset conversion 
> for these function. It pass tests and reveals a quite significant impact 

My point is we can just convert a smartset to Python set in an efficient way
(ex. "[PATCH] smartset: add a "toset" method"). Then the change will just be
something like:

  # in obsolete.py
  - s = set(nonpublic)
  + s = nonpublic.toset()

I'd expect it to be as efficient as accessing "phasecache._phasesets"
directly. It is more flexible.

> [...]

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -44,20 +44,6 @@ 
         anchors.update(rev(t[0]) for t in tags.values() if t[0] in nodemap)
     return anchors
 
-def _consistencyblocker(pfunc, hideable, revs):
-    """return non-hideable changeset blocking hideable one
-
-    For consistency, we cannot actually hide a changeset if one of it children
-    are visible, this function find such children.
-    """
-    blockers = set()
-    for r in revs:
-        for p in pfunc(r):
-            if p != nullrev and p in hideable:
-                blockers.add(r)
-                break
-    return blockers
-
 def _revealancestors(hidden, pfunc, revs):
     """reveals contiguous chains of hidden ancestors of 'revs' by removing them
     from 'hidden'
@@ -89,16 +75,12 @@ 
         mutablephases = (phases.draft, phases.secret)
         mutable = repo._phasecache.getrevset(repo, mutablephases)
 
-        visible = mutable - hidden
-        blockers = _consistencyblocker(pfunc, hidden, visible)
-
-        # check if we have wd parents, bookmarks or tags pointing to hidden
-        # changesets and remove those.
-        blockers |= (hidden & anchorrevs(repo))
-        if blockers:
+        visible = set(mutable - hidden)
+        visible |= (hidden & anchorrevs(repo))
+        if visible:
             # don't modify possibly cached result of hideablerevs()
             hidden = hidden.copy()
-            _revealancestors(hidden, pfunc, blockers)
+            _revealancestors(hidden, pfunc, visible)
     return frozenset(hidden)
 
 def computeunserved(repo):