From: Derrick Stolee <stolee@gmail.com>
Date: Sat, 07 Oct 2017 19:12:08 GMT
Subject: Re: [PATCH v2 1/5] test-list-objects: List a subset of object ids
Message-ID: <d5b3107f-76ec-8e8c-d7f5-d0f1ec75b1c9@gmail.com>
In-Reply-To: <20171006141128.rkuhd4m7hp2f7vvx@sigill.intra.peff.net>

On 10/6/2017 10:11 AM, Jeff King wrote:
> On Thu, Oct 05, 2017 at 08:39:42AM -0400, Derrick Stolee wrote:
>> I'll run some perf numbers for these commands you recommend, and also see if
>> I can replicate some of the pain points that triggered this change using the
>> Linux repo.
> 
> Thanks!
> 
> -Peff
> 

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.

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!)

I made note of a few things, but will not include them in my re-roll. 
I'll revisit them later if they are valuable:

- nth_packed_object_sha1() could be simplified in
   find_abbrev_len_for_pack().

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

- Ramsay caught my lack of "static" in test-list-objects.c, but that
   file will be removed in the next patch. I'll make sure to use "static"
   in the future.

I'm not re-rolling immediately to allow for some extra review over the 
weekend, if anyone is so inclined.

Thanks,
-Stolee