Re: [PATCH] bisect: save heap memory. allocate only the required amount
From: Christian Couder <hidden>
Date: 2016-06-15 23:02:20
On Mon, Aug 25, 2014 at 3:35 PM, Jeff King [off-list ref] wrote:
On Sun, Aug 24, 2014 at 07:47:24PM +0530, Arjun Sreedharan wrote:quoted
diff --git a/bisect.c b/bisect.c index d6e851d..c96aab0 100644 --- a/bisect.c +++ b/bisect.c@@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n } qsort(array, cnt, sizeof(*array), compare_commit_dist); for (p = list, i = 0; i < cnt; i++) { - struct name_decoration *r = xmalloc(sizeof(*r) + 100); + char name[100]; + sprintf(name, "dist=%d", array[i].distance); + int name_len = strlen(name); + struct name_decoration *r = xmalloc(sizeof(*r) + name_len);This allocation should be name_len + 1 for the NUL-terminator, no?
I wondered about that too, but as struct name_decoration is defined like this:
struct name_decoration {
struct name_decoration *next;
int type;
char name[1];
};
the .name field of this struct already has one char, so the allocation
above should be ok.
It looks like add_name_decoration in log-tree already handles half of what you are adding here. Can we just make that available globally (it is manipulating the already-global "struct decoration name_decoration")?
Yeah, it looks like it should be better. Note that add_name_decoration() does: int nlen = strlen(name); struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen); so it also relies on the fact that .name contains one char.
quoted hunk ↗ jump to hunk
I also notice that we do not set r->type at all, meaning the decoration lookup code in log-tree will access uninitialized memory (worse, it will use it as a pointer offset into the color list; I got a segfault when I tried to run "git rev-list --bisect-all v1.8.0..v1.9.0"). I think we need this:diff --git a/bisect.c b/bisect.c index d6e851d..e2a7682 100644 --- a/bisect.c +++ b/bisect.c@@ -219,6 +219,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n struct object *obj = &(array[i].commit->object); sprintf(r->name, "dist=%d", array[i].distance); + r->type = 0; r->next = add_decoration(&name_decoration, obj, r); p->item = array[i].commit; p = p->next;at a minimum.
Yeah if we don't use add_name_decoration() we would need that. Thanks for noticing.
It looks like this was a regression caused by eb3005e (commit.h: add 'type' to struct name_decoration, 2010-06-19). Which makes me wonder if anybody actually _uses_ --bisect-all (which AFAICT is the only way to trigger the problem), but since it's public, I guess we should keep it.
Yeah, we should probably keep it.
I think the sane thing here is to stop advertising name_decoration as a global, and make all callers use add_name_decoration. That makes it easier for callers like this one, and would have caught the regression caused be eb3005e (the compiler would have noticed that we were not passing a type parameter to the function).
I agree. Thanks, Christian.