[PATCH 3 of 3 v2] graft: apply get-only changes in-memory

David Schleimer dschleimer at fb.com
Mon Dec 3 16:28:47 CST 2012


Regarding the reviewability.  The code that actually moved is the bit that shows up as unchanged in the middle.  Doing that in a prior changeset helps a little, but most of the diff still ends up as a single large insertion.  I'll resend with it split up into more pieces, but I don't think I can get rid of the giant hunk.

> That could probably be applied to rebase and other similar operation too.

Potentially.  Siddharth was looking into it, but realized that the rebase time for us was dominated by the update at the end.  At that point, he changed direction to making update faster.  I understand he's planning on coming back to it after this lands.

> Applying this speedup to trivial filemerge without conflict seems possible too.

Yeah, it's absolutely possible.  The reason I haven't done it is that it's quite a bit more complex, and this diff is makes graft good enough for our needs.

> 40% speedup, woot!!
:)

> > In my testing, grafting a single revision (
with no file-level merges)
> > took 16 seconds both before and after this change.  Grafting 2
> > revisions (with no file-level merges) took 25 seconds before this
> > change, and 17 seconds after this change.  Grafting a single revision
> > (with a file-level merge) took 17 seconds before this change and 17
> > seconds after this change.  Grafting 2 revisions (with file-level
> > merges) took 29 seconds before this change and 29 seconds after this
> > change.
> 
> This want to be a summary table

Will do.

> > +        current = repo['.']
> 
> This change to the current logic (and other related) can probably be done before
> actually introducing something that require it.

I pulled the current tracking change out, but I don't want to pull out the change that checks if we've committed and updates the working copy.  I don't like the idea of committing unreachable (and therefore untestable) code.  It doesn't feel like such a commit moves from good, working state to good, working state.  However, if the consensus is that we would prefer smaller patches with dead code, then I will bow to that consensus and split it out.

> Same here, This change to extra is not necessary until you use in memory context
> but that would work to apply that before.

When I pull the code movement into a separate patch, this appears as a separate hunk.  I think that's sufficient to make the patch readable, and that sending too many patches can be as hard to review as too few so I'm not going to split it out.

> nickpick1: your message annonce a "fallback" but I do not see earlier message
> announcing what the main goal was.

Fair point.  I'll switch it to just ui.debug() which version of the merging it's using without the fallback bit.

> nickpick2: "if filelist:" is a bit hard to link to memctx. consider using an explicit
> boolean for clarity

I went back and forth about this.  I eventually settled on not having an explicit Boolean because it seemed like needless indirection.  I can see how someone that didn't write the code might differ, though.



More information about the Mercurial-devel mailing list