Patchwork [1,of,2,censor,RFC,v2] changegroup: clean up misnamed local variable in delta generation logic

login
register
mail settings
Submitter adgar@google.com
Date Feb. 20, 2015, 9:54 p.m.
Message ID <96a3db818b2ec56f420c.1424469273@adgar.nyc.corp.google.com>
Download mbox | patch
Permalink /patch/7813/
State Deferred
Headers show

Comments

adgar@google.com - Feb. 20, 2015, 9:54 p.m.
# HG changeset patch
# User Mike Edgar <adgar@google.com>
# Date 1424465057 18000
#      Fri Feb 20 15:44:17 2015 -0500
# Node ID 96a3db818b2ec56f420c677ca38ea34dc46af9ff
# Parent  75f94dcf76fdfeaebeaaea279ca5b88e3bc8a20b
changegroup: clean up misnamed local variable in delta generation logic

The actual changegroup delta that is produced is sometimes split across two
variables, "prefix" and "delta"; othertimes, it is wholly contained in
"delta". This is confusing and inhibits extension of this logic.
Matt Mackall - Feb. 26, 2015, 12:26 a.m.
On Fri, 2015-02-20 at 16:54 -0500, Mike Edgar wrote:
> # HG changeset patch
> # User Mike Edgar <adgar@google.com>
> # Date 1424465057 18000
> #      Fri Feb 20 15:44:17 2015 -0500
> # Node ID 96a3db818b2ec56f420c677ca38ea34dc46af9ff
> # Parent  75f94dcf76fdfeaebeaaea279ca5b88e3bc8a20b
> changegroup: clean up misnamed local variable in delta generation logic
> 
> The actual changegroup delta that is produced is sometimes split across two
> variables, "prefix" and "delta"; othertimes, it is wholly contained in
> "delta". This is confusing and inhibits extension of this logic.

The reason for the original code is that sometimes a snapshot was, say,
1G.. and adding 12 bytes to it meant 2G peak memory use (not to mention
some memcpy overhead). The split yield kept us at 1G, this brings us
back to 2.

> -            delta = revlog.revision(node)
> -            prefix = mdiff.trivialdiffheader(len(delta))
> +            snapshot = revlog.revision(node)
> +            delta = mdiff.trivialdiffheader(len(snapshot)) + snapshot
Pierre-Yves David - March 4, 2015, 11:26 p.m.
On 02/25/2015 04:26 PM, Matt Mackall wrote:
> On Fri, 2015-02-20 at 16:54 -0500, Mike Edgar wrote:
>> # HG changeset patch
>> # User Mike Edgar <adgar@google.com>
>> # Date 1424465057 18000
>> #      Fri Feb 20 15:44:17 2015 -0500
>> # Node ID 96a3db818b2ec56f420c677ca38ea34dc46af9ff
>> # Parent  75f94dcf76fdfeaebeaaea279ca5b88e3bc8a20b
>> changegroup: clean up misnamed local variable in delta generation logic
>>
>> The actual changegroup delta that is produced is sometimes split across two
>> variables, "prefix" and "delta"; othertimes, it is wholly contained in
>> "delta". This is confusing and inhibits extension of this logic.
>
> The reason for the original code is that sometimes a snapshot was, say,
> 1G.. and adding 12 bytes to it meant 2G peak memory use (not to mention
> some memcpy overhead). The split yield kept us at 1G, this brings us
> back to 2.

I would like to point that the content of mpm email would make a lovely 
in code comment to be left for the new refactor-adventurer.

Patch

diff -r 75f94dcf76fd -r 96a3db818b2e mercurial/changegroup.py
--- a/mercurial/changegroup.py	Thu Feb 19 19:32:06 2015 +0800
+++ b/mercurial/changegroup.py	Fri Feb 20 15:44:17 2015 -0500
@@ -480,16 +480,14 @@ 
         p1, p2 = revlog.parentrevs(rev)
         base = self.deltaparent(revlog, rev, p1, p2, prev)
 
-        prefix = ''
         if base == nullrev:
-            delta = revlog.revision(node)
-            prefix = mdiff.trivialdiffheader(len(delta))
+            snapshot = revlog.revision(node)
+            delta = mdiff.trivialdiffheader(len(snapshot)) + snapshot
         else:
             delta = revlog.revdiff(base, rev)
         p1n, p2n = revlog.parents(node)
         basenode = revlog.node(base)
         meta = self.builddeltaheader(node, p1n, p2n, basenode, linknode)
-        meta += prefix
         l = len(meta) + len(delta)
         yield chunkheader(l)
         yield meta