| Message ID | 20200214182211.GB150965@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | combining object filters and bitmaps | expand |
On Fri, Feb 14, 2020 at 01:22:11PM -0500, Jeff King wrote: > When we do a bitmap-aware revision traversal, we create an object_list > for each of the "haves" and "wants" tips. After creating the result > bitmaps these are no longer needed or used, but we never free the list > memory. > > Signed-off-by: Jeff King <peff@peff.net> > --- > object.c | 9 +++++++++ > object.h | 2 ++ > pack-bitmap.c | 5 +++++ > 3 files changed, 16 insertions(+) > > diff --git a/object.c b/object.c > index 142ef69399..4d11949b38 100644 > --- a/object.c > +++ b/object.c > @@ -307,6 +307,15 @@ int object_list_contains(struct object_list *list, struct object *obj) > return 0; > } > > +void object_list_free(struct object_list **list) > +{ > + while (*list) { > + struct object_list *p = *list; > + *list = p->next; > + free(p); > + } > +} Hmm. I was going to write a comment saying more-or-less, "I think I'm nitpicking here, but I'm not crazy about this as a 'while' loop". But, when I wrote this as a 'for' loop instead, I wrote a use-after-free, and then when I rid the code of that, it wasn't any more readable than the version above. > /* > * A zero-length string to which object_array_entry::name can be > * initialized without requiring a malloc/free. > diff --git a/object.h b/object.h > index 25f5ab3d54..2dbabfca0a 100644 > --- a/object.h > +++ b/object.h > @@ -151,6 +151,8 @@ struct object_list *object_list_insert(struct object *item, > > int object_list_contains(struct object_list *list, struct object *obj); > > +void object_list_free(struct object_list **list); > + > /* Object array handling .. */ > void add_object_array(struct object *obj, const char *name, struct object_array *array); > void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path); > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 9ca356ee29..6c06099dc7 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -787,10 +787,15 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs) > bitmap_git->result = wants_bitmap; > bitmap_git->haves = haves_bitmap; > > + object_list_free(&wants); > + object_list_free(&haves); > + Makes sense. > return bitmap_git; > > cleanup: > free_bitmap_index(bitmap_git); > + object_list_free(&wants); > + object_list_free(&haves); Ditto. > return NULL; > } > > -- > 2.25.0.796.gcc29325708 Thanks, Taylor
On Fri, Feb 14, 2020 at 04:15:52PM -0800, Taylor Blau wrote: > > +void object_list_free(struct object_list **list) > > +{ > > + while (*list) { > > + struct object_list *p = *list; > > + *list = p->next; > > + free(p); > > + } > > +} > > Hmm. I was going to write a comment saying more-or-less, "I think I'm > nitpicking here, but I'm not crazy about this as a 'while' loop". But, > when I wrote this as a 'for' loop instead, I wrote a use-after-free, and > then when I rid the code of that, it wasn't any more readable than the > version above. Yep, linked lists are deceptive that way. This _could_ be converted to use the generic list.h macros. Or better still, we could probably ditch it entirely in favor of an object_array. But I'd prefer to do that separately, if at all. -Peff
On 2/14/2020 1:22 PM, Jeff King wrote: > When we do a bitmap-aware revision traversal, we create an object_list > for each of the "haves" and "wants" tips. After creating the result > bitmaps these are no longer needed or used, but we never free the list > memory. It's surprising that we got away with this for so long. Is it possible that this loop of freeing memory could provide significant performance drawbacks? I'm assuming that the free() loop is much faster than the malloc() loop, so the impact is negligible. Thanks, -Stolee
On Tue, Feb 18, 2020 at 12:58:10PM -0500, Derrick Stolee wrote: > On 2/14/2020 1:22 PM, Jeff King wrote: > > When we do a bitmap-aware revision traversal, we create an object_list > > for each of the "haves" and "wants" tips. After creating the result > > bitmaps these are no longer needed or used, but we never free the list > > memory. > > It's surprising that we got away with this for so long. Is it > possible that this loop of freeing memory could provide significant > performance drawbacks? I'm assuming that the free() loop is much > faster than the malloc() loop, so the impact is negligible. I don't think it's surprising. We're only talking about leaking hundreds or perhaps thousands of structs. So maybe a few kilobytes at most. And we'd typically only run one such bitmap traversal per program. Meanwhile rev-list is generally storing that same set of objects in a pending array, and I think we don't generally clean up after it (nor is there even a convenient function to do so). So I think it's sort of a drop in the bucket of Git's "eh, this pretty much lasts about the whole program anyway" leaks. I don't think there's much performance difference either way in freeing or not. It's mostly just a cleanliness thing. (I do one day dream of being able to run the test suite through a leakchecker, but we're still a ways off, I think). -Peff
diff --git a/object.c b/object.c index 142ef69399..4d11949b38 100644 --- a/object.c +++ b/object.c @@ -307,6 +307,15 @@ int object_list_contains(struct object_list *list, struct object *obj) return 0; } +void object_list_free(struct object_list **list) +{ + while (*list) { + struct object_list *p = *list; + *list = p->next; + free(p); + } +} + /* * A zero-length string to which object_array_entry::name can be * initialized without requiring a malloc/free. diff --git a/object.h b/object.h index 25f5ab3d54..2dbabfca0a 100644 --- a/object.h +++ b/object.h @@ -151,6 +151,8 @@ struct object_list *object_list_insert(struct object *item, int object_list_contains(struct object_list *list, struct object *obj); +void object_list_free(struct object_list **list); + /* Object array handling .. */ void add_object_array(struct object *obj, const char *name, struct object_array *array); void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path); diff --git a/pack-bitmap.c b/pack-bitmap.c index 9ca356ee29..6c06099dc7 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -787,10 +787,15 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs) bitmap_git->result = wants_bitmap; bitmap_git->haves = haves_bitmap; + object_list_free(&wants); + object_list_free(&haves); + return bitmap_git; cleanup: free_bitmap_index(bitmap_git); + object_list_free(&wants); + object_list_free(&haves); return NULL; }
When we do a bitmap-aware revision traversal, we create an object_list for each of the "haves" and "wants" tips. After creating the result bitmaps these are no longer needed or used, but we never free the list memory. Signed-off-by: Jeff King <peff@peff.net> --- object.c | 9 +++++++++ object.h | 2 ++ pack-bitmap.c | 5 +++++ 3 files changed, 16 insertions(+)