| Message ID | 0906e14c0e55b52573c7e0b632c7c639850700ec.1744924321.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pack-bitmap: enable lookup tables by default, misc. cleanups | expand |
Taylor Blau <me@ttaylorr.com> writes: > 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. > > Use test_expect_success to avoid spending time repeating an idempotent > portion of the setup for performance tests that use test_pack_bitmap(). > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > t/perf/lib-bitmap.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) OK. > > 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 > '
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
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 '
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. Use test_expect_success to avoid spending time repeating an idempotent portion of the setup for performance tests that use test_pack_bitmap(). Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/perf/lib-bitmap.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)