Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 10, 2016, 20:46 UTC
- Message-ID
- <af55f6d7-e1b1-272b-4fbe-a6eb2422b3be@web.de>
- In-Reply-To
- <20161010000035.mfcf55wqfcbcnarh@sigill.intra.peff.net>
Am 10.10.2016 um 02:00 schrieb Jeff King:
Show 48 quoted lines
> On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:
>
>> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
>> instead of taking detours through find_unique_abbrev() and its static
>> buffer. This is shorter in most cases and a bit more efficient.
>>
>> The changes here are not easily handled by a semantic patch because
>> they involve removing temporary variables and deconstructing format
>> strings for strbuf_addf().
>
> Yeah, the thing to look for here is whether the sha1 variable holds the
> same value at both times.
>
> These all look OK to me. Mild rambling below.
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 5200d5c..9041c2f 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
>> strbuf_addf(&o->obuf, "virtual %s\n",
>> merge_remote_util(commit)->name);
>> else {
>> - strbuf_addf(&o->obuf, "%s ",
>> - find_unique_abbrev(commit->object.oid.hash,
>> - DEFAULT_ABBREV));
>> + strbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,
>> + DEFAULT_ABBREV);
>> + strbuf_addch(&o->obuf, ' ');
>
> I've often wondered whether a big strbuf_addf() is more efficient than
> several strbuf_addstrs. It amortizes the size-checks, certainly, though
> those are probably not very big. It shouldn't matter much for amortizing
> the cost of malloc, as it's very unlikely to have a case where:
>
> strbuf_addf("%s%s", foo, bar);
>
> would require one malloc, but:
>
> strbuf_addstr(foo);
> strbuf_addstr(bar);
>
> would require two (one of the strings would have to be around the same
> size as the ALLOC_GROW() doubling).
>
> So it probably doesn't matter much in practice (not that most of these
> cases are very performance sensitive anyway). Mostly just something I've
> pondered while tweaking strbuf invocations.Good question. ALLOC_GROW() doesn't double exactly, but indeed the number of reallocations depends on the size of the added pieces. I always thought of strbuf_addf() as an expensive function for convenience, but never timed it.
Numbers vary a bit, but here's what I get for the crude test program at the end:
$ time t/helper/test-strbuf strbuf_addf 123 123456789012345678901234567890 123123456789012345678901234567890
real 0m0.168s user 0m0.164s sys 0m0.000s $ time t/helper/test-strbuf strbuf_addstr 123 123456789012345678901234567890 123123456789012345678901234567890
real 0m0.141s user 0m0.140s sys 0m0.000s
Just a data-point, but it confirms my bias, so I stop here. :)
Show 7 quoted lines
> Just thinking aloud, I've also wondered if strbuf_addoid() would be > handy. We already have oid_to_hex_r(). In fact, you could write it > already as: > > strbuf_add_unique_abbrev(sb, oidp->hash, 0); > > but that is probably not helping maintainability. ;)
Yes, a function for adding full hashes without using a static variable is useful for library functions that may end up being called often or in parallel. I'd call it strbuf_add_hash, though.
diff --git a/Makefile b/Makefile index 1aad150..ad5e98c 100644 --- a/Makefile +++ b/Makefile @@ -628,6 +628,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree TEST_PROGRAMS_NEED_X += test-sha1 TEST_PROGRAMS_NEED_X += test-sha1-array TEST_PROGRAMS_NEED_X += test-sigchain +TEST_PROGRAMS_NEED_X += test-strbuf TEST_PROGRAMS_NEED_X += test-string-list TEST_PROGRAMS_NEED_X += test-submodule-config TEST_PROGRAMS_NEED_X += test-subprocess diff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c new file mode 100644 index 0000000..177f3e2 --- /dev/null +++ b/t/helper/test-strbuf.c @@ -0,0 +1,25 @@ +#include "cache.h" + +int cmd_main(int argc, const char **argv) +{ + struct strbuf sb = STRBUF_INIT; + int i = 1000000; + + if (argc == 4) { + if (!strcmp(argv[1], "strbuf_addf")) { + while (i--) { + strbuf_release(&sb); + strbuf_addf(&sb, "%s%s", argv[2], argv[3]); + } + } + if (!strcmp(argv[1], "strbuf_addstr")) { + while (i--) { + strbuf_release(&sb); + strbuf_addstr(&sb, argv[2]); + strbuf_addstr(&sb, argv[3]); + } + } + puts(sb.buf); + } + return 0; +}