| Message ID | 51c4604e16c886d888138f2b513e4d3407b10728.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: > Subject: Re: [PATCH 2/4] p5312: removed duplicate performance test script "removed" -> "remove"??? > When the reachability bitmap format learned to read and write a lookup > table containing the set of commits which received reachability bitmaps, > commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup > table, 2022-08-14) added that mirrored p5310 but with reverse indexes > enabled. "added that" -> "added a <something> that"??? > Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by > default, 2023-04-12), we enabled reverse indexes by default, which made > these two tests indistinguishable from one another. Commit a8dd7e05b1 > should have removed p5312 as a duplicate, but didn't do so. Or to retain the same coverage, it should have explicitly disabled reverse index in one of the tests, while allowing the other to use the reverse index enabled by default, perhaps? > Correct that by removing p5312 as a functional duplicate of p5310. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > t/perf/p5312-pack-bitmaps-revs.sh | 34 ------------------------------- > 1 file changed, 34 deletions(-) > delete mode 100755 t/perf/p5312-pack-bitmaps-revs.sh > > diff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh > deleted file mode 100755 > index ceec60656b..0000000000 > --- a/t/perf/p5312-pack-bitmaps-revs.sh > +++ /dev/null > @@ -1,34 +0,0 @@ > -#!/bin/sh > - > -test_description='Tests pack performance using bitmaps (rev index enabled)' > -. ./perf-lib.sh > -. "${TEST_DIRECTORY}/perf/lib-bitmap.sh" > - > -test_lookup_pack_bitmap () { > - test_expect_success 'start the test from scratch' ' > - rm -rf * .git > - ' > - > - test_perf_large_repo > - > - test_expect_success 'setup bitmap config' ' > - git config pack.writebitmaps true > - ' > - > - # we need to create the tag up front such that it is covered by the repack and > - # thus by generated bitmaps. > - test_expect_success 'create tags' ' > - git tag --message="tag pointing to HEAD" perf-tag HEAD > - ' > - > - test_perf "enable lookup table: $1" ' > - git config pack.writeBitmapLookupTable '"$1"' > - ' > - > - test_pack_bitmap > -} > - > -test_lookup_pack_bitmap false > -test_lookup_pack_bitmap true > - > -test_done
On Thu, Apr 17, 2025 at 03:08:59PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > Subject: Re: [PATCH 2/4] p5312: removed duplicate performance test script > > "removed" -> "remove"??? > > > When the reachability bitmap format learned to read and write a lookup > > table containing the set of commits which received reachability bitmaps, > > commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup > > table, 2022-08-14) added that mirrored p5310 but with reverse indexes > > enabled. > > "added that" -> "added a <something> that"??? I am embarrassed. These are both awful. Here's the relevant portion of the range-diff: 2: 51c4604e16 ! 2: a80a7b5e60 p5312: removed duplicate performance test script @@ Metadata Author: Taylor Blau <me@ttaylorr.com> ## Commit message ## - p5312: removed duplicate performance test script + p5312: remove duplicate performance test script When the reachability bitmap format learned to read and write a lookup table containing the set of commits which received reachability bitmaps, commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup - table, 2022-08-14) added that mirrored p5310 but with reverse indexes - enabled. + table, 2022-08-14) added a new performance test script mirroring p5310 + but with reverse indexes enabled. Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by default, 2023-04-12), we enabled reverse indexes by default, which made > > Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by > > default, 2023-04-12), we enabled reverse indexes by default, which made > > these two tests indistinguishable from one another. Commit a8dd7e05b1 > > should have removed p5312 as a duplicate, but didn't do so. > > Or to retain the same coverage, it should have explicitly disabled > reverse index in one of the tests, while allowing the other to use > the reverse index enabled by default, perhaps? I don't think we necessarily would benefit from having two variants of this performance test. It is a little annoying to maintain, but that isn't the main reason that I proposed removing it here. I think that the pair of performance tests were useful in proving out the lookup table extension as useful to bitmaps' performance characteristics by comparison to the non-lookup table version. In that sense, I think the pair of performance tests were useful as a contrast to one another. Since we have evidence of their usefulness, the contrast is less important IMHO. I think we still want to keep the "lookup table enabled" version to prevent regressions, though. Thanks, Taylor
diff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh deleted file mode 100755 index ceec60656b..0000000000 --- a/t/perf/p5312-pack-bitmaps-revs.sh +++ /dev/null @@ -1,34 +0,0 @@ -#!/bin/sh - -test_description='Tests pack performance using bitmaps (rev index enabled)' -. ./perf-lib.sh -. "${TEST_DIRECTORY}/perf/lib-bitmap.sh" - -test_lookup_pack_bitmap () { - test_expect_success 'start the test from scratch' ' - rm -rf * .git - ' - - test_perf_large_repo - - test_expect_success 'setup bitmap config' ' - git config pack.writebitmaps true - ' - - # we need to create the tag up front such that it is covered by the repack and - # thus by generated bitmaps. - test_expect_success 'create tags' ' - git tag --message="tag pointing to HEAD" perf-tag HEAD - ' - - test_perf "enable lookup table: $1" ' - git config pack.writeBitmapLookupTable '"$1"' - ' - - test_pack_bitmap -} - -test_lookup_pack_bitmap false -test_lookup_pack_bitmap true - -test_done
When the reachability bitmap format learned to read and write a lookup table containing the set of commits which received reachability bitmaps, commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup table, 2022-08-14) added that mirrored p5310 but with reverse indexes enabled. Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by default, 2023-04-12), we enabled reverse indexes by default, which made these two tests indistinguishable from one another. Commit a8dd7e05b1 should have removed p5312 as a duplicate, but didn't do so. Correct that by removing p5312 as a functional duplicate of p5310. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/perf/p5312-pack-bitmaps-revs.sh | 34 ------------------------------- 1 file changed, 34 deletions(-) delete mode 100755 t/perf/p5312-pack-bitmaps-revs.sh