Re: [PATCH v2 6/7] p5326: generate pack bitmaps before writing the MIDX bitmap

From: Jeff King <peff@peff.net>
Date: Sep 16, 2021, 22:45 UTC
Message-ID: <YUPJDf7u8Nn5als3@coredump.intra.peff.net>
In-Reply-To: <040bb40548017bae807c1d349fa078c21ac46725.1631657157.git.me@ttaylorr.com>

On Tue, Sep 14, 2021 at 06:06:14PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> To help test the performance of permuting the contents of the hash-cache
> when generating a MIDX bitmap, we need a bitmap which has its hash-cache
> populated.
> 
> And since multi-pack bitmaps don't add *new* values to the hash-cache,
> we have to rely on a single-pack bitmap to generate those values for us.
> 
> Therefore, generate a pack bitmap before the MIDX one in order to ensure
> that the MIDX bitmap has entries in its hash-cache.

Makes sense. This is a little more contrived of a setup than the original, but an utterly realistic one. If you are using midx bitmaps, you are probably interspersing them with occasional full pack bitmaps.

It might be interesting to also do:

  rm -f .git/objects/pack/pack-*.bitmap

after generating the midx bitmap. That would confirm the further timing tests are using the midx bitmap, and not ever "cheating" by looking at the pack one (having poked in this direction before, I know that this all works, so it would be a future-proofing thing).

Show 12 quoted lines

> diff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh
> index a9c5499537..38557859b7 100755
> --- a/t/perf/p5326-multi-pack-bitmaps.sh
> +++ b/t/perf/p5326-multi-pack-bitmaps.sh
> @@ -13,7 +13,7 @@ test_expect_success 'create tags' '
>  '
>  
>  test_perf 'setup multi-pack index' '
> -	git repack -ad &&
> +	git repack -adb &&
>  	git multi-pack-index write --bitmap
>  '

This sort-of existed before your series, but I think is a bit "worse" now: we are timing both "repack" and "multi-pack-index" write together. So:

  - the timing for the midx write that we are interested in timing will
    be diluted by the much-bigger full-repack

  - we'll actually do _three_ full repacks (the default
    GIT_PERF_REPEAT_COUNT for the "run" script), since it's inside a
    test_perf()

So:

  test_expect_success 'start with bitmapped pack' '
	git repack -adb
  '

  test_perf 'setup multi-pack index' '
	git multi-pack-index write --bitmap
  '

would run faster and give us more interesting timings. Possibly you'd want to drop the midx and its bitmaps as part of that test_perf, too. The first run will be using the pack bitmap, and the others will use the midx. I doubt it makes much difference either way, though.

And of course if you want to take my earlier suggestion, then it's easy to add:

  test_expect_success 'drop pack bitmap' '
	rm -f .git/objects/pack/pack-*.bitmap
  '

afterwards; you wouldn't want to do it inside the test_perf() call because of the repeat-count.

-Peff