Object store refactoring: make bitmap_git not global
cover Object store refactoring: make bitmap_git not global
This is a continuation of the object store refactoring effort.
We cannot truly free an object store without ensuring that any generated bitmaps are first freed, so here are patches to drastically reduce the lifetime of any bitmaps generated. As a bonus, the API is also improved, and global state reduced.
Jonathan Tan (2):
pack-bitmap: remove bitmap_git global variable
pack-bitmap: add free function
builtin/pack-objects.c | 7 +- builtin/rev-list.c | 13 +- pack-bitmap-write.c | 10 +- pack-bitmap.c | 344 ++++++++++++++++++++++++----------------- pack-bitmap.h | 20 ++- 5 files changed, 234 insertions(+), 160 deletions(-)
Signature · 1 line
2.17.0.768.g1526ddbba1.dirty
PATCH 1/2 pack-bitmap: remove bitmap_git global variable
Remove the bitmap_git global variable. Instead, generate on demand an instance of struct bitmap_index for code that needs to access it.
This allows us significant control over the lifetime of instances of struct bitmap_index. In particular, packs can now be closed without worrying if an unnecessarily long-lived "pack" field in struct bitmap_index still points to it.
The bitmap API is also clearer in that we need to first obtain a struct bitmap_index, then we use it.
--- builtin/pack-objects.c | 6 +- builtin/rev-list.c | 11 +- pack-bitmap-write.c | 6 +- pack-bitmap.c | 317 ++++++++++++++++++++++------------------- pack-bitmap.h | 19 ++- 5 files changed, 201 insertions(+), 158 deletions(-)
builtin/pack-objects.c +4 -2
@@ -2929,11 +2929,13 @@ static int pack_options_allow_reuse(void)static int get_object_list_from_bitmap (struct rev_info *revs) { -if (prepare_bitmap_walk (revs) <0 ) +struct bitmap_index *bitmap_git; +if (!(bitmap_git =prepare_bitmap_walk (revs)))return -1 ;if (pack_options_allow_reuse () && !reuse_partial_packfile_from_bitmap ( + bitmap_git, &reuse_packfile, &reuse_packfile_objects, &reuse_packfile_offset)) {
@@ -2942,7 +2944,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)display_progress (progress_state, nr_result); } -traverse_bitmap_commit_list (&add_object_entry_from_bitmap); +traverse_bitmap_commit_list (bitmap_git, &add_object_entry_from_bitmap);return 0 ; }
builtin/rev-list.c +7 -4
@@ -16,6 +16,7 @@#include "reflog-walk.h" #include "oidset.h" #include "packfile.h" +#include "object-store.h" static const char rev_list_usage[] ="git rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
@@ -514,8 +515,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (revs.count && !revs.left_right && !revs.cherry_mark) {uint32_t commit_count;int max_count = revs.max_count; -if (!prepare_bitmap_walk (&revs)) { -count_bitmap_commit_list (&commit_count,NULL ,NULL ,NULL ); +struct bitmap_index *bitmap_git; +if ((bitmap_git =prepare_bitmap_walk (&revs))) { +count_bitmap_commit_list (bitmap_git, &commit_count,NULL ,NULL ,NULL );if (max_count >=0 && max_count < commit_count) commit_count = max_count;printf ("%d\n" , commit_count);
@@ -523,8 +525,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) } }else if (revs.max_count <0 && revs.tag_objects && revs.tree_objects && revs.blob_objects) { -if (!prepare_bitmap_walk (&revs)) { -traverse_bitmap_commit_list (&show_object_fast); +struct bitmap_index *bitmap_git; +if ((bitmap_git =prepare_bitmap_walk (&revs))) { +traverse_bitmap_commit_list (bitmap_git, &show_object_fast);return 0 ; } }
pack-bitmap-write.c +4 -2
@@ -360,11 +360,13 @@ static int date_compare(const void *_a, const void *_b)void bitmap_writer_reuse_bitmaps (struct packing_data *to_pack) { -if (prepare_bitmap_git () <0 ) +struct bitmap_index *bitmap_git; +if (!(bitmap_git =prepare_bitmap_git ()))return ; writer.reused =kh_init_sha1 (); -rebuild_existing_bitmaps (to_pack, writer.reused, writer.show_progress); +rebuild_existing_bitmaps (bitmap_git, to_pack, writer.reused, + writer.show_progress); }static struct ewah_bitmap *find_reused_bitmap (const unsigned char *sha1)
pack-bitmap.c +173 -144
717 lines not on this page · raw message
pack-bitmap.h +13 -6
@@ -34,13 +34,20 @@ typedef int (*show_reachable_fn)(struct packed_git *found_pack, off_t found_offset); -int prepare_bitmap_git (void ); -void count_bitmap_commit_list (uint32_t *commits,uint32_t *trees,uint32_t *blobs,uint32_t *tags); -void traverse_bitmap_commit_list (show_reachable_fn show_reachable); +struct bitmap_index ; + +struct bitmap_index *prepare_bitmap_git (void ); +void count_bitmap_commit_list (struct bitmap_index *,uint32_t *commits, +uint32_t *trees,uint32_t *blobs,uint32_t *tags); +void traverse_bitmap_commit_list (struct bitmap_index *, +show_reachable_fn show_reachable);void test_bitmap_walk (struct rev_info *revs); -int prepare_bitmap_walk (struct rev_info *revs); -int reuse_partial_packfile_from_bitmap (struct packed_git **packfile,uint32_t *entries,off_t *up_to); -int rebuild_existing_bitmaps (struct packing_data *mapping,khash_sha1 *reused_bitmaps,int show_progress); +struct bitmap_index *prepare_bitmap_walk (struct rev_info *revs); +int reuse_partial_packfile_from_bitmap (struct bitmap_index *, +struct packed_git **packfile, +uint32_t *entries,off_t *up_to); +int rebuild_existing_bitmaps (struct bitmap_index *,struct packing_data *mapping, +khash_sha1 *reused_bitmaps,int show_progress);void bitmap_writer_show_progress (int show);void bitmap_writer_set_checksum (unsigned char *sha1);
Signature · 1 line
2.17.0.768.g1526ddbba1.dirty
Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable
Quoted 10 lines · Jonathan Tan
Remove the
bitmap_gitglobal variable. Instead, generate on demand an instance of structbitmap_indexfor code that needs to access it.This allows us significant control over the lifetime of instances of struct
bitmap_index. In particular, packs can now be closed without worrying if an unnecessarily long-lived "pack" field in structbitmap_indexstill points to it.The bitmap API is also clearer in that we need to first obtain a struct
bitmap_index, then we use it.
I think this is the right direction, and overall it looks pretty good. There's one call that gave me pause:
pack-bitmap.c static int open_pack_bitmap_1(struct packed_git *packfile) · line 348
Quoted 6 lines
pack-bitmap.c +2
-3-int prepare_bitmap_git (void ) +struct bitmap_index *prepare_bitmap_git (void ) { -if (bitmap_git.loaded) -return 0 ; +struct bitmap_index *bitmap_git =xcalloc (1 ,sizeof (*bitmap_git));
This function used to be idempotent, so any code which wanted to use the global bitmap_git could call it "just in case". After your patch, it's not. I think it's probably OK, since such functions would generally now take a bitmap_git argument and use that (e.g., rebuild_existing_bitmaps works that way after your patch).
pack-bitmap.c static int open_pack_bitmap_1(struct packed_git *packfile) · line 354
Quoted 8 lines
pack-bitmap.c +3
-3-if (!open_pack_bitmap ()) -return load_pack_bitmap (); +if (!open_pack_bitmap (bitmap_git) && !load_pack_bitmap (bitmap_git)) +return bitmap_git; -return -1 ; +return NULL ; }
We probably need to free(bitmap_git) before returning NULL here (this is still in prepare_bitmap_git()).
Quoted 5 lines
pack-bitmap.c +0
-1@@ -662,12 +686,11 @@ int prepare_bitmap_walk(struct rev_info *revs)struct bitmap *wants_bitmap =NULL ;struct bitmap *haves_bitmap =NULL ; -if (!bitmap_git.loaded) {
pack-bitmap.c int prepare_bitmap_walk(struct rev_info *revs) · line 693
Quoted 10 lines
pack-bitmap.c +5
-5-/* try to open a bitmapped pack, but don't parse it yet -* because we may not need to use it */ -if (open_pack_bitmap () <0 ) -return -1 ; - } +struct bitmap_index *bitmap_git =xcalloc (1 ,sizeof (*bitmap_git)); +/* try to open a bitmapped pack, but don't parse it yet +* because we may not need to use it */ +if (open_pack_bitmap (bitmap_git) <0 ) +return NULL ;
Ditto here (and probably other error returns lower in the function, but I didn't go through it carefully).
-Peff
Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable
Quoted 5 lines · On Sat, 9 Jun 2018 02:04:38 -0400 Jeff King <[email protected]> wrote:
This function used to be idempotent, so any code which wanted to use the global
bitmap_gitcould call it "just in case". After your patch, it's not. I think it's probably OK, since such functions would generally now take abitmap_gitargument and use that (e.g.,rebuild_existing_bitmapsworks that way after your patch).
Yes, and I've checked that the existing callers do not use bitmaps twice. I've appended an extra paragraph for the commit message to this e-mail below that will hopefully clarify things.
Quoted 2 lines
We probably need to
free(bitmap_git)before returning NULL here (this is still inprepare_bitmap_git()).
[snip]
Quoted 2 lines
Ditto here (and probably other error returns lower in the function, but I didn't go through it carefully).
The contents of the struct also need to be handled, and I wanted to handle it all at once in the subsequent patch. But you're right that the commit message should explain that.
Here's an paragraph to be added to the end of the commit message. I can send a reroll with the exact same code but with the updated commit message if Junio requests it.
[additional paragraph begin]
This patch raises two potential issues: (1) memory for the struct bitmap_index is allocated without being freed, and (2) prepare_bitmap_git() and prepare_bitmap_walk() can reuse a previously loaded bitmap. For (1), this will be dealt with in a subsequent patch in this patch set that also deals with freeing the contents of the struct bitmap_index (which were not freed previously, because they have global scope). For (2), current bitmap users only load the bitmap once at most (note that pack-objects can use bitmaps or write bitmaps, but not both at the same time), so support for reuse has no effect - and future users can pass around the struct bitmap_index * obtained if they need to do 2 or more things with the same bitmap.
[additional paragraph end]
Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable
Quoted 18 lines · Jonathan Tan
Here's an paragraph to be added to the end of the commit message. I can send a reroll with the exact same code but with the updated commit message if Junio requests it.
[additional paragraph begin]
This patch raises two potential issues: (1) memory for the struct
bitmap_indexis allocated without being freed, and (2)prepare_bitmap_git()andprepare_bitmap_walk()can reuse a previously loaded bitmap. For (1), this will be dealt with in a subsequent patch in this patch set that also deals with freeing the contents of the structbitmap_index(which were not freed previously, because they have global scope). For (2), current bitmap users only load the bitmap once at most (note that pack-objects can use bitmaps or write bitmaps, but not both at the same time), so support for reuse has no effect - and future users can pass around the structbitmap_index* obtained if they need to do 2 or more things with the same bitmap.
Quoted 1 line
[additional paragraph end]
Yeah, that sounds fine to me. Thanks.
-Peff
PATCH 2/2 pack-bitmap: add free function
In reply to Jonathan TanAdd a function to free struct bitmap_index instances, and use it where needed (except when rebuild_existing_bitmaps() is used, since it creates references to the bitmaps within the struct bitmap_index passed to it).
Note that the hashes field in struct bitmap_index is not freed because it points to another field within the same struct. The documentation for that field has been updated to clarify that.
--- builtin/pack-objects.c | 1 + builtin/rev-list.c | 2 ++ pack-bitmap-write.c | 4 ++++ pack-bitmap.c | 35 +++++++++++++++++++++++++++++------ pack-bitmap.h | 1 + 5 files changed, 37 insertions(+), 6 deletions(-)
builtin/pack-objects.c +1 -0
@@ -2945,6 +2945,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs) }traverse_bitmap_commit_list (bitmap_git, &add_object_entry_from_bitmap); +free_bitmap_index (bitmap_git);return 0 ; }
builtin/rev-list.c +2 -0
@@ -521,6 +521,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (max_count >=0 && max_count < commit_count) commit_count = max_count;printf ("%d\n" , commit_count); +free_bitmap_index (bitmap_git);return 0 ; } } else if (revs.max_count < 0 &&
@@ -528,6 +529,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)struct bitmap_index *bitmap_git;if ((bitmap_git =prepare_bitmap_walk (&revs))) {traverse_bitmap_commit_list (bitmap_git, &show_object_fast); +free_bitmap_index (bitmap_git);return 0 ; } }
pack-bitmap-write.c +4 -0
@@ -367,6 +367,10 @@ void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack) writer.reused =kh_init_sha1 ();rebuild_existing_bitmaps (bitmap_git, to_pack, writer.reused, writer.show_progress); +/* +* NEEDSWORK: rebuild_existing_bitmaps() makes writer.reused reference +* some bitmaps in bitmap_git, so we can't free the latter. +*/ }static struct ewah_bitmap *find_reused_bitmap (const unsigned char *sha1)
pack-bitmap.c +29 -6
@@ -66,7 +66,7 @@ struct bitmap_index {/* Number of bitmapped commits */ uint32_t entry_count; -/* Name-hash cache (or NULL if not present). */ +/* If not NULL, this is a name-hash cache pointing into map. */ uint32_t *hashes;/*
@@ -350,6 +350,7 @@ struct bitmap_index *prepare_bitmap_git(void)if (!open_pack_bitmap (bitmap_git) && !load_pack_bitmap (bitmap_git))return bitmap_git; +free_bitmap_index (bitmap_git);return NULL ; }
@@ -690,7 +691,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)/* try to open a bitmapped pack, but don't parse it yet * because we may not need to use it */ if (open_pack_bitmap (bitmap_git) <0 ) -return NULL ; + goto cleanup;for (i =0 ; i < revs->pending.nr; ++i) {struct object *object = revs->pending.objects[i].item;
@@ -723,11 +724,11 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)* optimize here */ if (haves && !in_bitmapped_pack (bitmap_git, haves)) -return NULL ; + goto cleanup;/* if we don't want anything, we're done here */ if (!wants) -return NULL ; + goto cleanup;/* * now we're going to use bitmaps, so load the actual bitmap entries
@@ -735,7 +736,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)* becomes invalidated and we must perform the revwalk through bitmaps */ if (!bitmap_git->loaded &&load_pack_bitmap (bitmap_git) <0 ) -return NULL ; + goto cleanup;object_array_clear (&revs->pending);
@@ -761,6 +762,10 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)bitmap_free (haves_bitmap);return bitmap_git; + +cleanup: +free_bitmap_index (bitmap_git); +return NULL ; } int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
@@ -1001,7 +1006,7 @@ void test_bitmap_walk(struct rev_info *revs)else fprintf (stderr,"Mismatch!\n" ); -bitmap_free (result); +free_bitmap_index (bitmap_git); } static int rebuild_bitmap(uint32_t *reposition,
@@ -1093,3 +1098,21 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,bitmap_free (rebuild);return 0 ; } + +void free_bitmap_index (struct bitmap_index *b) +{ +if (!b) +return ; + +if (b->map) +munmap (b->map, b->map_size); +ewah_pool_free (b->commits); +ewah_pool_free (b->trees); +ewah_pool_free (b->blobs); +ewah_pool_free (b->tags); +kh_destroy_sha1 (b->bitmaps); +free (b->ext_index.objects); +free (b->ext_index.hashes); +bitmap_free (b->result); +free (b); +}
pack-bitmap.h +1 -0
@@ -48,6 +48,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *, uint32_t *entries, off_t *up_to);int rebuild_existing_bitmaps (struct bitmap_index *,struct packing_data *mapping,khash_sha1 *reused_bitmaps,int show_progress); +void free_bitmap_index (struct bitmap_index *);void bitmap_writer_show_progress (int show);void bitmap_writer_set_checksum (unsigned char *sha1);
Signature · 1 line
2.17.0.768.g1526ddbba1.dirty
Re: [PATCH 2/2] pack-bitmap: add free function
Quoted 7 lines · Jonathan Tan
Add a function to free struct
bitmap_indexinstances, and use it where needed (except whenrebuild_existing_bitmaps()is used, since it creates references to the bitmaps within the structbitmap_indexpassed to it).Note that the hashes field in struct
bitmap_indexis not freed because it points to another field within the same struct. The documentation for that field has been updated to clarify that.
Hrm, this one fixes the leaks I noticed in the previous patch. I'm OK with doing it separately if it breaking it down this way makes the code easier to reason about. But we should probably say something in the commit message about it.
-Peff
Quoted 8 lines · Jonathan Tan
This is a continuation of the object store refactoring effort.
We cannot truly free an object store without ensuring that any generated bitmaps are first freed, so here are patches to drastically reduce the lifetime of any bitmaps generated. As a bonus, the API is also improved, and global state reduced.
Thanks for writing this series, I have no comments on it.
Thanks,
Stefan
Quoted 6 lines · Jonathan Tan
This is a continuation of the object store refactoring effort.
We cannot truly free an object store without ensuring that any generated bitmaps are first freed, so here are patches to drastically reduce the lifetime of any bitmaps generated. As a bonus, the API is also improved, and global state reduced.
I've reviewed this series and haven't found any issues.
Signature · 1 line
Brandon Williams