From 07647c92ffabbb639436de8b5634244cbdfd6ef2 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 14 May 2024 15:56:56 -0400 Subject: pack-bitmap: avoid use of static `bitmap_writer` The pack-bitmap machinery uses a structure called 'bitmap_writer' to collect the data necessary to write out .bitmap files. Since its introduction in 7cc8f971085 (pack-objects: implement bitmap writing, 2013-12-21), there has been a single static bitmap_writer structure, which is responsible for all bitmap writing-related operations. In practice, this is OK, since we are only ever writing a single .bitmap file in a single process (e.g., `git multi-pack-index write --bitmap`, `git pack-objects --write-bitmap-index`, `git repack -b`, etc.). However, having a single static variable makes issues like data ownership unclear, when to free variables, what has/hasn't been initialized unclear. Refactor this code to be written in terms of a given bitmap_writer structure instead of relying on a static global. Note that this exposes the structure definition of the bitmap_writer at the pack-bitmap.h level. We could work around this by, e.g., forcing callers to declare their writers as: struct bitmap_writer *writer; bitmap_writer_init(&bitmap_writer); and then declaring `bitmap_writer_init()` as taking in a double-pointer like so: void bitmap_writer_init(struct bitmap_writer **writer); which would avoid us having to expose the definition of the structure itself. This patch takes a different approach, since future patches (like for the ongoing pseudo-merge bitmaps work) will want to modify the innards of this structure (in the previous example, via pseudo-merge.c). Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- pack-bitmap.h | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) (limited to 'pack-bitmap.h') diff --git a/pack-bitmap.h b/pack-bitmap.h index c7dea13217..5a1890a2c5 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -97,9 +97,29 @@ int bitmap_has_oid_in_uninteresting(struct bitmap_index *, const struct object_i off_t get_disk_usage_from_bitmap(struct bitmap_index *, struct rev_info *); -void bitmap_writer_show_progress(int show); -void bitmap_writer_set_checksum(const unsigned char *sha1); -void bitmap_writer_build_type_index(struct packing_data *to_pack, +struct bitmap_writer { + struct ewah_bitmap *commits; + struct ewah_bitmap *trees; + struct ewah_bitmap *blobs; + struct ewah_bitmap *tags; + + kh_oid_map_t *bitmaps; + struct packing_data *to_pack; + + struct bitmapped_commit *selected; + unsigned int selected_nr, selected_alloc; + + struct progress *progress; + int show_progress; + unsigned char pack_checksum[GIT_MAX_RAWSZ]; +}; + +void bitmap_writer_init(struct bitmap_writer *writer); +void bitmap_writer_show_progress(struct bitmap_writer *writer, int show); +void bitmap_writer_set_checksum(struct bitmap_writer *writer, + const unsigned char *sha1); +void bitmap_writer_build_type_index(struct bitmap_writer *writer, + struct packing_data *to_pack, struct pack_idx_entry **index, uint32_t index_nr); uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git, @@ -109,10 +129,14 @@ int rebuild_bitmap(const uint32_t *reposition, struct bitmap *dest); struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git, struct commit *commit); -void bitmap_writer_select_commits(struct commit **indexed_commits, - unsigned int indexed_commits_nr, int max_bitmaps); -int bitmap_writer_build(struct packing_data *to_pack); -void bitmap_writer_finish(struct pack_idx_entry **index, +void bitmap_writer_select_commits(struct bitmap_writer *writer, + struct commit **indexed_commits, + unsigned int indexed_commits_nr, + int max_bitmaps); +int bitmap_writer_build(struct bitmap_writer *writer, + struct packing_data *to_pack); +void bitmap_writer_finish(struct bitmap_writer *writer, + struct pack_idx_entry **index, uint32_t index_nr, const char *filename, uint16_t options); -- cgit v1.3 From 9675b0691732d5475a353a69c3a8e14804b22a64 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 14 May 2024 15:57:00 -0400 Subject: pack-bitmap: drop unused `max_bitmaps` parameter The `max_bitmaps` parameter in `bitmap_writer_select_commits()` was introduced back in 7cc8f97108 (pack-objects: implement bitmap writing, 2013-12-21), making it original to the bitmap implementation in Git itself. When that patch was merged via 0f9e62e084 (Merge branch 'jk/pack-bitmap', 2014-02-27), its sole caller in builtin/pack-objects.c passed a value of "-1" for `max_bitmaps`, indicating no limit. Since then, the only other caller (in midx.c, added via c528e17966 (pack-bitmap: write multi-pack bitmaps, 2021-08-31)) also uses a value of "-1" for `max_bitmaps`. Since no callers have needed a finite limit for the `max_bitmaps` parameter in the nearly decade that has passed since 0f9e62e084, let's remove the parameter and any dead pieces of code connected to it. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 3 +-- midx-write.c | 2 +- pack-bitmap-write.c | 8 +------- pack-bitmap.h | 3 +-- 4 files changed, 4 insertions(+), 12 deletions(-) (limited to 'pack-bitmap.h') diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index ba4c93d241..10e69fdc8e 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1364,8 +1364,7 @@ static void write_pack_file(void) progress); bitmap_writer_select_commits(&bitmap_writer, indexed_commits, - indexed_commits_nr, - -1); + indexed_commits_nr); if (bitmap_writer_build(&bitmap_writer, &to_pack) < 0) die(_("failed to write bitmap index")); bitmap_writer_finish(&bitmap_writer, diff --git a/midx-write.c b/midx-write.c index 5fdc8f2ff5..78fb0a2c8c 100644 --- a/midx-write.c +++ b/midx-write.c @@ -841,7 +841,7 @@ static int write_midx_bitmap(const char *midx_name, for (i = 0; i < pdata->nr_objects; i++) index[pack_order[i]] = &pdata->objects[i].idx; - bitmap_writer_select_commits(&writer, commits, commits_nr, -1); + bitmap_writer_select_commits(&writer, commits, commits_nr); ret = bitmap_writer_build(&writer, pdata); if (ret < 0) goto cleanup; diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index e22fa70745..c0087dab12 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -587,8 +587,7 @@ static int date_compare(const void *_a, const void *_b) void bitmap_writer_select_commits(struct bitmap_writer *writer, struct commit **indexed_commits, - unsigned int indexed_commits_nr, - int max_bitmaps) + unsigned int indexed_commits_nr) { unsigned int i = 0, j, next; @@ -611,11 +610,6 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer, if (i + next >= indexed_commits_nr) break; - if (max_bitmaps > 0 && writer->selected_nr >= max_bitmaps) { - writer->selected_nr = max_bitmaps; - break; - } - if (next == 0) { chosen = indexed_commits[i]; } else { diff --git a/pack-bitmap.h b/pack-bitmap.h index 5a1890a2c5..b2d13d40eb 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -131,8 +131,7 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git, struct commit *commit); void bitmap_writer_select_commits(struct bitmap_writer *writer, struct commit **indexed_commits, - unsigned int indexed_commits_nr, - int max_bitmaps); + unsigned int indexed_commits_nr); int bitmap_writer_build(struct bitmap_writer *writer, struct packing_data *to_pack); void bitmap_writer_finish(struct bitmap_writer *writer, -- cgit v1.3 From 85f360fee53933d230fd231db5306b26809fabcf Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 14 May 2024 15:57:06 -0400 Subject: pack-bitmap: introduce `bitmap_writer_free()` Now that there is clearer memory ownership around the bitmap_writer structure, introduce a bitmap_writer_free() function that callers may use to free any memory associated with their instance of the bitmap_writer structure. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 3 ++- midx-write.c | 1 + pack-bitmap-write.c | 23 +++++++++++++++++++++++ pack-bitmap.h | 1 + 4 files changed, 27 insertions(+), 1 deletion(-) (limited to 'pack-bitmap.h') diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 10e69fdc8e..26a6d0d791 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1245,7 +1245,6 @@ static void write_pack_file(void) uint32_t nr_remaining = nr_result; time_t last_mtime = 0; struct object_entry **write_order; - struct bitmap_writer bitmap_writer; if (progress > pack_to_stdout) progress_state = start_progress(_("Writing objects"), nr_result); @@ -1315,6 +1314,7 @@ static void write_pack_file(void) if (!pack_to_stdout) { struct stat st; struct strbuf tmpname = STRBUF_INIT; + struct bitmap_writer bitmap_writer; char *idx_tmp_name = NULL; /* @@ -1370,6 +1370,7 @@ static void write_pack_file(void) bitmap_writer_finish(&bitmap_writer, written_list, nr_written, tmpname.buf, write_bitmap_options); + bitmap_writer_free(&bitmap_writer); write_bitmap_index = 0; strbuf_setlen(&tmpname, tmpname_len); } diff --git a/midx-write.c b/midx-write.c index 78fb0a2c8c..7c0c08c64b 100644 --- a/midx-write.c +++ b/midx-write.c @@ -853,6 +853,7 @@ static int write_midx_bitmap(const char *midx_name, cleanup: free(index); free(bitmap_name); + bitmap_writer_free(&writer); trace2_region_leave("midx", "write_midx_bitmap", the_repository); diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 420f17c2e0..6cae670412 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -32,6 +32,29 @@ void bitmap_writer_init(struct bitmap_writer *writer) memset(writer, 0, sizeof(struct bitmap_writer)); } +void bitmap_writer_free(struct bitmap_writer *writer) +{ + uint32_t i; + + if (!writer) + return; + + ewah_free(writer->commits); + ewah_free(writer->trees); + ewah_free(writer->blobs); + ewah_free(writer->tags); + + kh_destroy_oid_map(writer->bitmaps); + + for (i = 0; i < writer->selected_nr; i++) { + struct bitmapped_commit *bc = &writer->selected[i]; + if (bc->write_as != bc->bitmap) + ewah_free(bc->write_as); + ewah_free(bc->bitmap); + } + free(writer->selected); +} + void bitmap_writer_show_progress(struct bitmap_writer *writer, int show) { writer->show_progress = show; diff --git a/pack-bitmap.h b/pack-bitmap.h index b2d13d40eb..3091095f33 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -139,6 +139,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer, uint32_t index_nr, const char *filename, uint16_t options); +void bitmap_writer_free(struct bitmap_writer *writer); char *midx_bitmap_filename(struct multi_pack_index *midx); char *pack_bitmap_filename(struct packed_git *p); -- cgit v1.3