Patchwork [3,of,3,STABLE] remotephase: avoid full changelog iteration (issue5964)

login
register
mail settings
Submitter Boris Feld
Date Aug. 17, 2018, 9:42 p.m.
Message ID <1c92e2f2b7f64d7e9758.1534542136@FB-lair>
Download mbox | patch
Permalink /patch/33836/
State Accepted
Headers show

Comments

Boris Feld - Aug. 17, 2018, 9:42 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1534530952 -7200
#      Fri Aug 17 20:35:52 2018 +0200
# Branch stable
# Node ID 1c92e2f2b7f64d7e975846de6fae36a1415ffe84
# Parent  0fee760d9736505d2315b8452ae9a60053ebefa0
# EXP-Topic phases-perf
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 1c92e2f2b7f6
remotephase: avoid full changelog iteration (issue5964)

Changeset 88efb7d6bcb6 introduced a performance regression by triggering a
full ancestors walk.

This changeset reworks this logic so that we no longer walk down the full
changelog. The motivation for 88efb7d6bcb6, issue5939, is still fixed.

mercurial compared to a draft repository
----------------------------------------

8eeed92475d5: 0.012637 seconds
88efb7d6bcb6: 0.202699 seconds (x16)
46da52f4b820: 0.215551 seconds (+6%)
this code:    0.008397 seconds (-33% from base)

The payload size reduction we see in `test-bookmarks-pushpull.t` comes from a
more aggressive filter of nullid and is harmless.
Yuya Nishihara - Aug. 18, 2018, 5:43 a.m.
On Fri, 17 Aug 2018 23:42:16 +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1534530952 -7200
> #      Fri Aug 17 20:35:52 2018 +0200
> # Branch stable
> # Node ID 1c92e2f2b7f64d7e975846de6fae36a1415ffe84
> # Parent  0fee760d9736505d2315b8452ae9a60053ebefa0
> # EXP-Topic phases-perf
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 1c92e2f2b7f6
> remotephase: avoid full changelog iteration (issue5964)
> 
> Changeset 88efb7d6bcb6 introduced a performance regression by triggering a
> full ancestors walk.
> 
> This changeset reworks this logic so that we no longer walk down the full
> changelog. The motivation for 88efb7d6bcb6, issue5939, is still fixed.
> 
> mercurial compared to a draft repository
> ----------------------------------------
> 
> 8eeed92475d5: 0.012637 seconds
> 88efb7d6bcb6: 0.202699 seconds (x16)
> 46da52f4b820: 0.215551 seconds (+6%)
> this code:    0.008397 seconds (-33% from base)
> 
> The payload size reduction we see in `test-bookmarks-pushpull.t` comes from a
> more aggressive filter of nullid and is harmless.
> 
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -664,11 +664,39 @@ def newheads(repo, heads, roots):
>  
>      * `heads`: define the first subset
>      * `roots`: define the second we subtract from the first"""
> +    # prevent an import cycle
> +    # phases > dagop > patch > copies > scmutil > obsolete > obsutil > phases
> +    from . import dagop
> +
> +    repo = repo.unfiltered()
> +    cl = repo.changelog
> +    rev = cl.nodemap.get
> +    if not roots:
> +        return heads
> +    if not heads or heads == [nullrev]:
                                ^^^^^^^^^
                                [nullid] ?

> +        return []
> +    # The logic operated on revisions, convert arguments early for convenience
> +    new_heads = set(rev(n) for n in heads if n != nullid)
> +    roots = [rev(n) for n in roots]

>      if not heads or not roots:
>          return heads

Nit: maybe dead code?

Patch

diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -664,11 +664,39 @@  def newheads(repo, heads, roots):
 
     * `heads`: define the first subset
     * `roots`: define the second we subtract from the first"""
+    # prevent an import cycle
+    # phases > dagop > patch > copies > scmutil > obsolete > obsutil > phases
+    from . import dagop
+
+    repo = repo.unfiltered()
+    cl = repo.changelog
+    rev = cl.nodemap.get
+    if not roots:
+        return heads
+    if not heads or heads == [nullrev]:
+        return []
+    # The logic operated on revisions, convert arguments early for convenience
+    new_heads = set(rev(n) for n in heads if n != nullid)
+    roots = [rev(n) for n in roots]
     if not heads or not roots:
         return heads
-    repo = repo.unfiltered()
-    revs = repo.revs('heads(::%ln - (%ln::%ln))', heads, roots, heads)
-    return pycompat.maplist(repo.changelog.node, revs)
+    # compute the area we need to remove
+    affected_zone = repo.revs("(%ld::%ld)", roots, new_heads)
+    # heads in the area are no longer heads
+    new_heads.difference_update(affected_zone)
+    # revisions in the area have children outside of it,
+    # They might be new heads
+    candidates = repo.revs("parents(%ld + (%ld and merge())) and not null",
+                           roots, affected_zone)
+    candidates -= affected_zone
+    if new_heads or candidates:
+        # remove candidate that are ancestors of other heads
+        new_heads.update(candidates)
+        prunestart = repo.revs("parents(%ld) and not null", new_heads)
+        pruned = dagop.reachableroots(repo, candidates, prunestart)
+        new_heads.difference_update(pruned)
+
+    return pycompat.maplist(cl.node, sorted(new_heads))
 
 def newcommitphase(ui):
     """helper to get the target phase of new commit
diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -141,10 +141,10 @@  delete a remote bookmark
   bundle2-output: payload chunk size: 23
   bundle2-output: closing payload chunk
   bundle2-output: bundle part: "check:phases"
-  bundle2-output-part: "check:phases" 48 bytes payload
+  bundle2-output-part: "check:phases" 24 bytes payload
   bundle2-output: part 2: "CHECK:PHASES"
   bundle2-output: header chunk size: 19
-  bundle2-output: payload chunk size: 48
+  bundle2-output: payload chunk size: 24
   bundle2-output: closing payload chunk
   bundle2-output: bundle part: "pushkey"
   bundle2-output-part: "pushkey" (params: 4 mandatory) empty payload
@@ -180,9 +180,9 @@  delete a remote bookmark
   bundle2-input: part parameters: 0
   bundle2-input: found a handler for part check:phases
   bundle2-input-part: "check:phases" supported
-  bundle2-input: payload chunk size: 48
+  bundle2-input: payload chunk size: 24
   bundle2-input: payload chunk size: 0
-  bundle2-input-part: total payload size 48
+  bundle2-input-part: total payload size 24
   bundle2-input: part header size: 90
   bundle2-input: part type: "PUSHKEY"
   bundle2-input: part id: "3"
@@ -253,10 +253,10 @@  delete a remote bookmark
   bundle2-output: payload chunk size: 23
   bundle2-output: closing payload chunk
   bundle2-output: bundle part: "check:phases"
-  bundle2-output-part: "check:phases" 48 bytes payload
+  bundle2-output-part: "check:phases" 24 bytes payload
   bundle2-output: part 2: "CHECK:PHASES"
   bundle2-output: header chunk size: 19
-  bundle2-output: payload chunk size: 48
+  bundle2-output: payload chunk size: 24
   bundle2-output: closing payload chunk
   bundle2-output: bundle part: "bookmarks"
   bundle2-output-part: "bookmarks" 23 bytes payload
@@ -293,9 +293,9 @@  delete a remote bookmark
   bundle2-input: part parameters: 0
   bundle2-input: found a handler for part check:phases
   bundle2-input-part: "check:phases" supported
-  bundle2-input: payload chunk size: 48
+  bundle2-input: payload chunk size: 24
   bundle2-input: payload chunk size: 0
-  bundle2-input-part: total payload size 48
+  bundle2-input-part: total payload size 24
   bundle2-input: part header size: 16
   bundle2-input: part type: "BOOKMARKS"
   bundle2-input: part id: "3"