Re: [PATCH 0/3] lazily load commit->buffer

[Jeff King <peff@peff.net>]

On Sat, Jan 26, 2013 at 01:26:53PM -0800, Junio C Hamano wrote:

> This looks very good.
> 
> I wonder if this lets us get rid of the hack in cmd_log_walk() that
> does this:
> 
>         while ((commit = get_revision(rev)) != NULL) {
>                 if (!log_tree_commit(rev, commit) &&
>                     rev->max_count >= 0)
>                         rev->max_count++;
> !               if (!rev->reflog_info) {
> !                       /* we allow cycles in reflog ancestry */
>                         free(commit->buffer);
>                         commit->buffer = NULL;
> !               }
>                 free_commit_list(commit->parents);
>                 commit->parents = NULL;
> 
> After log_tree_commit() handles the commit, using the buffer, we
> discard the memory associated to it because we know we no longer
> will use it in normal cases.
> [...]
> But that is a performance thing, not a correctness issue, so "we
> allow cycles" implying "therefore if we discard the buffer, we will
> show wrong output" becomes an incorrect justification.

Right. I think the correctness issue goes away with my patches, and it
is just a question of estimating the workload for performance. I doubt
it makes a big difference either way, especially when compared to
actually showing the commit (even a single pathspec limiter, or doing
"-p", would likely dwarf a few extra commit decompressions).

My HEAD has about 400/3000 non-unique commits, which matches your
numbers percentage-wise. Dropping the lines above (and always freeing)
takes my best-of-five for "git log -g" from 0.085s to 0.080s. Which is
well within the noise.  Doing "git log -g Makefile" ended up at 0.183s
both before and after.

So I suspect it does not matter at all in normal cases, and the time is
indeed dwarfed by adding even a rudimentary pathspec. I'd be in favor of
dropping the lines just to decrease complexity of the code.

-Peff