Re: [PATCH 4/4] t/perf/lib-bitmap.sh: avoid test_perf during setup

[Jeff King <peff@peff.net>]

On Thu, Apr 17, 2025 at 05:12:23PM -0400, Taylor Blau wrote:

> In the test_pack_bitmap() helper function, we first repack the
> repository under test for consistency and to eliminate any effects from
> different distributions of objects among packs.
>
> This step is performed with test_perf, so it is repeated
> $GIT_PERF_REPEAT_COUNT number of times. But we do not care about timing
> this portion of the setup phase, and repeating the process does not
> change the outcome.

Isn't this also where we actually generate the bitmaps? I.e., it is
where we would see a performance regression in the bitmap writing
process (whereas the rest of the script is about the reading side).

That said, I don't think it's even doing that very well. It is mutating
the on-disk state, so the first run will potentially be much slower than
subsequent runs (since everything is now in one big pack with bitmaps,
and we try to reuse deltas and bitmap data as much as possible). And
since we take best-of-N, we're basically just measuring those subsequent
noop repacks (unless you set the repeat count to 1!).

I think we've run into this before, e.g. in 775c71e16d (p5302: create
the repo in each index-pack test, 2019-04-22). And there the solution
was to reset the repo state before each timing, assuming it is quick
enough not to affect the test too much. Our perf suite doesn't provide
much support there (we'd want something like hyperfine's --prepare
option).

So I dunno. It is possible for timing this operation to provide some
value, but I don't think the current implementation is doing that. And
it's quite expensive to run.

> diff --git a/t/perf/lib-bitmap.sh b/t/perf/lib-bitmap.sh
> index 55a8feb1dc..fdf5f35f1b 100644
> --- a/t/perf/lib-bitmap.sh
> +++ b/t/perf/lib-bitmap.sh
> @@ -69,7 +69,7 @@ test_partial_bitmap () {
>  }
>  
>  test_pack_bitmap () {
> -	test_perf "repack to disk" '
> +	test_expect_success "repack to disk" '
>  		git repack -ad
>  	'

The same issue exists in t5326, which calls "multi-pack-index write"
with the "--bitmap" flag, I think. So if we are going to do this, we'd
probably want the same there.

-Peff