Re: [PATCH 1/3] use struct sha1_array in diff_tree_combined()

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Dec 17, 2011, 11:16 UTC
Message-ID: <4EEC7A10.3080705@lsrfire.ath.cx>
In-Reply-To: <20111217105315.GA23935@sigill.intra.peff.net>

Am 17.12.2011 11:53, schrieb Jeff King:

Show 17 quoted lines

>> -	parent = xmalloc(ents * sizeof(*parent));
>> -	for (i = 0; i<  ents; i++)
>> -		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
>> -	diff_tree_combined(parent[0], parent + 1, ents - 1,
>> +	for (i = 1; i<  ents; i++)
>> +		sha1_array_append(&parents, ent[i].item->sha1);
>> +	diff_tree_combined(ent[0].item->sha1,&parents,
>>   			   revs->dense_combined_merges, revs);
>> -	free((void *)parent);
>> +	sha1_array_clear(&parents);
>
> The original code is slightly more efficient, as it is able to use a
> single malloc (because it knows the number of entries ahead of time).
> It probably doesn't make a difference, but we could also add a
> sha1_array_grow() for this case.
>
> I think it could be used in all three spots you converted in this patch.

We coulddo that, yes. In the case above we have the number already, in the other cases we'd have to count.

But I don't think it's worth it here. ALLOC_GROW gives us 24 entries initially, which should be enough in most cases -- I'm not sure I want to see combined diff of that many tree. And 24 times 20 bytes is small enough to not cause any memory allocation issues.

Taking out the counting and not having to worry about the number of items is a feature in my book. Simple timings before and after the patch didn't show any significant difference.

René