Re: [PATCH 08/10] name-rev: pre-size buffer in get_parent_name()
- From
René Scharfe <l.s.r@web.de>
- Date
- Feb 5, 2020, 15:16 UTC
- Message-ID
- <ea3910e1-f1cb-8a3c-0e32-5c33eeeb8af6@web.de>
- In-Reply-To
- <9b98835d-d94c-19f5-42ba-d583a33306cc@gmail.com>
Am 05.02.20 um 04:19 schrieb Derrick Stolee:
Show 27 quoted lines
> 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.
Show 16 quoted lines
>> + 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
>>
>