diff mbox series

[3/4] t/perf: avoid testing bitmaps without lookup table

Message ID 8cc5952e594b78ffb2ba4bcaabd62a8e5b8fe72a.1744924321.git.me@ttaylorr.com (mailing list archive)
State New
Headers show
Series pack-bitmap: enable lookup tables by default, misc. cleanups | expand

Commit Message

Taylor Blau April 17, 2025, 9:12 p.m. UTC
In a previous commit, the setting which controls whether or not the
pack- and MIDX-bitmap machinery writes a lookup table,
'pack.writeBitmapLookupTable' was enabled by default.

As a result, we can clean up many of our bitmap-related performance
tests. Many of the relevant performance tests look something like:

    test_it () {
      test_expect_success 'setup pack.writeBitmapLookupTable' '
        git config pack.writeBitmapLookupTable '"$1"'
      '

      # ...
    }

    test_it true
    test_it false

, where the two invocations of 'test_it' run the tests with and without
bitmap lookup tables enabled.

But now that lookup tables are enabled by default and have proven to be
a performance win, let's avoid benchmarking what is now an uncommon and
non-default scenario.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/perf/p5310-pack-bitmaps.sh         |  47 +++++-------
 t/perf/p5311-pack-bitmaps-fetch.sh   |  76 +++++++++----------
 t/perf/p5326-multi-pack-bitmaps.sh   | 107 ++++++++++++---------------
 t/perf/p5333-pseudo-merge-bitmaps.sh |   1 -
 4 files changed, 102 insertions(+), 129 deletions(-)

Comments

Junio C Hamano April 17, 2025, 10:21 p.m. UTC | #1
Taylor Blau <me@ttaylorr.com> writes:

> In a previous commit, the setting which controls whether or not the
> pack- and MIDX-bitmap machinery writes a lookup table,
> 'pack.writeBitmapLookupTable' was enabled by default.
>
> As a result, we can clean up many of our bitmap-related performance
> tests. Many of the relevant performance tests look something like:
>
>     test_it () {
>       test_expect_success 'setup pack.writeBitmapLookupTable' '
>         git config pack.writeBitmapLookupTable '"$1"'
>       '
>
>       # ...
>     }
>
>     test_it true
>     test_it false
>
> , where the two invocations of 'test_it' run the tests with and without
> bitmap lookup tables enabled.
>
> But now that lookup tables are enabled by default and have proven to be
> a performance win, let's avoid benchmarking what is now an uncommon and
> non-default scenario.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---

Hmph, how costly are these tests to run and maintain?

I somehow have a feeling that removal of these "performance" tests
is less worrysome than removing correctness tests, but as long as we
claim to support both configurations (i.e. with and without lookup
tables), it feels a bit premature to remove tests for one of them.
Junio C Hamano April 18, 2025, 4:24 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> I somehow have a feeling that removal of these "performance" tests
> is less worrysome than removing correctness tests, but as long as we
> claim to support both configurations (i.e. with and without lookup
> tables), it feels a bit premature to remove tests for one of them.

In case the implication was missed, I was hinting that in the longer
term, once one variant proves to be better than the other variant(s)
in any and all aspects, it would be a great move to remove the other
one(s).  It is exactly what is happening on the recursive-ort front.

Once we become so confident about correctness and performance with
the configuration with lookup tables that we are willing to lose an
escape hatch to operate without them, we can obviously remove these
tests for configuration without lookup tables.  If we are not there
yet, and still rely on the "escape hatch" value of the configuration
that does not use the lookup tables, we want to make sure that the
escape hatch still functions, right ;-)?

Thanks.
Jeff King April 18, 2025, 10:02 a.m. UTC | #3
On Thu, Apr 17, 2025 at 09:24:46PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I somehow have a feeling that removal of these "performance" tests
> > is less worrysome than removing correctness tests, but as long as we
> > claim to support both configurations (i.e. with and without lookup
> > tables), it feels a bit premature to remove tests for one of them.
> 
> In case the implication was missed, I was hinting that in the longer
> term, once one variant proves to be better than the other variant(s)
> in any and all aspects, it would be a great move to remove the other
> one(s).  It is exactly what is happening on the recursive-ort front.
> 
> Once we become so confident about correctness and performance with
> the configuration with lookup tables that we are willing to lose an
> escape hatch to operate without them, we can obviously remove these
> tests for configuration without lookup tables.  If we are not there
> yet, and still rely on the "escape hatch" value of the configuration
> that does not use the lookup tables, we want to make sure that the
> escape hatch still functions, right ;-)?

I think the perf tests differ from the correctness tests in that a
single run is not all that interesting. You can see how long something
takes, but that's meaningless without a baseline.

The interesting results come from comparing two versions. So in this
case:

  - running the simplified test script comparing an old version (where
    lookup tables were not the default) with one where they are (i.e.,
    one with the first patch from this series). That will show off the
    perf improvement from the lookup tables (and in a better way than
    the original, because we'll actually compute the time difference
    between the two versions, rather than showing them as separate lines
    which the perf suite does not realize are related).

  - going forward, comparing two "new" versions will show us if the
    operations regress in performance, using config both from Git's
    defaults but also the repo.

    So most of the time, you'd be testing the default case, where we do
    generate the lookup tables (because they're now the default). But if
    you have a particular repo or config setup you care about, you can
    provide a repo with pack.writeBitmapLookupTable set as you like.

That does create a blind spot if no developers running the perf suite
ever do the "you can provide..." step. But I think that is the tip of
the iceberg in terms of repo and config blind spots in the perf suite.
There are so many possible combinations, and it's expensive to test them
all. I don't think we have any particular reason to think that the
non-lookup-table code path would significantly regress in performance.

You asked earlier how much these tests cost to run. It's basically
doubling the cost of each script, since we're running everything twice.
So p5310 using linux.git, for instance, just took ~10 minutes after this
patch. And that's with PERF_REPEAT_COUNT set to 1! The default would
have been 30 minutes (and thus prior to this patch, 60 minutes total).

That's a lot of minutes.

-Peff
diff mbox series

Patch

diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index b1399f1007..52f9fca14b 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -4,37 +4,28 @@  test_description='Tests pack performance using bitmaps'
 . ./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_expect_success 'start the test from scratch' '
+	rm -rf * .git
+'
 
-	test_perf_large_repo
+test_perf_large_repo
 
-	# note that we do everything through config,
-	# since we want to be able to compare bitmap-aware
-	# git versus non-bitmap git
-	#
-	# We intentionally use the deprecated pack.writebitmaps
-	# config so that we can test against older versions of git.
-	test_expect_success 'setup bitmap config' '
-		git config pack.writebitmaps true
-	'
+# note that we do everything through config,
+# since we want to be able to compare bitmap-aware
+# git versus non-bitmap git
+#
+# We intentionally use the deprecated pack.writebitmaps
+# config so that we can test against older versions of git.
+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
-	'
+# 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_pack_bitmap
 
 test_done
diff --git a/t/perf/p5311-pack-bitmaps-fetch.sh b/t/perf/p5311-pack-bitmaps-fetch.sh
index 047efb995d..75b05a600e 100755
--- a/t/perf/p5311-pack-bitmaps-fetch.sh
+++ b/t/perf/p5311-pack-bitmaps-fetch.sh
@@ -3,52 +3,46 @@ 
 test_description='performance of fetches from bitmapped packs'
 . ./perf-lib.sh
 
-test_fetch_bitmaps () {
-	test_expect_success 'setup test directory' '
-		rm -fr * .git
-	'
-
-	test_perf_default_repo
+test_expect_success 'setup test directory' '
+	rm -fr * .git
+'
 
-	test_expect_success 'create bitmapped server repo' '
-		git config pack.writebitmaps true &&
-		git config pack.writeBitmapLookupTable '"$1"' &&
-		git repack -ad
-	'
+test_perf_default_repo
 
-	# simulate a fetch from a repository that last fetched N days ago, for
-	# various values of N. We do so by following the first-parent chain,
-	# and assume the first entry in the chain that is N days older than the current
-	# HEAD is where the HEAD would have been then.
-	for days in 1 2 4 8 16 32 64 128; do
-		title=$(printf '%10s' "($days days)")
-		test_expect_success "setup revs from $days days ago" '
-			now=$(git log -1 --format=%ct HEAD) &&
-			then=$(($now - ($days * 86400))) &&
-			tip=$(git rev-list -1 --first-parent --until=$then HEAD) &&
-			{
-				echo HEAD &&
-				echo ^$tip
-			} >revs
-		'
+test_expect_success 'create bitmapped server repo' '
+	git config pack.writebitmaps true &&
+	git repack -ad
+'
 
-		test_perf "server $title (lookup=$1)" '
-			git pack-objects --stdout --revs \
-					--thin --delta-base-offset \
-					<revs >tmp.pack
-		'
+# simulate a fetch from a repository that last fetched N days ago, for
+# various values of N. We do so by following the first-parent chain,
+# and assume the first entry in the chain that is N days older than the current
+# HEAD is where the HEAD would have been then.
+for days in 1 2 4 8 16 32 64 128; do
+	title=$(printf '%10s' "($days days)")
+	test_expect_success "setup revs from $days days ago" '
+		now=$(git log -1 --format=%ct HEAD) &&
+		then=$(($now - ($days * 86400))) &&
+		tip=$(git rev-list -1 --first-parent --until=$then HEAD) &&
+		{
+			echo HEAD &&
+			echo ^$tip
+		} >revs
+	'
 
-		test_size "size   $title" '
-			test_file_size tmp.pack
-		'
+	test_perf "server $title" '
+		git pack-objects --stdout --revs \
+				--thin --delta-base-offset \
+				<revs >tmp.pack
+	'
 
-		test_perf "client $title (lookup=$1)" '
-			git index-pack --stdin --fix-thin <tmp.pack
-		'
-	done
-}
+	test_size "size   $title" '
+		test_file_size tmp.pack
+	'
 
-test_fetch_bitmaps true
-test_fetch_bitmaps false
+	test_perf "client $title" '
+		git index-pack --stdin --fix-thin <tmp.pack
+	'
+done
 
 test_done
diff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh
index d082e6cacb..9f32582dec 100755
--- a/t/perf/p5326-multi-pack-bitmaps.sh
+++ b/t/perf/p5326-multi-pack-bitmaps.sh
@@ -4,64 +4,53 @@  test_description='Tests performance using midx bitmaps'
 . ./perf-lib.sh
 . "${TEST_DIRECTORY}/perf/lib-bitmap.sh"
 
-test_bitmap () {
-	local enabled="$1"
-
-	test_expect_success "remove existing repo (lookup=$enabled)" '
-		rm -fr * .git
-	'
-
-	test_perf_large_repo
-
-	# 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_expect_success "use lookup table: $enabled" '
-		git config pack.writeBitmapLookupTable '"$enabled"'
-	'
-
-	test_expect_success "start with bitmapped pack (lookup=$enabled)" '
-		git repack -adb
-	'
-
-	test_perf "setup multi-pack index (lookup=$enabled)" '
-		git multi-pack-index write --bitmap
-	'
-
-	test_expect_success "drop pack bitmap (lookup=$enabled)" '
-		rm -f .git/objects/pack/pack-*.bitmap
-	'
-
-	test_full_bitmap
-
-	test_expect_success "create partial bitmap state (lookup=$enabled)" '
-		# pick a commit to represent the repo tip in the past
-		cutoff=$(git rev-list HEAD~100 -1) &&
-		orig_tip=$(git rev-parse HEAD) &&
-
-		# now pretend we have just one tip
-		rm -rf .git/logs .git/refs/* .git/packed-refs &&
-		git update-ref HEAD $cutoff &&
-
-		# and then repack, which will leave us with a nice
-		# big bitmap pack of the "old" history, and all of
-		# the new history will be loose, as if it had been pushed
-		# up incrementally and exploded via unpack-objects
-		git repack -Ad &&
-		git multi-pack-index write --bitmap &&
-
-		# and now restore our original tip, as if the pushes
-		# had happened
-		git update-ref HEAD $orig_tip
-	'
-
-	test_partial_bitmap
-}
-
-test_bitmap false
-test_bitmap true
+test_expect_success "remove existing repo" '
+	rm -fr * .git
+'
+
+test_perf_large_repo
+
+# 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_expect_success "start with bitmapped pack" '
+	git repack -adb
+'
+
+test_perf "setup multi-pack index" '
+	git multi-pack-index write --bitmap
+'
+
+test_expect_success "drop pack bitmap" '
+	rm -f .git/objects/pack/pack-*.bitmap
+'
+
+test_full_bitmap
+
+test_expect_success "create partial bitmap state" '
+	# pick a commit to represent the repo tip in the past
+	cutoff=$(git rev-list HEAD~100 -1) &&
+	orig_tip=$(git rev-parse HEAD) &&
+
+	# now pretend we have just one tip
+	rm -rf .git/logs .git/refs/* .git/packed-refs &&
+	git update-ref HEAD $cutoff &&
+
+	# and then repack, which will leave us with a nice
+	# big bitmap pack of the "old" history, and all of
+	# the new history will be loose, as if it had been pushed
+	# up incrementally and exploded via unpack-objects
+	git repack -Ad &&
+	git multi-pack-index write --bitmap &&
+
+	# and now restore our original tip, as if the pushes
+	# had happened
+	git update-ref HEAD $orig_tip
+'
+
+test_partial_bitmap
 
 test_done
diff --git a/t/perf/p5333-pseudo-merge-bitmaps.sh b/t/perf/p5333-pseudo-merge-bitmaps.sh
index 2e8b1d2635..91b1c06745 100755
--- a/t/perf/p5333-pseudo-merge-bitmaps.sh
+++ b/t/perf/p5333-pseudo-merge-bitmaps.sh
@@ -11,7 +11,6 @@  test_expect_success 'setup' '
 		-c bitmapPseudoMerge.all.threshold=now \
 		-c bitmapPseudoMerge.all.stableThreshold=never \
 		-c bitmapPseudoMerge.all.maxMerges=64 \
-		-c pack.writeBitmapLookupTable=true \
 		repack -adb
 '