pack-revindex: enable on-disk reverse indexes by default

Here is a (tiny) reroll of my series to enable pack reverse-indexes to be written to disk by default instead of computed on-the-fly in memory.

The original cover letter[1] and commits herein have all of the gore-y details. Not much has changed since last time, except:

- squashing in a patch from Stolee to propagate a struct repository * a little further out from load_pack_revindex()

- a tweak to the linux-TEST-vars CI job to continue running it with GIT_TEST_NO_WRITE_REV_INDEX

- and an additional benchmark to demonstrate the performance of git cat-file --batch-check='%(objectsize:disk)' --batch--all-objects --unordered with and without on-disk reverse indexes

As always, a range-diff is included below for convenience. Thanks in advance for taking (another) look!

[1]: https://lore.kernel.org/git/[email protected]/

Taylor Blau (7):
pack-write.c: plug a leak in stage_tmp_packfiles()
t5325: mark as leak-free
pack-revindex: make load_pack_revindex take a repository
pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK
pack-revindex: introduce pack.readReverseIndex
config: enable pack.writeReverseIndex by default
t: invert GIT_TEST_WRITE_REV_INDEX

 Documentation/config/pack.txt     |  8 +++++++-
 builtin/index-pack.c              |  5 +++--
 builtin/pack-objects.c            |  5 +++--
 ci/run-build-and-tests.sh         |  2 +-
 pack-bitmap.c                     | 23 +++++++++++++----------
 pack-revindex.c                   | 12 +++++++++---
 pack-revindex.h                   |  6 ++++--
 pack-write.c                      |  2 ++
 packfile.c                        |  2 +-
 repo-settings.c                   |  1 +
 repository.h                      |  1 +
 t/README                          |  2 +-
 t/perf/p5312-pack-bitmaps-revs.sh |  3 +--
 t/t5325-reverse-index.sh          | 16 +++++++++++++++-
 14 files changed, 62 insertions(+), 26 deletions(-)

Range-diff against v1:

-:  ----------- > 1:  18be29c3988 pack-write.c: plug a leak in stage_tmp_packfiles()
-:  ----------- > 2:  affb5e2574b t5325: mark as leak-free
1:  be4faf11011 ! 3:  687a9a58924 pack-revindex: make `load_pack_revindex` take a repository

@@ Commit message
to load_pack_revindex, and update all callers to pass the correct instance (in all cases, the_repository).

- Signed-off-by: Taylor Blauy <[email protected]>
+ In certain instances, a new function-local variable is introduced to
+ take the place of a struct repository * argument to the function
+ itself to avoid propagating the new parameter even further throughout
+ the tree.
+
+ Co-authored-by: Derrick Stolee <[email protected]>
+ Signed-off-by: Derrick Stolee <[email protected]>
+ Signed-off-by: Taylor Blau <[email protected]>

## pack-bitmap.c ##
+@@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
+ return 0;
+ }
+
+-static int load_reverse_index(struct bitmap_index *bitmap_git)
++static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_git)
+ {
+ if (bitmap_is_midx(bitmap_git)) {
+ uint32_t i;
@@ pack-bitmap.c: static int load_reverse_index(struct bitmap_index *bitmap_git)
* since we will need to make use of them in pack-objects.
*/
for (i = 0; i < bitmap_git->midx->num_packs; i++) {
- ret = load_pack_revindex(bitmap_git->midx->packs[i]);
-+ ret = load_pack_revindex(the_repository,
-+ bitmap_git->midx->packs[i]);
++ ret = load_pack_revindex(r, bitmap_git->midx->packs[i]);
if (ret)
return ret;
}
return 0;
}
- return load_pack_revindex(bitmap_git->pack);
-+ return load_pack_revindex(the_repository, bitmap_git->pack);
++ return load_pack_revindex(r, bitmap_git->pack);
}

- static int load_bitmap(struct bitmap_index *bitmap_git)
+-static int load_bitmap(struct bitmap_index *bitmap_git)
++static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git)
+ {
+ assert(bitmap_git->map);
+
+ bitmap_git->bitmaps = kh_init_oid_map();
+ bitmap_git->ext_index.positions = kh_init_oid_pos();
+
+- if (load_reverse_index(bitmap_git))
++ if (load_reverse_index(r, bitmap_git))
+ goto failed;
+
+ if (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||
+@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_git(struct repository *r)
+ {
+ struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+- if (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))
++ if (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))
+ return bitmap_git;
+
+ free_bitmap_index(bitmap_git);
+@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_git(struct repository *r) +
+ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
+ {
++ struct repository *r = the_repository;
+ struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+- if (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(bitmap_git))
++ if (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(r, bitmap_git))
+ return bitmap_git;
+
+ free_bitmap_index(bitmap_git);
+@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
+ * from disk. this is the point of no return; after this the rev_list
+ * becomes invalidated and we must perform the revwalk through bitmaps
+ */
+- if (load_bitmap(bitmap_git) < 0)
++ if (load_bitmap(revs->repo, bitmap_git) < 0)
+ goto cleanup;
+
+ object_array_clear(&revs->pending);
+@@ pack-bitmap.c: int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
+ uint32_t *entries,
+ struct bitmap **reuse_out)
+ {
++ struct repository *r = the_repository;
+ struct packed_git *pack;
+ struct bitmap *result = bitmap_git->result;
+ struct bitmap *reuse;
+@@ pack-bitmap.c: int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git, +
+ assert(result);
+
+- load_reverse_index(bitmap_git);
++ load_reverse_index(r, bitmap_git);
+
+ if (bitmap_is_midx(bitmap_git))
+ pack = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];
+@@ pack-bitmap.c: int rebuild_bitmap(const uint32_t *reposition,
+ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
+ struct packing_data *mapping)
+ {
++ struct repository *r = the_repository;
+ uint32_t i, num_objects;
+ uint32_t *reposition;
+
+ if (!bitmap_is_midx(bitmap_git))
+- load_reverse_index(bitmap_git);
++ load_reverse_index(r, bitmap_git);
+ else if (load_midx_revindex(bitmap_git->midx) < 0)
+ BUG("rebuild_existing_bitmaps: missing required rev-cache "
+ "extension");

## pack-revindex.c ##
@@ pack-revindex.c: static int load_pack_revindex_from_disk(struct packed_git *p)

2:  0f368e2347e = 4:  8eec5bacd3a pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK
3:  6f692d470cb ! 5:  a62fc3e4ec1 pack-revindex: introduce `pack.readReverseIndex`

@@ Commit message
'git.compile -c pack.readReverseIndex=false cat-file --batch-check="%(objectsize:disk)" --batch-all-objects' ran
2.06 ± 0.02 times faster than 'git.compile -c pack.readReverseIndex=true cat-file --batch-check="%(objectsize:disk)" --batch-all-objects'

+ Luckily, the results when running git cat-file with --unordered are
+ closer together:
+
+ $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --unordered --batch-check="%(objectsize:disk)" --batch-all-objects'
+ Benchmark 1: git.compile -c pack.readReverseIndex=false cat-file --unordered --batch-check="%(objectsize:disk)" --batch-all-objects
+ Time (mean ± σ): 5.066 s ± 0.105 s [User: 4.792 s, System: 0.274 s]
+ Range (min … max): 4.943 s … 5.220 s 10 runs
+
+ Benchmark 2: git.compile -c pack.readReverseIndex=true cat-file --unordered --batch-check="%(objectsize:disk)" --batch-all-objects
+ Time (mean ± σ): 6.193 s ± 0.069 s [User: 5.937 s, System: 0.255 s]
+ Range (min … max): 6.145 s … 6.356 s 10 runs
+
+ Summary
+ 'git.compile -c pack.readReverseIndex=false cat-file --unordered --batch-check="%(objectsize:disk)" --batch-all-objects' ran
+ 1.22 ± 0.03 times faster than 'git.compile -c pack.readReverseIndex=true cat-file --unordered --batch-check="%(objectsize:disk)" --batch-all-objects'
+
Because the equilibrium point between these two is highly machine- and repository-dependent, allow users to configure whether or not they will read any ".rev" file(s) with this configuration knob.

4:  56a0fc0098e = 6:  f8298fb0bac config: enable `pack.writeReverseIndex` by default
5:  9c803799588 ! 7:  edff6a80c63 t: invert `GIT_TEST_WRITE_REV_INDEX`

@@ Commit message
disable writing ".rev" files, thereby running the test suite in a mode where the reverse index is generated from scratch.

- This ensures that we are still running and exercising Git's behavior
- when forced to generate reverse indexes from scratch.
+ This ensures that, when GIT_TEST_NO_WRITE_REV_INDEX is set to some
+ spelling of "true", we are still running and exercising Git's behavior
+ when forced to generate reverse indexes from scratch. Do so by setting
+ it in the linux-TEST-vars CI run to ensure that we are maintaining good
+ coverage of this now-legacy code.

@@ ci/run-build-and-tests.sh: linux-TEST-vars)
export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
- export GIT_TEST_WRITE_REV_INDEX=1
++ export GIT_TEST_NO_WRITE_REV_INDEX=1
export GIT_TEST_CHECKOUT_WORKERS=2
;;
linux-clang)

2.40.0.323.gedff6a80c63