Re: [PATCH 4/4] t/perf/lib-bitmap.sh: avoid test_perf during setup
- From
Jeff King <peff@peff.net>
- Date
- Apr 18, 2025, 10:17 UTC
- Message-ID
- <20250418101725.GD10441@coredump.intra.peff.net>
- In-Reply-To
- <0906e14c0e55b52573c7e0b632c7c639850700ec.1744924321.git.me@ttaylorr.com>
On Thu, Apr 17, 2025 at 05:12:23PM -0400, Taylor Blau wrote:
Show 8 quoted lines
> 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.
Show 12 quoted lines
> 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