Re: [PATCH v2 2/2] sha1_file: improve sha1_file_name() perfs

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2018, 20:54 UTC
Message-ID: <xmqq4lnkryt2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <3cbace45-61df-3074-a48c-66c68c96d542@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 13 quoted lines

>>     void sha1_file_name(struct strbuf *buf, const unsigned char
>> *sha1)
>>   {
>> -	strbuf_addf(buf, "%s/", get_object_directory());
>> +	const char *obj_dir = get_object_directory();
>> +	size_t extra = strlen(obj_dir) + 1 + GIT_MAX_HEXSZ;
>
> Very minor nit.  Should this be "+3" rather than "+1"?
> One for the slash after obj_dir, one for the slash between
> "xx/y[38]", and one for the trailing NUL.
>
>>   +	if (extra > strbuf_avail(buf))
>> +		strbuf_grow(buf, extra);

The callers who care use static strbuf with 1/2, which lets them grow it to an appropriate size after they make their first call.

On the other hand, the ones to which performance does not matter by definition do not care.

I actually think this whole "extra -> grow" business should be discarded. With a miscomputed "extra" like this, it does not help anybody---everybody may pay cost for one extra realloc due to the miscalculation, and the ones that do care also do during their first call.