| Message ID | e65ac7deb5973b8209e8d94ad9140e005e22c44e.1634787555.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | midx: clean up t5319 under 'SANITIZE=leak' | expand |
On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote: > The function free_bitmap_index() is somewhat lax in what it frees. There > are two notable examples: > > - While it does call kh_destroy_oid_map on the "bitmaps" map (which > maps) commit OIDs to their corresponding bitmaps, the bitmaps > themselves are not freed. Note here that we recycle already-freed > ewah_bitmaps into a pool, but these are handled correctly by > ewah_pool_free(). The parentheses placement seems off; it's not clear what the intent is. Perhaps either move the closing parenthesis to just before the comma or drop them altogether. > - We never bother to free the extended index's "positions" map, which > we always allocate in load_bitmap(). > > Fix both of these. > > Signed-off-by: Taylor Blau <me@ttaylorr.com>
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote: >> The function free_bitmap_index() is somewhat lax in what it frees. There >> are two notable examples: >> >> - While it does call kh_destroy_oid_map on the "bitmaps" map (which >> maps) commit OIDs to their corresponding bitmaps, the bitmaps >> themselves are not freed. Note here that we recycle already-freed >> ewah_bitmaps into a pool, but these are handled correctly by >> ewah_pool_free(). > > The parentheses placement seems off; it's not clear what the intent > is. Perhaps either move the closing parenthesis to just before the > comma or drop them altogether. Yeah, I think we can do without them and the sentence becomes clearer (we can add a comma before "which", too). >> - We never bother to free the extended index's "positions" map, which >> we always allocate in load_bitmap(). >> >> Fix both of these. >> >> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes: > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 0f6656db02..451ca3512c 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b) > ewah_pool_free(b->trees); > ewah_pool_free(b->blobs); > ewah_pool_free(b->tags); > + if (b->bitmaps) { > + struct stored_bitmap *sb; > + kh_foreach_value(b->bitmaps, sb, { > + ewah_pool_free(sb->root); > + free(sb); > + }); > + } > kh_destroy_oid_map(b->bitmaps); > free(b->ext_index.objects); > free(b->ext_index.hashes); > + kh_destroy_oid_pos(b->ext_index.positions); > bitmap_free(b->result); > bitmap_free(b->haves); > if (bitmap_is_midx(b)) { It is not a fault of this patch, but I was reading the corresponding load_bitmap() to see if we are covering everything here. On the error path of that function, where it does "got failed", it does not clean after itself very well, it seems. If we read commits bitmap successfully but failed to read trees bitmap, for example, the resource held by commits bitmap is never reclaimed.
On Thu, Oct 21, 2021 at 11:32:16AM -0700, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote: > >> The function free_bitmap_index() is somewhat lax in what it frees. There > >> are two notable examples: > >> > >> - While it does call kh_destroy_oid_map on the "bitmaps" map (which > >> maps) commit OIDs to their corresponding bitmaps, the bitmaps > >> themselves are not freed. Note here that we recycle already-freed > >> ewah_bitmaps into a pool, but these are handled correctly by > >> ewah_pool_free(). > > > > The parentheses placement seems off; it's not clear what the intent > > is. Perhaps either move the closing parenthesis to just before the > > comma or drop them altogether. > > Yeah, I think we can do without them and the sentence becomes > clearer (we can add a comma before "which", too). Yep, thanks both. Thanks, Taylor
diff --git a/pack-bitmap.c b/pack-bitmap.c index 0f6656db02..451ca3512c 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b) ewah_pool_free(b->trees); ewah_pool_free(b->blobs); ewah_pool_free(b->tags); + if (b->bitmaps) { + struct stored_bitmap *sb; + kh_foreach_value(b->bitmaps, sb, { + ewah_pool_free(sb->root); + free(sb); + }); + } kh_destroy_oid_map(b->bitmaps); free(b->ext_index.objects); free(b->ext_index.hashes); + kh_destroy_oid_pos(b->ext_index.positions); bitmap_free(b->result); bitmap_free(b->haves); if (bitmap_is_midx(b)) {
The function free_bitmap_index() is somewhat lax in what it frees. There are two notable examples: - While it does call kh_destroy_oid_map on the "bitmaps" map (which maps) commit OIDs to their corresponding bitmaps, the bitmaps themselves are not freed. Note here that we recycle already-freed ewah_bitmaps into a pool, but these are handled correctly by ewah_pool_free(). - We never bother to free the extended index's "positions" map, which we always allocate in load_bitmap(). Fix both of these. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap.c | 8 ++++++++ 1 file changed, 8 insertions(+)