Patchwork [4,of,4,more-matchers] manifestmerge: have manifest do matching before diffing

login
register
mail settings
Submitter Augie Fackler
Date Dec. 15, 2015, 3:17 a.m.
Message ID <7048ea9ba5b92e4f5825.1450149440@imladris.local>
Download mbox | patch
Permalink /patch/12051/
State Superseded
Commit 2e31a17ad1bfba63741e9eecaa48ae12daded4c4
Headers show

Comments

Augie Fackler - Dec. 15, 2015, 3:17 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1450144641 18000
#      Mon Dec 14 20:57:21 2015 -0500
# Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
# Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
manifestmerge: have manifest do matching before diffing

This means that the diff code does less work, potentially
significantly less in the case of treemanifests. It also should help
with narrowed clone cases (such as narrowhg) when we don't always have
the entire set of treemanifest revlogs locally.
Martin von Zweigbergk - Dec. 15, 2015, 5:15 a.m.
On Mon, Dec 14, 2015 at 7:20 PM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1450144641 18000
> #      Mon Dec 14 20:57:21 2015 -0500
> # Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
> # Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
> manifestmerge: have manifest do matching before diffing
>
> This means that the diff code does less work, potentially
> significantly less in the case of treemanifests. It also should help
> with narrowed clone cases (such as narrowhg) when we don't always have
> the entire set of treemanifest revlogs locally.
>

Looks good to me. I'd simplify that definition of "partial" in flight and
queue if only the clowncopter had been up. And perhaps it appropriate for
someone not from the same company looks at it too.
Augie Fackler - Dec. 15, 2015, 5:56 p.m.
On Tue, Dec 15, 2015 at 12:15 AM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
>
> On Mon, Dec 14, 2015 at 7:20 PM Augie Fackler <raf@durin42.com> wrote:
>>
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1450144641 18000
>> #      Mon Dec 14 20:57:21 2015 -0500
>> # Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
>> # Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
>> manifestmerge: have manifest do matching before diffing
>>
>> This means that the diff code does less work, potentially
>> significantly less in the case of treemanifests. It also should help
>> with narrowed clone cases (such as narrowhg) when we don't always have
>> the entire set of treemanifest revlogs locally.
>
>
> Looks good to me. I'd simplify that definition of "partial" in flight and
> queue if only the clowncopter had been up. And perhaps it appropriate for
> someone not from the same company looks at it too.

Per an irc conversation I've added some mozilla-central timing
information and pushed these to the clowncopter. Thanks!
Matt Mackall - Dec. 15, 2015, 10:36 p.m.
On Tue, 2015-12-15 at 12:56 -0500, Augie Fackler wrote:
> On Tue, Dec 15, 2015 at 12:15 AM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
> > 
> > 
> > On Mon, Dec 14, 2015 at 7:20 PM Augie Fackler <raf@durin42.com>
> > wrote:
> > > 
> > > # HG changeset patch
> > > # User Augie Fackler <augie@google.com>
> > > # Date 1450144641 18000
> > > #      Mon Dec 14 20:57:21 2015 -0500
> > > # Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
> > > # Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
> > > manifestmerge: have manifest do matching before diffing
> > > 
> > > This means that the diff code does less work, potentially
> > > significantly less in the case of treemanifests. It also should
> > > help
> > > with narrowed clone cases (such as narrowhg) when we don't always
> > > have
> > > the entire set of treemanifest revlogs locally.
> > 
> > 
> > Looks good to me. I'd simplify that definition of "partial" in
> > flight and
> > queue if only the clowncopter had been up. And perhaps it
> > appropriate for
> > someone not from the same company looks at it too.
> 
> Per an irc conversation I've added some mozilla-central timing
> information and pushed these to the clowncopter. Thanks!

This timing info is mystifying:

-----
Before this refactoring:                                                                                                                                                                                                                       
                                                                                                                                                                                                                                               
augie% python ~/Programming/hg/crew/contrib/hgperf diff -c tip --stat                                                                                                                                                                          
! wall 0.547483 comb 0.550000 user 0.520000 sys 0.030000 (best of 18)                                                                                                                                                                          
augie% python ~/Programming/hg/crew/contrib/hgperf diff -c tip --stat xpcom                                                                                                                                                                    
! wall 0.179505 comb 0.180000 user 0.180000 sys 0.000000 (best of 47)                                                                                                                                                                          
                                                                                                                                                                                                                                               
After this refactoring:                                                                                                                                                                                                                        
                                                                                                                                                                                                                                               
augie% python ~/Programming/hg/crew/contrib/hgperf diff -c tip --stat                                                                                                                                                                          
! wall 0.467176 comb 0.460000 user 0.450000 sys 0.010000 (best of 20)                                                                                                                                                                          
augie% python ~/Programming/hg/crew/contrib/hgperf diff -c tip --stat xpcom                                                                                                                                                                    
! wall 0.208571 comb 0.200000 user 0.200000 sys 0.000000 (best of 45)                                                                                                                                                                          
-----     

So we're benchmarking the diff command... but the diff command never
touches merge.py. I stuck a 'print "yo"' right before the first line
touched by the diff to confirm that diffs are still not merges and was
pleased to see I'm not losing my mind.
Matt Mackall - Dec. 15, 2015, 10:38 p.m.
On Tue, 2015-12-15 at 16:36 -0600, Matt Mackall wrote:
> On Tue, 2015-12-15 at 12:56 -0500, Augie Fackler wrote:
> > On Tue, Dec 15, 2015 at 12:15 AM, Martin von Zweigbergk
> > <martinvonz@google.com> wrote:
> > > 
> > > 
> > > On Mon, Dec 14, 2015 at 7:20 PM Augie Fackler <raf@durin42.com>
> > > wrote:
> > > > 
> > > > # HG changeset patch
> > > > # User Augie Fackler <augie@google.com>
> > > > # Date 1450144641 18000
> > > > #      Mon Dec 14 20:57:21 2015 -0500
> > > > # Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
> > > > # Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
> > > > manifestmerge: have manifest do matching before diffing
> > > > 
> > > > This means that the diff code does less work, potentially
> > > > significantly less in the case of treemanifests. It also should
> > > > help
> > > > with narrowed clone cases (such as narrowhg) when we don't
> > > > always
> > > > have
> > > > the entire set of treemanifest revlogs locally.
> > > 
> > > 
> > > Looks good to me. I'd simplify that definition of "partial" in
> > > flight and
> > > queue if only the clowncopter had been up. And perhaps it
> > > appropriate for
> > > someone not from the same company looks at it too.
> > 
> > Per an irc conversation I've added some mozilla-central timing
> > information and pushed these to the clowncopter. Thanks!
> 
> This timing info is mystifying:

Also, what appears to be a stray verify hunk has snuck in. Dropping for
now.

-- 
Mathematics is the supreme nostalgia of our time.
Augie Fackler - Dec. 15, 2015, 11:32 p.m.
> On Dec 15, 2015, at 5:38 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> On Tue, 2015-12-15 at 16:36 -0600, Matt Mackall wrote:
>> On Tue, 2015-12-15 at 12:56 -0500, Augie Fackler wrote:
>>> On Tue, Dec 15, 2015 at 12:15 AM, Martin von Zweigbergk
>>> <martinvonz@google.com> wrote:
>>>> 
>>>> 
>>>> On Mon, Dec 14, 2015 at 7:20 PM Augie Fackler <raf@durin42.com>
>>>> wrote:
>>>>> 
>>>>> # HG changeset patch
>>>>> # User Augie Fackler <augie@google.com>
>>>>> # Date 1450144641 18000
>>>>> #      Mon Dec 14 20:57:21 2015 -0500
>>>>> # Node ID 7048ea9ba5b92e4f582524463a6167039fa44eeb
>>>>> # Parent  5eee49ed60caa99be140f7f80ac7fb07f1c0324d
>>>>> manifestmerge: have manifest do matching before diffing
>>>>> 
>>>>> This means that the diff code does less work, potentially
>>>>> significantly less in the case of treemanifests. It also should
>>>>> help
>>>>> with narrowed clone cases (such as narrowhg) when we don't
>>>>> always
>>>>> have
>>>>> the entire set of treemanifest revlogs locally.
>>>> 
>>>> 
>>>> Looks good to me. I'd simplify that definition of "partial" in
>>>> flight and
>>>> queue if only the clowncopter had been up. And perhaps it
>>>> appropriate for
>>>> someone not from the same company looks at it too.
>>> 
>>> Per an irc conversation I've added some mozilla-central timing
>>> information and pushed these to the clowncopter. Thanks!
>> 
>> This timing info is mystifying:
> 
> Also, what appears to be a stray verify hunk has snuck in. Dropping for
> now.

Mailed a v2 with timing info dropped (marmoute suggested that benchmark, I should have confirmed it mattered). As far as I can tell this is a barely-used codepath anyway (with the matcher), so I’m not too worried about timing overall.

Sorry about that verify hunk. That’s mystifying to me. I suspect an evolve bug (in grab), but have no supporting evidence.

> 
> -- 
> Mathematics is the supreme nostalgia of our time.
>

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -722,12 +722,13 @@  def manifestmerge(repo, wctx, p2, pa, br
                 break
 
     # Compare manifests
+    if matcher is not None:
+        m1 = m1.matches(matcher)
+        m2 = m2.matches(matcher)
     diff = m1.diff(m2)
 
     actions = {}
     for f, ((n1, fl1), (n2, fl2)) in diff.iteritems():
-        if matcher and not matcher(f):
-            continue
         if n1 and n2: # file exists on both local and remote side
             if f not in ma:
                 fa = copy.get(f, None)