| Message ID | bf65967764f34adc2ca00d4c8195840ad3e4e127.1715716605.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 85f360fee53933d230fd231db5306b26809fabcf |
| Headers | show |
| Series | pack-bitmap: various pack-bitmap-write cleanups | expand |
On Tue, May 14, 2024 at 03:57:06PM -0400, Taylor Blau wrote: > 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. Great. I wanted to ask about this in preceding commits already, good to see that you already thought of if. > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > 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(-) > > 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; > > /* Nit: we could have avoided moving the struct if it was introduced in this spot in the preceding patch. [snip] > 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); I was wondering whether we also want to set all those fields to `NULL` at the end, or just `memset()` the whole structure. But there probably isn't much of a reason to do it currently, so I don't mind if your answer is "no". Patrick
On Wed, May 15, 2024 at 11:05:18AM +0200, Patrick Steinhardt wrote: > > 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; > > > > /* > > Nit: we could have avoided moving the struct if it was introduced in > this spot in the preceding patch. Ugh, I meant to move the declaration here in the previous patch, but apparently didn't. I don't think it's worth sending another round just to fix that minor issue, but happy to do so if you feel strongly. Thanks, Taylor
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);
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 <me@ttaylorr.com> --- 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(-)