Re: [PATCH v2 1/5] test-list-objects: List a subset of object ids

From: Jeff King <peff@peff.net>
Date: Oct 7, 2017, 19:33 UTC
Message-ID: <20171007193311.c2izgofpyh357yi7@sigill.intra.peff.net>
In-Reply-To: <d5b3107f-76ec-8e8c-d7f5-d0f1ec75b1c9@gmail.com>

On Sat, Oct 07, 2017 at 03:12:08PM -0400, Derrick Stolee wrote:

Show 11 quoted lines

> In my local copy, I added a test to p4211-line-log.sh that runs "git log
> --raw -r" and tested it on three copies of the Linux repo. In order, they
> have 1 packfile (0 loose), 24 packfiles (0 loose), and 23 packfiles
> (~324,000 loose).
> 
> 4211.6: git log --raw -r  43.34(42.62+0.65)   40.47(40.16+0.27)  -6.6%
> 4211.6: git log --raw -r  88.77(86.54+2.12)   82.44(81.87+0.52)  -7.1%
> 4211.6: git log --raw -r 108.86(103.97+4.81) 103.92(100.63+3.19) -4.5%
> 
> We have moderate performance gains for this command, despite the command
> doing many more things than just checking abbreviations.

Yeah, while it's less exciting than seeing the 90% numbers for a micro-benchmark, I think this represents real-world gains (and 5-7% is nothing to sneeze at).

You might also try adding "--format=%h" or --oneline to your invocation, which would compute abbreviations for each commit (making your workload more abbrev-heavy and possibly showing off the difference more).

I also think "-r" isn't doing anything. Recursive diffs are the default for the "log" porcelain (even for --raw).

Show 11 quoted lines

> I plan to re-roll my patch on Monday including the following feedback items:
> 
> * Remove test-list-objects and test-abbrev in favor of a new "git log"
>   performance test.
> 
> * Fix binary search overflow error.
> 
> * Check response from open_pack_index(p) in find_abbrev_len_for_pack().
>   I plan to return without failure on non-zero result, which results in
>   no failure on a bad pack and the abbreviation length will be the
>   minimum required among all valid packs. (Thanks Stefan!)

That all sounds reasonable to me.

> - Teach 'cat-file' to --batch-check='%(objectsize:short)'. (Peff already
>   included a patch, perhaps that could be reviewed separately.)

I think I'll let it lie in the list archive for now unless somebody has a real use case for it (though I'm tempted to add it purely for completionism, since it's so easy).

-Peff