Re: [PATCH 08/10] name-rev: pre-size buffer in get_parent_name()

["René Scharfe" <l.s.r@web.de>]

Am 05.02.20 um 04:19 schrieb Derrick Stolee:
> On 2/4/2020 4:24 PM, René Scharfe wrote:
>> We can calculate the size of new name easily and precisely. Open-code
>> the xstrfmt() calls and grow the buffers as needed before filling them.
>> This provides a surprisingly large benefit when working with the
>> Chromium repository; here are the numbers measured using hyperfine
>> before:
>>
>> Benchmark #1: ./git -C ../chromium/src name-rev --all
>>   Time (mean ± σ):      5.822 s ±  0.013 s    [User: 5.304 s, System: 0.516 s]
>>   Range (min … max):    5.803 s …  5.837 s    10 runs
>>
>> ... and with this patch:
>>
>> Benchmark #1: ./git -C ../chromium/src name-rev --all
>>   Time (mean ± σ):      1.527 s ±  0.003 s    [User: 1.015 s, System: 0.511 s]
>>   Range (min … max):    1.524 s …  1.535 s    10 runs
>
> Nice!
>
>> +	if (name->generation > 0) {
>> +		strbuf_grow(&sb, len +
>> +			    1 + decimal_width(name->generation) +
>> +			    1 + decimal_width(parent_number));
>
> Just curious: these strbuf_grow() calls are what _really_ improve the
> performance, right? If you dropped them, then can we expect the performance
> to be similar to the old code?

The replaced xstrfmt() is defined in strbuf.c and trivially wraps
xstrvfmt(), which in turn does:

	struct strbuf buf = STRBUF_INIT;
	strbuf_vaddf(&buf, fmt, ap);
	return strbuf_detach(&buf, NULL);

And strbuf_addf() trivially wraps strbuf_vaddf().  So indeed, all
I did was to inline xstrfmt() and add the strbuf_grow() calls.

Their high impact shocked me a bit.  vsnprintf(3) really doesn't seem
to like working with a buffer that is too small (when it's supposed to
just calculate how many bytes it would have needed), at least not in
the one from glibc 2.29.

>> +		strbuf_addf(&sb, "%.*s~%d^%d", (int)len, name->tip_name,
>> +			    name->generation, parent_number);
>> +	} else {
>> +		strbuf_grow(&sb, len +
>> +			    1 + decimal_width(parent_number));
>> +		strbuf_addf(&sb, "%.*s^%d", (int)len, name->tip_name,
>> +			    parent_number);
>> +	}
>> +	return strbuf_detach(&sb, NULL);
>>  }
>>
>>  static void name_rev(struct commit *start_commit,
>> --
>> 2.25.0
>>
>