{"thread":{"id":"63307","subject":"[PATCH 0/4] pack-bitmap: enable lookup tables by default, misc. cleanups","startedAt":"2025-04-17T21:12:13Z","lastAt":"2025-05-05T07:11:17Z","messageCount":18,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"516350","messageId":"cover.1744924321.git.me@ttaylorr.com","threadId":"63307","inReplyTo":null,"subject":"[PATCH 0/4] pack-bitmap: enable lookup tables by default, misc. cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-17T21:12:10Z","receivedAt":"2025-04-17T21:12:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a short series I extracted from a larger topic on reusing\n\"external\"[^1] deltas during verbatim pack reuse.\n\nAs part of performance-testing that series, I realized that bitmap\nlookup tables are not written by default. Since it has been a\nsignificant period of time since their introduction, the first patch of\nthis series makes writing the lookup table extension the default\nbehavior. This is:\n\n * pack-bitmap: write lookup table extension by default\n\nThe next three patches clean up some t/perf scripts that were redundant\nnow that lookup tables are the default behavior. Those are:\n\n * p5312: removed duplicate performance test script\n * t/perf: avoid testing bitmaps without lookup table\n * t/perf/lib-bitmap.sh: avoid test_perf during setup\n\nThanks in advance for your review :-).\n\n[^1]: The term I'm using to describe delta/base pairs which either (a)\nare represented from different packs in a MIDX bitmap, or (b) the client\nis known to already have the base.\n\nTaylor Blau (4):\n pack-bitmap: write lookup table extension by default\n p5312: removed duplicate performance test script\n t/perf: avoid testing bitmaps without lookup table\n t/perf/lib-bitmap.sh: avoid test_perf during setup\n\n Documentation/config/pack.adoc | 2 +-\n builtin/multi-pack-index.c | 1 +\n builtin/pack-objects.c | 2 +-\n t/perf/lib-bitmap.sh | 2 +-\n t/perf/p5310-pack-bitmaps.sh | 47 +++++-------\n t/perf/p5311-pack-bitmaps-fetch.sh | 76 +++++++++----------\n t/perf/p5312-pack-bitmaps-revs.sh | 34 ---------\n t/perf/p5326-multi-pack-bitmaps.sh | 107 ++++++++++++---------------\n t/perf/p5333-pseudo-merge-bitmaps.sh | 1 -\n 9 files changed, 106 insertions(+), 166 deletions(-)\n delete mode 100755 t/perf/p5312-pack-bitmaps-revs.sh\n\n\nbase-commit: c152ae3ef50dc7bbbf5089571df5bba404a96e0d\n-- \n2.49.0.226.g0e6cae136d\n"},{"id":"516351","messageId":"b7cfb1267fdd7f50f414c9f79377cb338a0c1ab0.1744924321.git.me@ttaylorr.com","threadId":"63307","inReplyTo":"cover.1744924321.git.me@ttaylorr.com","subject":"[PATCH 1/4] pack-bitmap: write lookup table extension by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-17T21:12:14Z","receivedAt":"2025-04-17T21:12:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The lookup table extension for reachability bitmaps was first introduced\nvia 3fe0121479 (Merge branch 'ac/bitmap-lookup-table', 2022-09-05).\nFor each bitmapped commit, the lookup table encodes three unsigned\nintegers:\n\n - The pack position of the commit.\n - An offset within the *.bitmap itself where that commit's bitmap\n lives.\n - An index into the same table where information on that commit's XOR\n pair can be found.\n\nLookup tables make bitmap operations faster because they no longer have\nto read the entire *.bitmap file to discover what commits have\ncorresponding reachability bitmaps.\n\nWhen bitmap lookup tables were first introduced, we established a\nbaseline level of performance in p5310 with and without lookup tables.\nHere is the baseline without:\n\n Test this tree\n -----------------------------------------------------------------------\n 5310.6: simulated clone 14.04(5.78+1.79)\n 5310.7: simulated fetch 1.95(3.05+0.20)\n 5310.8: pack to file (bitmap) 44.73(20.55+7.45)\n 5310.9: rev-list (commits) 0.78(0.46+0.10)\n 5310.10: rev-list (objects) 4.07(3.97+0.08)\n 5310.11: rev-list with tag negated via --not 0.06(0.02+0.03)\n --all (objects)\n 5310.12: rev-list with negative tag (objects) 0.21(0.15+0.05)\n 5310.13: rev-list count with blob:none 0.24(0.17+0.06)\n 5310.14: rev-list count with blob:limit=1k 7.07(5.92+0.48)\n 5310.15: rev-list count with tree:0 0.25(0.17+0.07)\n 5310.16: simulated partial clone 5.67(3.28+0.64)\n 5310.18: clone (partial bitmap) 16.05(8.34+1.86)\n 5310.19: pack to file (partial bitmap) 59.76(27.22+7.43)\n 5310.20: rev-list with tree filter (partial bitmap) 0.90(0.18+0.16)\n\n, and here is the same set of tests, this time with the lookup table\nenabled:\n\n Test this tree\n -----------------------------------------------------------------------\n 5310.26: simulated clone 13.69(5.72+1.78)\n 5310.27: simulated fetch 1.84(3.02+0.16)\n 5310.28: pack to file (bitmap) 45.63(20.67+7.50)\n 5310.29: rev-list (commits) 0.56(0.39+0.8)\n 5310.30: rev-list (objects) 3.77(3.74+0.08)\n 5310.31: rev-list with tag negated via --not 0.05(0.02+0.03)\n --all (objects)\n 5310.32: rev-list with negative tag (objects) 0.21(0.15+0.05)\n 5310.33: rev-list count with blob:none 0.23(0.17+0.05)\n 5310.34: rev-list count with blob:limit=1k 6.65(5.72+0.40)\n 5310.35: rev-list count with tree:0 0.23(0.16+0.06)\n 5310.36: simulated partial clone 5.57(3.26+0.59)\n 5310.38: clone (partial bitmap) 15.89(8.39+1.84)\n 5310.39: pack to file (partial bitmap) 58.32(27.55+7.47)\n 5310.40: rev-list with tree filter (partial bitmap) 0.73(0.18+0.15)\n\n(All numbers here come from a ~2022-era copy of the kernel, via\nAbhradeep Chakraborty who implemented the lookup table extension).\n\nIn the almost three years since lookup tables were introduced, GitHub\nhas used them in production without issue, taking advantage of the above\nperformance benefits along the way.\n\nSince this feature has had sufficient time to flush out any bugs and/or\nperformance regressions, let's enable it by default so that all bitmap\nusers can reap similar performance benefits.\n\n[1]: https://lore.kernel.org/git/pull.1266.git.1655728395.gitgitgadget@gmail.com/\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.adoc | 2 +-\n builtin/multi-pack-index.c | 1 +\n builtin/pack-objects.c | 2 +-\n 3 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc\nindex da527377fa..ba538e3d9c 100644\n--- a/Documentation/config/pack.adoc\n+++ b/Documentation/config/pack.adoc\n@@ -191,7 +191,7 @@ pack.writeBitmapLookupTable::\n \tbitmap index (if one is written). This table is used to defer\n \tloading individual bitmaps as late as possible. This can be\n \tbeneficial in repositories that have relatively large bitmap\n-\tindexes. Defaults to false.\n+\tindexes. Defaults to true.\n \n pack.readReverseIndex::\n \tWhen true, git will read any .rev file(s) that may be available\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 2a938466f5..6ad6f814e3 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -142,6 +142,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n \tint ret;\n \n \topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\topts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;\n \n \tgit_config(git_multi_pack_index_write_config, NULL);\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3973267e9e..384fefbb1d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -239,7 +239,7 @@ static enum {\n \tWRITE_BITMAP_QUIET,\n \tWRITE_BITMAP_TRUE,\n } write_bitmap_index;\n-static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n+static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE | BITMAP_OPT_LOOKUP_TABLE;\n \n static int exclude_promisor_objects;\n static int exclude_promisor_objects_best_effort;\n-- \n2.49.0.226.g0e6cae136d\n\n"},{"id":"516352","messageId":"51c4604e16c886d888138f2b513e4d3407b10728.1744924321.git.me@ttaylorr.com","threadId":"63307","inReplyTo":"cover.1744924321.git.me@ttaylorr.com","subject":"[PATCH 2/4] p5312: removed duplicate performance test script","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-17T21:12:17Z","receivedAt":"2025-04-17T21:12:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When the reachability bitmap format learned to read and write a lookup\ntable containing the set of commits which received reachability bitmaps,\ncommit 761416ef91 (bitmap-lookup-table: add performance tests for lookup\ntable, 2022-08-14) added that mirrored p5310 but with reverse indexes\nenabled.\n\nLater on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by\ndefault, 2023-04-12), we enabled reverse indexes by default, which made\nthese two tests indistinguishable from one another. Commit a8dd7e05b1\nshould have removed p5312 as a duplicate, but didn't do so.\n\nCorrect that by removing p5312 as a functional duplicate of p5310.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5312-pack-bitmaps-revs.sh | 34 -------------------------------\n 1 file changed, 34 deletions(-)\n delete mode 100755 t/perf/p5312-pack-bitmaps-revs.sh\n\ndiff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh\ndeleted file mode 100755\nindex ceec60656b..0000000000\n--- a/t/perf/p5312-pack-bitmaps-revs.sh\n+++ /dev/null\n@@ -1,34 +0,0 @@\n-#!/bin/sh\n-\n-test_description='Tests pack performance using bitmaps (rev index enabled)'\n-. ./perf-lib.sh\n-. \"${TEST_DIRECTORY}/perf/lib-bitmap.sh\"\n-\n-test_lookup_pack_bitmap () {\n-\ttest_expect_success 'start the test from scratch' '\n-\t\trm -rf * .git\n-\t'\n-\n-\ttest_perf_large_repo\n-\n-\ttest_expect_success 'setup bitmap config' '\n-\t\tgit config pack.writebitmaps true\n-\t'\n-\n-\t# we need to create the tag up front such that it is covered by the repack and\n-\t# thus by generated bitmaps.\n-\ttest_expect_success 'create tags' '\n-\t\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n-\t'\n-\n-\ttest_perf \"enable lookup table: $1\" '\n-\t\tgit config pack.writeBitmapLookupTable '\"$1\"'\n-\t'\n-\n-\ttest_pack_bitmap\n-}\n-\n-test_lookup_pack_bitmap false\n-test_lookup_pack_bitmap true\n-\n-test_done\n-- \n2.49.0.226.g0e6cae136d\n\n"},{"id":"516353","messageId":"8cc5952e594b78ffb2ba4bcaabd62a8e5b8fe72a.1744924321.git.me@ttaylorr.com","threadId":"63307","inReplyTo":"cover.1744924321.git.me@ttaylorr.com","subject":"[PATCH 3/4] t/perf: avoid testing bitmaps without lookup table","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-17T21:12:20Z","receivedAt":"2025-04-17T21:12:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a previous commit, the setting which controls whether or not the\npack- and MIDX-bitmap machinery writes a lookup table,\n'pack.writeBitmapLookupTable' was enabled by default.\n\nAs a result, we can clean up many of our bitmap-related performance\ntests. Many of the relevant performance tests look something like:\n\n test_it () {\n test_expect_success 'setup pack.writeBitmapLookupTable' '\n git config pack.writeBitmapLookupTable '\"$1\"'\n '\n\n # ...\n }\n\n test_it true\n test_it false\n\n, where the two invocations of 'test_it' run the tests with and without\nbitmap lookup tables enabled.\n\nBut now that lookup tables are enabled by default and have proven to be\na performance win, let's avoid benchmarking what is now an uncommon and\nnon-default scenario.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5310-pack-bitmaps.sh | 47 +++++-------\n t/perf/p5311-pack-bitmaps-fetch.sh | 76 +++++++++----------\n t/perf/p5326-multi-pack-bitmaps.sh | 107 ++++++++++++---------------\n t/perf/p5333-pseudo-merge-bitmaps.sh | 1 -\n 4 files changed, 102 insertions(+), 129 deletions(-)\n\ndiff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh\nindex b1399f1007..52f9fca14b 100755\n--- a/t/perf/p5310-pack-bitmaps.sh\n+++ b/t/perf/p5310-pack-bitmaps.sh\n@@ -4,37 +4,28 @@ test_description='Tests pack performance using bitmaps'\n . ./perf-lib.sh\n . \"${TEST_DIRECTORY}/perf/lib-bitmap.sh\"\n \n-test_lookup_pack_bitmap () {\n-\ttest_expect_success 'start the test from scratch' '\n-\t\trm -rf * .git\n-\t'\n+test_expect_success 'start the test from scratch' '\n+\trm -rf * .git\n+'\n \n-\ttest_perf_large_repo\n+test_perf_large_repo\n \n-\t# note that we do everything through config,\n-\t# since we want to be able to compare bitmap-aware\n-\t# git versus non-bitmap git\n-\t#\n-\t# We intentionally use the deprecated pack.writebitmaps\n-\t# config so that we can test against older versions of git.\n-\ttest_expect_success 'setup bitmap config' '\n-\t\tgit config pack.writebitmaps true\n-\t'\n+# note that we do everything through config,\n+# since we want to be able to compare bitmap-aware\n+# git versus non-bitmap git\n+#\n+# We intentionally use the deprecated pack.writebitmaps\n+# config so that we can test against older versions of git.\n+test_expect_success 'setup bitmap config' '\n+\tgit config pack.writebitmaps true\n+'\n \n-\t# we need to create the tag up front such that it is covered by the repack and\n-\t# thus by generated bitmaps.\n-\ttest_expect_success 'create tags' '\n-\t\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n-\t'\n+# we need to create the tag up front such that it is covered by the repack and\n+# thus by generated bitmaps.\n+test_expect_success 'create tags' '\n+\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n+'\n \n-\ttest_perf \"enable lookup table: $1\" '\n-\t\tgit config pack.writeBitmapLookupTable '\"$1\"'\n-\t'\n-\n-\ttest_pack_bitmap\n-}\n-\n-test_lookup_pack_bitmap false\n-test_lookup_pack_bitmap true\n+test_pack_bitmap\n \n test_done\ndiff --git a/t/perf/p5311-pack-bitmaps-fetch.sh b/t/perf/p5311-pack-bitmaps-fetch.sh\nindex 047efb995d..75b05a600e 100755\n--- a/t/perf/p5311-pack-bitmaps-fetch.sh\n+++ b/t/perf/p5311-pack-bitmaps-fetch.sh\n@@ -3,52 +3,46 @@\n test_description='performance of fetches from bitmapped packs'\n . ./perf-lib.sh\n \n-test_fetch_bitmaps () {\n-\ttest_expect_success 'setup test directory' '\n-\t\trm -fr * .git\n-\t'\n-\n-\ttest_perf_default_repo\n+test_expect_success 'setup test directory' '\n+\trm -fr * .git\n+'\n \n-\ttest_expect_success 'create bitmapped server repo' '\n-\t\tgit config pack.writebitmaps true &&\n-\t\tgit config pack.writeBitmapLookupTable '\"$1\"' &&\n-\t\tgit repack -ad\n-\t'\n+test_perf_default_repo\n \n-\t# simulate a fetch from a repository that last fetched N days ago, for\n-\t# various values of N. We do so by following the first-parent chain,\n-\t# and assume the first entry in the chain that is N days older than the current\n-\t# HEAD is where the HEAD would have been then.\n-\tfor days in 1 2 4 8 16 32 64 128; do\n-\t\ttitle=$(printf '%10s' \"($days days)\")\n-\t\ttest_expect_success \"setup revs from $days days ago\" '\n-\t\t\tnow=$(git log -1 --format=%ct HEAD) &&\n-\t\t\tthen=$(($now - ($days * 86400))) &&\n-\t\t\ttip=$(git rev-list -1 --first-parent --until=$then HEAD) &&\n-\t\t\t{\n-\t\t\t\techo HEAD &&\n-\t\t\t\techo ^$tip\n-\t\t\t} >revs\n-\t\t'\n+test_expect_success 'create bitmapped server repo' '\n+\tgit config pack.writebitmaps true &&\n+\tgit repack -ad\n+'\n \n-\t\ttest_perf \"server $title (lookup=$1)\" '\n-\t\t\tgit pack-objects --stdout --revs \\\n-\t\t\t\t\t--thin --delta-base-offset \\\n-\t\t\t\t\ttmp.pack\n-\t\t'\n+# simulate a fetch from a repository that last fetched N days ago, for\n+# various values of N. We do so by following the first-parent chain,\n+# and assume the first entry in the chain that is N days older than the current\n+# HEAD is where the HEAD would have been then.\n+for days in 1 2 4 8 16 32 64 128; do\n+\ttitle=$(printf '%10s' \"($days days)\")\n+\ttest_expect_success \"setup revs from $days days ago\" '\n+\t\tnow=$(git log -1 --format=%ct HEAD) &&\n+\t\tthen=$(($now - ($days * 86400))) &&\n+\t\ttip=$(git rev-list -1 --first-parent --until=$then HEAD) &&\n+\t\t{\n+\t\t\techo HEAD &&\n+\t\t\techo ^$tip\n+\t\t} >revs\n+\t'\n \n-\t\ttest_size \"size $title\" '\n-\t\t\ttest_file_size tmp.pack\n-\t\t'\n+\ttest_perf \"server $title\" '\n+\t\tgit pack-objects --stdout --revs \\\n+\t\t\t\t--thin --delta-base-offset \\\n+\t\t\t\ttmp.pack\n+\t'\n \n-\t\ttest_perf \"client $title (lookup=$1)\" '\n-\t\t\tgit index-pack --stdin --fix-thin \n---\n t/perf/lib-bitmap.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/lib-bitmap.sh b/t/perf/lib-bitmap.sh\nindex 55a8feb1dc..fdf5f35f1b 100644\n--- a/t/perf/lib-bitmap.sh\n+++ b/t/perf/lib-bitmap.sh\n@@ -69,7 +69,7 @@ test_partial_bitmap () {\n }\n \n test_pack_bitmap () {\n-\ttest_perf \"repack to disk\" '\n+\ttest_expect_success \"repack to disk\" '\n \t\tgit repack -ad\n \t'\n \n-- \n2.49.0.226.g0e6cae136d\n"},{"id":"516355","messageId":"xmqqmscee97z.fsf@gitster.g","threadId":"63307","inReplyTo":"b7cfb1267fdd7f50f414c9f79377cb338a0c1ab0.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 1/4] pack-bitmap: write lookup table extension by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-17T22:04:00Z","receivedAt":"2025-04-17T22:04:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 2a938466f5..6ad6f814e3 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -142,6 +142,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n> \tint ret;\n> \n> \topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\topts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;\n> \n> \tgit_config(git_multi_pack_index_write_config, NULL);\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 3973267e9e..384fefbb1d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -239,7 +239,7 @@ static enum {\n> \tWRITE_BITMAP_QUIET,\n> \tWRITE_BITMAP_TRUE,\n> } write_bitmap_index;\n> -static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n> +static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE | BITMAP_OPT_LOOKUP_TABLE;\n\nAre these two hunks required to be kept in sync?\n\nIf so, I am wondering what is the right approach to make sure they\nare. The definition of MIDX_WRITE_BITMAP_* flag bits in midx.h and\nBITMAP_OPT_* flag bits in write_bitmap_index enum are distinct two\nsets, and we need a way to somehow convert between them back and\nforth if we really wanted to ensure that these \"default\" values are\nkept in sync automatically.\n\nThe reason I ask is mostly because I do not know offhand, and I\nwould imagine that it would be hard for third-parties to verify, if\nthese are only two places that need to be updated in order for\nlookup table extensions to be written by default, when somebody\nnew wants to further update the default.\n\n"},{"id":"516357","messageId":"xmqqh62me8zo.fsf@gitster.g","threadId":"63307","inReplyTo":"51c4604e16c886d888138f2b513e4d3407b10728.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] p5312: removed duplicate performance test script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-17T22:08:59Z","receivedAt":"2025-04-17T22:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Subject: Re: [PATCH 2/4] p5312: removed duplicate performance test script\n\n\"removed\" -> \"remove\"???\n\n> When the reachability bitmap format learned to read and write a lookup\n> table containing the set of commits which received reachability bitmaps,\n> commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup\n> table, 2022-08-14) added that mirrored p5310 but with reverse indexes\n> enabled.\n\n\"added that\" -> \"added a that\"???\n\n> Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by\n> default, 2023-04-12), we enabled reverse indexes by default, which made\n> these two tests indistinguishable from one another. Commit a8dd7e05b1\n> should have removed p5312 as a duplicate, but didn't do so.\n\nOr to retain the same coverage, it should have explicitly disabled\nreverse index in one of the tests, while allowing the other to use\nthe reverse index enabled by default, perhaps?\n\n> Correct that by removing p5312 as a functional duplicate of p5310.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> t/perf/p5312-pack-bitmaps-revs.sh | 34 -------------------------------\n> 1 file changed, 34 deletions(-)\n> delete mode 100755 t/perf/p5312-pack-bitmaps-revs.sh\n>\n> diff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh\n> deleted file mode 100755\n> index ceec60656b..0000000000\n> --- a/t/perf/p5312-pack-bitmaps-revs.sh\n> +++ /dev/null\n> @@ -1,34 +0,0 @@\n> -#!/bin/sh\n> -\n> -test_description='Tests pack performance using bitmaps (rev index enabled)'\n> -. ./perf-lib.sh\n> -. \"${TEST_DIRECTORY}/perf/lib-bitmap.sh\"\n> -\n> -test_lookup_pack_bitmap () {\n> -\ttest_expect_success 'start the test from scratch' '\n> -\t\trm -rf * .git\n> -\t'\n> -\n> -\ttest_perf_large_repo\n> -\n> -\ttest_expect_success 'setup bitmap config' '\n> -\t\tgit config pack.writebitmaps true\n> -\t'\n> -\n> -\t# we need to create the tag up front such that it is covered by the repack and\n> -\t# thus by generated bitmaps.\n> -\ttest_expect_success 'create tags' '\n> -\t\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n> -\t'\n> -\n> -\ttest_perf \"enable lookup table: $1\" '\n> -\t\tgit config pack.writeBitmapLookupTable '\"$1\"'\n> -\t'\n> -\n> -\ttest_pack_bitmap\n> -}\n> -\n> -test_lookup_pack_bitmap false\n> -test_lookup_pack_bitmap true\n> -\n> -test_done\n"},{"id":"516358","messageId":"xmqqcydae8fg.fsf@gitster.g","threadId":"63307","inReplyTo":"8cc5952e594b78ffb2ba4bcaabd62a8e5b8fe72a.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] t/perf: avoid testing bitmaps without lookup table","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-17T22:21:07Z","receivedAt":"2025-04-17T22:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> In a previous commit, the setting which controls whether or not the\n> pack- and MIDX-bitmap machinery writes a lookup table,\n> 'pack.writeBitmapLookupTable' was enabled by default.\n>\n> As a result, we can clean up many of our bitmap-related performance\n> tests. Many of the relevant performance tests look something like:\n>\n> test_it () {\n> test_expect_success 'setup pack.writeBitmapLookupTable' '\n> git config pack.writeBitmapLookupTable '\"$1\"'\n> '\n>\n> # ...\n> }\n>\n> test_it true\n> test_it false\n>\n> , where the two invocations of 'test_it' run the tests with and without\n> bitmap lookup tables enabled.\n>\n> But now that lookup tables are enabled by default and have proven to be\n> a performance win, let's avoid benchmarking what is now an uncommon and\n> non-default scenario.\n>\n> Signed-off-by: Taylor Blau \n> ---\n\nHmph, how costly are these tests to run and maintain?\n\nI somehow have a feeling that removal of these \"performance\" tests\nis less worrysome than removing correctness tests, but as long as we\nclaim to support both configurations (i.e. with and without lookup\ntables), it feels a bit premature to remove tests for one of them.\n\n"},{"id":"516359","messageId":"xmqq8qnye8dp.fsf@gitster.g","threadId":"63307","inReplyTo":"0906e14c0e55b52573c7e0b632c7c639850700ec.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] t/perf/lib-bitmap.sh: avoid test_perf during setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-17T22:22:10Z","receivedAt":"2025-04-17T22:22:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> In the test_pack_bitmap() helper function, we first repack the\n> repository under test for consistency and to eliminate any effects from\n> different distributions of objects among packs.\n>\n> This step is performed with test_perf, so it is repeated\n> $GIT_PERF_REPEAT_COUNT number of times. But we do not care about timing\n> this portion of the setup phase, and repeating the process does not\n> change the outcome.\n>\n> Use test_expect_success to avoid spending time repeating an idempotent\n> portion of the setup for performance tests that use test_pack_bitmap().\n>\n> Signed-off-by: Taylor Blau \n> ---\n> t/perf/lib-bitmap.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nOK.\n\n>\n> diff --git a/t/perf/lib-bitmap.sh b/t/perf/lib-bitmap.sh\n> index 55a8feb1dc..fdf5f35f1b 100644\n> --- a/t/perf/lib-bitmap.sh\n> +++ b/t/perf/lib-bitmap.sh\n> @@ -69,7 +69,7 @@ test_partial_bitmap () {\n> }\n> \n> test_pack_bitmap () {\n> -\ttest_perf \"repack to disk\" '\n> +\ttest_expect_success \"repack to disk\" '\n> \t\tgit repack -ad\n> \t'\n"},{"id":"516363","messageId":"xmqqzfgeaygh.fsf@gitster.g","threadId":"63307","inReplyTo":"xmqqcydae8fg.fsf@gitster.g","subject":"Re: [PATCH 3/4] t/perf: avoid testing bitmaps without lookup table","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-18T04:24:46Z","receivedAt":"2025-04-18T04:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I somehow have a feeling that removal of these \"performance\" tests\n> is less worrysome than removing correctness tests, but as long as we\n> claim to support both configurations (i.e. with and without lookup\n> tables), it feels a bit premature to remove tests for one of them.\n\nIn case the implication was missed, I was hinting that in the longer\nterm, once one variant proves to be better than the other variant(s)\nin any and all aspects, it would be a great move to remove the other\none(s). It is exactly what is happening on the recursive-ort front.\n\nOnce we become so confident about correctness and performance with\nthe configuration with lookup tables that we are willing to lose an\nescape hatch to operate without them, we can obviously remove these\ntests for configuration without lookup tables. If we are not there\nyet, and still rely on the \"escape hatch\" value of the configuration\nthat does not use the lookup tables, we want to make sure that the\nescape hatch still functions, right ;-)?\n\nThanks.\n"},{"id":"516365","messageId":"20250418093335.GB10441@coredump.intra.peff.net","threadId":"63307","inReplyTo":"xmqqmscee97z.fsf@gitster.g","subject":"Re: [PATCH 1/4] pack-bitmap: write lookup table extension by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-18T09:33:35Z","receivedAt":"2025-04-18T09:33:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 17, 2025 at 03:04:00PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> > index 2a938466f5..6ad6f814e3 100644\n> > --- a/builtin/multi-pack-index.c\n> > +++ b/builtin/multi-pack-index.c\n> > @@ -142,6 +142,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n> > \tint ret;\n> > \n> > \topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\topts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;\n> > \n> > \tgit_config(git_multi_pack_index_write_config, NULL);\n> > \n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 3973267e9e..384fefbb1d 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -239,7 +239,7 @@ static enum {\n> > \tWRITE_BITMAP_QUIET,\n> > \tWRITE_BITMAP_TRUE,\n> > } write_bitmap_index;\n> > -static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n> > +static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE | BITMAP_OPT_LOOKUP_TABLE;\n> \n> Are these two hunks required to be kept in sync?\n\nThey're not technically required to be in sync. It is OK for the midx\nbitmaps to have different options than the ones we make for packs. And\nin theory they could intentionally diverge, though in practice I don't\nthink we (yet) have any extensions or options that would be more\nappropriate for one or the other.\n\nSo if we did want to join them, I think it would make sense to still be\nable to use different flags for each situation, but initialize them from\na common definition.\n\n> If so, I am wondering what is the right approach to make sure they\n> are. The definition of MIDX_WRITE_BITMAP_* flag bits in midx.h and\n> BITMAP_OPT_* flag bits in write_bitmap_index enum are distinct two\n> sets, and we need a way to somehow convert between them back and\n> forth if we really wanted to ensure that these \"default\" values are\n> kept in sync automatically.\n\nYeah, I think that would be much simpler if the bitmap options were held\nseparately from the other midx flags, and then we could use the same\nvalues for each (and a common \"defaults\" definition). Which is\nconceptually simple, but means we have to plumb a separate variable\nthrough a bunch of intermediate functions. See the (untested) patch\nbelow.\n\nI dunno if it's worth it for something that doesn't really change all\nthat often. OTOH, it does remove the extra layer of translation in\nwrite_midx_bitmap().\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 2a938466f5..fb073a946a 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -10,6 +10,7 @@\n #include \"object-store-ll.h\"\n #include \"replace-object.h\"\n #include \"repository.h\"\n+#include \"pack-bitmap.h\"\n \n #define BUILTIN_MIDX_WRITE_USAGE \\\n \tN_(\"git multi-pack-index [] write [--preferred-pack=]\" \\\n@@ -54,6 +55,7 @@ static struct opts_multi_pack_index {\n \tchar *refs_snapshot;\n \tunsigned long batch_size;\n \tunsigned flags;\n+\tuint16_t bitmap_options;\n \tint stdin_packs;\n } opts;\n \n@@ -89,16 +91,16 @@ static int git_multi_pack_index_write_config(const char *var, const char *value,\n {\n \tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n \t\tif (git_config_bool(var, value))\n-\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\t\topts.bitmap_options |= BITMAP_OPT_HASH_CACHE;\n \t\telse\n-\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\t\topts.bitmap_options &= ~BITMAP_OPT_HASH_CACHE;\n \t}\n \n \tif (!strcmp(var, \"pack.writebitmaplookuptable\")) {\n \t\tif (git_config_bool(var, value))\n-\t\t\topts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;\n+\t\t\topts.bitmap_options |= BITMAP_OPT_LOOKUP_TABLE;\n \t\telse\n-\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_LOOKUP_TABLE;\n+\t\t\topts.bitmap_options &= ~BITMAP_OPT_LOOKUP_TABLE;\n \t}\n \n \t/*\n@@ -141,7 +143,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n \t};\n \tint ret;\n \n-\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\topts.bitmap_options |= BITMAP_OPT_HASH_CACHE;\n \n \tgit_config(git_multi_pack_index_write_config, NULL);\n \n@@ -167,7 +169,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n \n \t\tret = write_midx_file_only(repo, opts.object_dir, &packs,\n \t\t\t\t\t opts.preferred_pack,\n-\t\t\t\t\t opts.refs_snapshot, opts.flags);\n+\t\t\t\t\t opts.refs_snapshot, opts.flags,\n+\t\t\t\t\t opts.bitmap_options);\n \n \t\tstring_list_clear(&packs, 0);\n \t\tfree(opts.refs_snapshot);\n@@ -177,7 +180,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,\n \t}\n \n \tret = write_midx_file(repo, opts.object_dir, opts.preferred_pack,\n-\t\t\t opts.refs_snapshot, opts.flags);\n+\t\t\t opts.refs_snapshot, opts.flags,\n+\t\t\t opts.bitmap_options);\n \n \tfree(opts.refs_snapshot);\n \treturn ret;\n@@ -236,7 +240,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv,\n \n \tFREE_AND_NULL(options);\n \n-\treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n+\treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags,\n+\t\t\t\t opts.bitmap_options);\n }\n \n static int cmd_multi_pack_index_repack(int argc, const char **argv,\n@@ -269,7 +274,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv,\n \tFREE_AND_NULL(options);\n \n \treturn midx_repack(the_repository, opts.object_dir,\n-\t\t\t (size_t)opts.batch_size, opts.flags);\n+\t\t\t (size_t)opts.batch_size, opts.flags,\n+\t\t\t opts.bitmap_options);\n }\n \n int cmd_multi_pack_index(int argc,\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f3330ade7b..69c9a8bc4c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1562,7 +1562,7 @@ int cmd_repack(int argc,\n \t\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL, 0))\n \t\t\tflags |= MIDX_WRITE_INCREMENTAL;\n \t\twrite_midx_file(the_repository, repo_get_object_directory(the_repository),\n-\t\t\t\tNULL, NULL, flags);\n+\t\t\t\tNULL, NULL, flags, 0);\n \t}\n \n cleanup:\ndiff --git a/midx-write.c b/midx-write.c\nindex 48a4dc5e94..5729b989cb 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -841,10 +841,10 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t\t\t struct packing_data *pdata,\n \t\t\t struct commit **commits,\n \t\t\t uint32_t commits_nr,\n-\t\t\t unsigned flags)\n+\t\t\t unsigned flags,\n+\t\t\t uint16_t options)\n {\n \tint ret, i;\n-\tuint16_t options = 0;\n \tstruct bitmap_writer writer;\n \tstruct pack_idx_entry **index;\n \tstruct strbuf bitmap_name = STRBUF_INIT;\n@@ -859,12 +859,6 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t\tget_midx_filename_ext(ctx->repo->hash_algo, &bitmap_name,\n \t\t\t\t object_dir, midx_hash, MIDX_EXT_BITMAP);\n \n-\tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n-\t\toptions |= BITMAP_OPT_HASH_CACHE;\n-\n-\tif (flags & MIDX_WRITE_BITMAP_LOOKUP_TABLE)\n-\t\toptions |= BITMAP_OPT_LOOKUP_TABLE;\n-\n \t/*\n \t * Build the MIDX-order index based on pdata.objects (which is already\n \t * in MIDX order; c.f., 'midx_pack_order_cmp()' for the definition of\n@@ -1070,7 +1064,8 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t struct string_list *packs_to_drop,\n \t\t\t const char *preferred_pack_name,\n \t\t\t const char *refs_snapshot,\n-\t\t\t unsigned flags)\n+\t\t\t unsigned flags,\n+\t\t\t uint16_t bitmap_options)\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n@@ -1433,7 +1428,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (write_midx_bitmap(&ctx, object_dir,\n \t\t\t\t midx_hash, &pdata, commits, commits_nr,\n-\t\t\t\t flags) < 0) {\n+\t\t\t\t flags, bitmap_options) < 0) {\n \t\t\terror(_(\"could not write multi-pack bitmap\"));\n \t\t\tresult = 1;\n \t\t\tclear_packing_data(&pdata);\n@@ -1529,23 +1524,27 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n int write_midx_file(struct repository *r, const char *object_dir,\n \t\t const char *preferred_pack_name,\n-\t\t const char *refs_snapshot, unsigned flags)\n+\t\t const char *refs_snapshot, unsigned flags,\n+\t\t uint16_t bitmap_options)\n {\n \treturn write_midx_internal(r, object_dir, NULL, NULL,\n \t\t\t\t preferred_pack_name, refs_snapshot,\n-\t\t\t\t flags);\n+\t\t\t\t flags, bitmap_options);\n }\n \n int write_midx_file_only(struct repository *r, const char *object_dir,\n \t\t\t struct string_list *packs_to_include,\n \t\t\t const char *preferred_pack_name,\n-\t\t\t const char *refs_snapshot, unsigned flags)\n+\t\t\t const char *refs_snapshot, unsigned flags,\n+\t\t\t uint16_t bitmap_options)\n {\n \treturn write_midx_internal(r, object_dir, packs_to_include, NULL,\n-\t\t\t\t preferred_pack_name, refs_snapshot, flags);\n+\t\t\t\t preferred_pack_name, refs_snapshot, flags,\n+\t\t\t\t bitmap_options);\n }\n \n-int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flags)\n+int expire_midx_packs(struct repository *r, const char *object_dir,\n+\t\t unsigned flags, uint16_t bitmap_options)\n {\n \tuint32_t i, *count, result = 0;\n \tstruct string_list packs_to_drop = STRING_LIST_INIT_DUP;\n@@ -1603,7 +1602,8 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \n \tif (packs_to_drop.nr)\n \t\tresult = write_midx_internal(r, object_dir, NULL,\n-\t\t\t\t\t &packs_to_drop, NULL, NULL, flags);\n+\t\t\t\t\t &packs_to_drop, NULL, NULL, flags,\n+\t\t\t\t\t bitmap_options);\n \n \tstring_list_clear(&packs_to_drop, 0);\n \n@@ -1718,7 +1718,8 @@ static void fill_included_packs_batch(struct repository *r,\n \tfree(pack_info);\n }\n \n-int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)\n+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n+\t\tunsigned flags, uint16_t bitmap_options)\n {\n \tint result = 0;\n \tuint32_t i, packs_to_repack = 0;\n@@ -1801,7 +1802,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t}\n \n \tresult = write_midx_internal(r, object_dir, NULL, NULL, NULL, NULL,\n-\t\t\t\t flags);\n+\t\t\t\t flags, bitmap_options);\n \n cleanup:\n \tfree(include_pack);\ndiff --git a/midx.h b/midx.h\nindex 9d1374cbd5..5a79302a7a 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -81,8 +81,6 @@ struct multi_pack_index {\n #define MIDX_PROGRESS (1 << 0)\n #define MIDX_WRITE_REV_INDEX (1 << 1)\n #define MIDX_WRITE_BITMAP (1 << 2)\n-#define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n-#define MIDX_WRITE_BITMAP_LOOKUP_TABLE (1 << 4)\n #define MIDX_WRITE_INCREMENTAL (1 << 5)\n \n #define MIDX_EXT_REV \"rev\"\n@@ -131,15 +129,16 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n */\n int write_midx_file(struct repository *r, const char *object_dir,\n \t\t const char *preferred_pack_name, const char *refs_snapshot,\n-\t\t unsigned flags);\n+\t\t unsigned flags, uint16_t bitmap_options);\n int write_midx_file_only(struct repository *r, const char *object_dir,\n \t\t\t struct string_list *packs_to_include,\n \t\t\t const char *preferred_pack_name,\n-\t\t\t const char *refs_snapshot, unsigned flags);\n+\t\t\t const char *refs_snapshot, unsigned flags,\n+\t\t\t uint16_t bitmap_options);\n void clear_midx_file(struct repository *r);\n int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags);\n-int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flags);\n-int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags);\n+int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flags, uint16_t bitmap_options);\n+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags, uint16_t bitmap_options);\n \n void close_midx(struct multi_pack_index *m);\n \n"},{"id":"516366","messageId":"20250418100246.GC10441@coredump.intra.peff.net","threadId":"63307","inReplyTo":"xmqqzfgeaygh.fsf@gitster.g","subject":"Re: [PATCH 3/4] t/perf: avoid testing bitmaps without lookup table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-18T10:02:46Z","receivedAt":"2025-04-18T10:02:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 17, 2025 at 09:24:46PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I somehow have a feeling that removal of these \"performance\" tests\n> > is less worrysome than removing correctness tests, but as long as we\n> > claim to support both configurations (i.e. with and without lookup\n> > tables), it feels a bit premature to remove tests for one of them.\n> \n> In case the implication was missed, I was hinting that in the longer\n> term, once one variant proves to be better than the other variant(s)\n> in any and all aspects, it would be a great move to remove the other\n> one(s). It is exactly what is happening on the recursive-ort front.\n> \n> Once we become so confident about correctness and performance with\n> the configuration with lookup tables that we are willing to lose an\n> escape hatch to operate without them, we can obviously remove these\n> tests for configuration without lookup tables. If we are not there\n> yet, and still rely on the \"escape hatch\" value of the configuration\n> that does not use the lookup tables, we want to make sure that the\n> escape hatch still functions, right ;-)?\n\nI think the perf tests differ from the correctness tests in that a\nsingle run is not all that interesting. You can see how long something\ntakes, but that's meaningless without a baseline.\n\nThe interesting results come from comparing two versions. So in this\ncase:\n\n - running the simplified test script comparing an old version (where\n lookup tables were not the default) with one where they are (i.e.,\n one with the first patch from this series). That will show off the\n perf improvement from the lookup tables (and in a better way than\n the original, because we'll actually compute the time difference\n between the two versions, rather than showing them as separate lines\n which the perf suite does not realize are related).\n\n - going forward, comparing two \"new\" versions will show us if the\n operations regress in performance, using config both from Git's\n defaults but also the repo.\n\n So most of the time, you'd be testing the default case, where we do\n generate the lookup tables (because they're now the default). But if\n you have a particular repo or config setup you care about, you can\n provide a repo with pack.writeBitmapLookupTable set as you like.\n\nThat does create a blind spot if no developers running the perf suite\never do the \"you can provide...\" step. But I think that is the tip of\nthe iceberg in terms of repo and config blind spots in the perf suite.\nThere are so many possible combinations, and it's expensive to test them\nall. I don't think we have any particular reason to think that the\nnon-lookup-table code path would significantly regress in performance.\n\nYou asked earlier how much these tests cost to run. It's basically\ndoubling the cost of each script, since we're running everything twice.\nSo p5310 using linux.git, for instance, just took ~10 minutes after this\npatch. And that's with PERF_REPEAT_COUNT set to 1! The default would\nhave been 30 minutes (and thus prior to this patch, 60 minutes total).\n\nThat's a lot of minutes.\n\n-Peff\n"},{"id":"516367","messageId":"20250418101725.GD10441@coredump.intra.peff.net","threadId":"63307","inReplyTo":"0906e14c0e55b52573c7e0b632c7c639850700ec.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] t/perf/lib-bitmap.sh: avoid test_perf during setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-18T10:17:25Z","receivedAt":"2025-04-18T10:17:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 17, 2025 at 05:12:23PM -0400, Taylor Blau wrote:\n\n> In the test_pack_bitmap() helper function, we first repack the\n> repository under test for consistency and to eliminate any effects from\n> different distributions of objects among packs.\n>\n> This step is performed with test_perf, so it is repeated\n> $GIT_PERF_REPEAT_COUNT number of times. But we do not care about timing\n> this portion of the setup phase, and repeating the process does not\n> change the outcome.\n\nIsn't this also where we actually generate the bitmaps? I.e., it is\nwhere we would see a performance regression in the bitmap writing\nprocess (whereas the rest of the script is about the reading side).\n\nThat said, I don't think it's even doing that very well. It is mutating\nthe on-disk state, so the first run will potentially be much slower than\nsubsequent runs (since everything is now in one big pack with bitmaps,\nand we try to reuse deltas and bitmap data as much as possible). And\nsince we take best-of-N, we're basically just measuring those subsequent\nnoop repacks (unless you set the repeat count to 1!).\n\nI think we've run into this before, e.g. in 775c71e16d (p5302: create\nthe repo in each index-pack test, 2019-04-22). And there the solution\nwas to reset the repo state before each timing, assuming it is quick\nenough not to affect the test too much. Our perf suite doesn't provide\nmuch support there (we'd want something like hyperfine's --prepare\noption).\n\nSo I dunno. It is possible for timing this operation to provide some\nvalue, but I don't think the current implementation is doing that. And\nit's quite expensive to run.\n\n> diff --git a/t/perf/lib-bitmap.sh b/t/perf/lib-bitmap.sh\n> index 55a8feb1dc..fdf5f35f1b 100644\n> --- a/t/perf/lib-bitmap.sh\n> +++ b/t/perf/lib-bitmap.sh\n> @@ -69,7 +69,7 @@ test_partial_bitmap () {\n> }\n> \n> test_pack_bitmap () {\n> -\ttest_perf \"repack to disk\" '\n> +\ttest_expect_success \"repack to disk\" '\n> \t\tgit repack -ad\n> \t'\n\nThe same issue exists in t5326, which calls \"multi-pack-index write\"\nwith the \"--bitmap\" flag, I think. So if we are going to do this, we'd\nprobably want the same there.\n\n-Peff\n"},{"id":"516369","messageId":"xmqqr01pbhk2.fsf@gitster.g","threadId":"63307","inReplyTo":"20250418093335.GB10441@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] pack-bitmap: write lookup table extension by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-18T15:44:29Z","receivedAt":"2025-04-18T15:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> They're not technically required to be in sync. It is OK for the midx\n> bitmaps to have different options than the ones we make for packs. And\n> in theory they could intentionally diverge, though in practice I don't\n> think we (yet) have any extensions or options that would be more\n> appropriate for one or the other.\n>\n> So if we did want to join them, I think it would make sense to still be\n> able to use different flags for each situation, but initialize them from\n> a common definition.\n\nThanks for great explanation---I guess it is not worth pursuing,\nthen. It is not like it would make the system misbehave when two\nare set differently.\n"},{"id":"516384","messageId":"aALJoFOUWq0v3fiB@nand.local","threadId":"63307","inReplyTo":"xmqqr01pbhk2.fsf@gitster.g","subject":"Re: [PATCH 1/4] pack-bitmap: write lookup table extension by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-18T21:52:32Z","receivedAt":"2025-04-18T21:52:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 18, 2025 at 08:44:29AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > They're not technically required to be in sync. It is OK for the midx\n> > bitmaps to have different options than the ones we make for packs. And\n> > in theory they could intentionally diverge, though in practice I don't\n> > think we (yet) have any extensions or options that would be more\n> > appropriate for one or the other.\n> >\n> > So if we did want to join them, I think it would make sense to still be\n> > able to use different flags for each situation, but initialize them from\n> > a common definition.\n>\n> Thanks for great explanation---I guess it is not worth pursuing,\n> then. It is not like it would make the system misbehave when two\n> are set differently.\n\nHah, Peff beat me to it. I saw your reply last night and was going to\nwrite you a very similar response.\n\nI think the summary from my perspective would be that: the two could\nfall out-of-sync intentionally if we want the two commands to ever have\ndifferent defaults. Tangentially we could use some common \"bitmap_flags\"\nfield whose bits are defined in pack-bitmap.h and used in both places.\n\nThe latter is a bit awkward currently because the current \"flags\" that\nwe pass into the MIDX machinery from the builtin all have MIDX-specific\nmeanings. So we would have to either make sure that MIDX uses separate\nbit positions (which is awful and far too fragile for my comfort/taste)\nor store them as a separate set of flags (I think what Peff is getting\nat above).\n\nThanks,\nTaylor\n"},{"id":"516386","messageId":"aALK0JlOZDVPUHUR@nand.local","threadId":"63307","inReplyTo":"xmqqh62me8zo.fsf@gitster.g","subject":"Re: [PATCH 2/4] p5312: removed duplicate performance test script","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-18T21:57:36Z","receivedAt":"2025-04-18T21:57:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 17, 2025 at 03:08:59PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Subject: Re: [PATCH 2/4] p5312: removed duplicate performance test script\n>\n> \"removed\" -> \"remove\"???\n>\n> > When the reachability bitmap format learned to read and write a lookup\n> > table containing the set of commits which received reachability bitmaps,\n> > commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup\n> > table, 2022-08-14) added that mirrored p5310 but with reverse indexes\n> > enabled.\n>\n> \"added that\" -> \"added a that\"???\n\nI am embarrassed. These are both awful. Here's the relevant portion of\nthe range-diff:\n\n2: 51c4604e16 ! 2: a80a7b5e60 p5312: removed duplicate performance test script\n @@ Metadata\n Author: Taylor Blau \n\n ## Commit message ##\n - p5312: removed duplicate performance test script\n + p5312: remove duplicate performance test script\n\n When the reachability bitmap format learned to read and write a lookup\n table containing the set of commits which received reachability bitmaps,\n commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup\n - table, 2022-08-14) added that mirrored p5310 but with reverse indexes\n - enabled.\n + table, 2022-08-14) added a new performance test script mirroring p5310\n + but with reverse indexes enabled.\n\n Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by\n default, 2023-04-12), we enabled reverse indexes by default, which made\n\n> > Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by\n> > default, 2023-04-12), we enabled reverse indexes by default, which made\n> > these two tests indistinguishable from one another. Commit a8dd7e05b1\n> > should have removed p5312 as a duplicate, but didn't do so.\n>\n> Or to retain the same coverage, it should have explicitly disabled\n> reverse index in one of the tests, while allowing the other to use\n> the reverse index enabled by default, perhaps?\n\nI don't think we necessarily would benefit from having two variants of\nthis performance test. It is a little annoying to maintain, but that\nisn't the main reason that I proposed removing it here.\n\nI think that the pair of performance tests were useful in proving out\nthe lookup table extension as useful to bitmaps' performance\ncharacteristics by comparison to the non-lookup table version. In that\nsense, I think the pair of performance tests were useful as a contrast\nto one another. Since we have evidence of their usefulness, the contrast\nis less important IMHO.\n\nI think we still want to keep the \"lookup table enabled\" version to\nprevent regressions, though.\n\nThanks,\nTaylor\n"},{"id":"517132","messageId":"xmqqfrhmu2s4.fsf@gitster.g","threadId":"63307","inReplyTo":"cover.1744924321.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] pack-bitmap: enable lookup tables by default, misc. cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-02T21:21:47Z","receivedAt":"2025-05-02T21:21:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> This is a short series I extracted from a larger topic on reusing\n> \"external\"[^1] deltas during verbatim pack reuse.\n>\n> As part of performance-testing that series, I realized that bitmap\n> lookup tables are not written by default. Since it has been a\n> significant period of time since their introduction, the first patch of\n> this series makes writing the lookup table extension the default\n> behavior. This is:\n>\n> * pack-bitmap: write lookup table extension by default\n>\n> The next three patches clean up some t/perf scripts that were redundant\n> now that lookup tables are the default behavior. Those are:\n>\n> * p5312: removed duplicate performance test script\n> * t/perf: avoid testing bitmaps without lookup table\n> * t/perf/lib-bitmap.sh: avoid test_perf during setup\n>\n> Thanks in advance for your review :-).\n>\n> [^1]: The term I'm using to describe delta/base pairs which either (a)\n> are represented from different packs in a MIDX bitmap, or (b) the client\n> is known to already have the base.\n>\n> Taylor Blau (4):\n> pack-bitmap: write lookup table extension by default\n> p5312: removed duplicate performance test script\n> t/perf: avoid testing bitmaps without lookup table\n> t/perf/lib-bitmap.sh: avoid test_perf during setup\n\nPeff and I were the only two people who read these patches?\nIs this topic still viable, or has it been backburnered?\n\nThanks.\n"},{"id":"517210","messageId":"aBhkjGtlA2N7yTVw@pks.im","threadId":"63307","inReplyTo":"xmqqfrhmu2s4.fsf@gitster.g","subject":"Re: [PATCH 0/4] pack-bitmap: enable lookup tables by default, misc. cleanups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-05T07:11:08Z","receivedAt":"2025-05-05T07:11:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 02, 2025 at 02:21:47PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n> \n> > This is a short series I extracted from a larger topic on reusing\n> > \"external\"[^1] deltas during verbatim pack reuse.\n> >\n> > As part of performance-testing that series, I realized that bitmap\n> > lookup tables are not written by default. Since it has been a\n> > significant period of time since their introduction, the first patch of\n> > this series makes writing the lookup table extension the default\n> > behavior. This is:\n> >\n> > * pack-bitmap: write lookup table extension by default\n> >\n> > The next three patches clean up some t/perf scripts that were redundant\n> > now that lookup tables are the default behavior. Those are:\n> >\n> > * p5312: removed duplicate performance test script\n> > * t/perf: avoid testing bitmaps without lookup table\n> > * t/perf/lib-bitmap.sh: avoid test_perf during setup\n> >\n> > Thanks in advance for your review :-).\n> >\n> > [^1]: The term I'm using to describe delta/base pairs which either (a)\n> > are represented from different packs in a MIDX bitmap, or (b) the client\n> > is known to already have the base.\n> >\n> > Taylor Blau (4):\n> > pack-bitmap: write lookup table extension by default\n> > p5312: removed duplicate performance test script\n> > t/perf: avoid testing bitmaps without lookup table\n> > t/perf/lib-bitmap.sh: avoid test_perf during setup\n> \n> Peff and I were the only two people who read these patches?\n> Is this topic still viable, or has it been backburnered?\n\nI read through the patch series, but didn't have anything to add over\nwhat has already been discussed. Overall I think that it makes sense to\nenable lookup tables -- we've had them enabled since February 2023 for\nall users of GitLab.\n\nPatrick\n"}]}