{"thread":{"id":"44254","subject":"[PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3","startedAt":"2016-10-08T15:39:20Z","lastAt":"2016-10-10T20:52:50Z","messageCount":4,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"303647","messageId":"c830e99d-4fc9-b001-cce3-38f66dc2c678@web.de","threadId":"44254","inReplyTo":null,"subject":"[PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-08T15:38:47Z","receivedAt":"2016-10-08T15:39:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\ninstead of taking detours through find_unique_abbrev() and its static\nbuffer.  This is shorter in most cases and a bit more efficient.\n\nThe changes here are not easily handled by a semantic patch because\nthey involve removing temporary variables and deconstructing format\nstrings for strbuf_addf().\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n merge-recursive.c |  6 +++---\n pretty.c          | 12 +++++-------\n submodule.c       |  7 +++----\n wt-status.c       | 10 ++++------\n 4 files changed, 15 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5200d5c..9041c2f 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \t\tstrbuf_addf(&o->obuf, \"virtual %s\\n\",\n \t\t\tmerge_remote_util(commit)->name);\n \telse {\n-\t\tstrbuf_addf(&o->obuf, \"%s \",\n-\t\t\tfind_unique_abbrev(commit->object.oid.hash,\n-\t\t\t\tDEFAULT_ABBREV));\n+\t\tstrbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,\n+\t\t\t\t\t DEFAULT_ABBREV);\n+\t\tstrbuf_addch(&o->obuf, ' ');\n \t\tif (parse_commit(commit) != 0)\n \t\t\tstrbuf_addstr(&o->obuf, _(\"(bad commit)\\n\"));\n \t\telse {\ndiff --git a/pretty.c b/pretty.c\nindex 25efbca..0c31495 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -544,15 +544,13 @@ static void add_merge_info(const struct pretty_print_context *pp,\n \tstrbuf_addstr(sb, \"Merge:\");\n \n \twhile (parent) {\n-\t\tstruct commit *p = parent->item;\n-\t\tconst char *hex = NULL;\n+\t\tstruct object_id *oidp = &parent->item->object.oid;\n+\t\tstrbuf_addch(sb, ' ');\n \t\tif (pp->abbrev)\n-\t\t\thex = find_unique_abbrev(p->object.oid.hash, pp->abbrev);\n-\t\tif (!hex)\n-\t\t\thex = oid_to_hex(&p->object.oid);\n+\t\t\tstrbuf_add_unique_abbrev(sb, oidp->hash, pp->abbrev);\n+\t\telse\n+\t\t\tstrbuf_addstr(sb, oid_to_hex(oidp));\n \t\tparent = parent->next;\n-\n-\t\tstrbuf_addf(sb, \" %s\", hex);\n \t}\n \tstrbuf_addch(sb, '\\n');\n }\ndiff --git a/submodule.c b/submodule.c\nindex 2de06a3..476f60b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -392,10 +392,9 @@ static void show_submodule_header(FILE *f, const char *path,\n \t}\n \n output_header:\n-\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\", line_prefix, meta, path,\n-\t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n-\tif (!fast_backward && !fast_forward)\n-\t\tstrbuf_addch(&sb, '.');\n+\tstrbuf_addf(&sb, \"%s%sSubmodule %s \", line_prefix, meta, path);\n+\tstrbuf_add_unique_abbrev(&sb, one->hash, DEFAULT_ABBREV);\n+\tstrbuf_addstr(&sb, (fast_backward || fast_forward) ? \"..\" : \"...\");\n \tstrbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV);\n \tif (message)\n \t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\ndiff --git a/wt-status.c b/wt-status.c\nindex 99d1b0a..ca5c45f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1110,7 +1110,6 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n \tsplit = strbuf_split_max(line, ' ', 3);\n \tif (split[0] && split[1]) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *abbrev;\n \n \t\t/*\n \t\t * strbuf_split_max left a space. Trim it and re-add\n@@ -1118,9 +1117,10 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n \t\t */\n \t\tstrbuf_trim(split[1]);\n \t\tif (!get_sha1(split[1]->buf, sha1)) {\n-\t\t\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n \t\t\tstrbuf_reset(split[1]);\n-\t\t\tstrbuf_addf(split[1], \"%s \", abbrev);\n+\t\t\tstrbuf_add_unique_abbrev(split[1], sha1,\n+\t\t\t\t\t\t DEFAULT_ABBREV);\n+\t\t\tstrbuf_addch(split[1], ' ');\n \t\t\tstrbuf_reset(line);\n \t\t\tfor (i = 0; split[i]; i++)\n \t\t\t\tstrbuf_addbuf(line, split[i]);\n@@ -1343,10 +1343,8 @@ static char *get_branch(const struct worktree *wt, const char *path)\n \telse if (starts_with(sb.buf, \"refs/\"))\n \t\t;\n \telse if (!get_sha1_hex(sb.buf, sha1)) {\n-\t\tconst char *abbrev;\n-\t\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n \t\tstrbuf_reset(&sb);\n-\t\tstrbuf_addstr(&sb, abbrev);\n+\t\tstrbuf_add_unique_abbrev(&sb, sha1, DEFAULT_ABBREV);\n \t} else if (!strcmp(sb.buf, \"detached HEAD\")) /* rebase */\n \t\tgoto got_nothing;\n \telse\t\t\t/* bisect */\n-- \n2.10.1\n\n"},{"id":"303686","messageId":"20161010000035.mfcf55wqfcbcnarh@sigill.intra.peff.net","threadId":"44254","inReplyTo":"c830e99d-4fc9-b001-cce3-38f66dc2c678@web.de","subject":"Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T00:00:35Z","receivedAt":"2016-10-10T00:00:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:\n\n> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\n> instead of taking detours through find_unique_abbrev() and its static\n> buffer.  This is shorter in most cases and a bit more efficient.\n> \n> The changes here are not easily handled by a semantic patch because\n> they involve removing temporary variables and deconstructing format\n> strings for strbuf_addf().\n\nYeah, the thing to look for here is whether the sha1 variable holds the\nsame value at both times.\n\nThese all look OK to me. Mild rambling below.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 5200d5c..9041c2f 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n>  \t\tstrbuf_addf(&o->obuf, \"virtual %s\\n\",\n>  \t\t\tmerge_remote_util(commit)->name);\n>  \telse {\n> -\t\tstrbuf_addf(&o->obuf, \"%s \",\n> -\t\t\tfind_unique_abbrev(commit->object.oid.hash,\n> -\t\t\t\tDEFAULT_ABBREV));\n> +\t\tstrbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,\n> +\t\t\t\t\t DEFAULT_ABBREV);\n> +\t\tstrbuf_addch(&o->obuf, ' ');\n\nI've often wondered whether a big strbuf_addf() is more efficient than\nseveral strbuf_addstrs. It amortizes the size-checks, certainly, though\nthose are probably not very big. It shouldn't matter much for amortizing\nthe cost of malloc, as it's very unlikely to have a case where:\n\n  strbuf_addf(\"%s%s\", foo, bar);\n\nwould require one malloc, but:\n\n  strbuf_addstr(foo);\n  strbuf_addstr(bar);\n\nwould require two (one of the strings would have to be around the same\nsize as the ALLOC_GROW() doubling).\n\nSo it probably doesn't matter much in practice (not that most of these\ncases are very performance sensitive anyway). Mostly just something I've\npondered while tweaking strbuf invocations.\n\nAnd anyway, in this case replacing find_unique_abbrev lets us write\ndirectly into the final buffer rather than copying through a static\nbuffer, so it's almost certainly a win there.\n\n> diff --git a/pretty.c b/pretty.c\n> index 25efbca..0c31495 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -544,15 +544,13 @@ static void add_merge_info(const struct pretty_print_context *pp,\n>  \tstrbuf_addstr(sb, \"Merge:\");\n>  \n>  \twhile (parent) {\n> -\t\tstruct commit *p = parent->item;\n> -\t\tconst char *hex = NULL;\n> +\t\tstruct object_id *oidp = &parent->item->object.oid;\n> +\t\tstrbuf_addch(sb, ' ');\n>  \t\tif (pp->abbrev)\n> -\t\t\thex = find_unique_abbrev(p->object.oid.hash, pp->abbrev);\n> -\t\tif (!hex)\n> -\t\t\thex = oid_to_hex(&p->object.oid);\n\nWow, this existing code was hard to follow. I wondered when\nfind_unique_abbrev() would return NULL, but it never does. This \"if\n(!hex)\" should have been an \"else\" all along.\n\n> +\t\t\tstrbuf_add_unique_abbrev(sb, oidp->hash, pp->abbrev);\n> +\t\telse\n> +\t\t\tstrbuf_addstr(sb, oid_to_hex(oidp));\n\n...which I see you changed here, though perhaps it is not immediately\nobvious that it is correct without knowing find_unique_abbrev's return\nvalue assumptions.\n\nJust thinking aloud, I've also wondered if strbuf_addoid() would be\nhandy.  We already have oid_to_hex_r(). In fact, you could write it\nalready as:\n\n  strbuf_add_unique_abbrev(sb, oidp->hash, 0);\n\nbut that is probably not helping maintainability. ;)\n\n> diff --git a/submodule.c b/submodule.c\n> index 2de06a3..476f60b 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -392,10 +392,9 @@ static void show_submodule_header(FILE *f, const char *path,\n>  \t}\n>  \n>  output_header:\n> -\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\", line_prefix, meta, path,\n> -\t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n> -\tif (!fast_backward && !fast_forward)\n> -\t\tstrbuf_addch(&sb, '.');\n> +\tstrbuf_addf(&sb, \"%s%sSubmodule %s \", line_prefix, meta, path);\n> +\tstrbuf_add_unique_abbrev(&sb, one->hash, DEFAULT_ABBREV);\n> +\tstrbuf_addstr(&sb, (fast_backward || fast_forward) ? \"..\" : \"...\");\n\nSpelling out \"..\" and \"...\" as you do makes this much clearer, IMHO.\n\n-Peff\n"},{"id":"303801","messageId":"af55f6d7-e1b1-272b-4fbe-a6eb2422b3be@web.de","threadId":"44254","inReplyTo":"20161010000035.mfcf55wqfcbcnarh@sigill.intra.peff.net","subject":"Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-10T20:46:21Z","receivedAt":"2016-10-10T20:46:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.10.2016 um 02:00 schrieb Jeff King:\n> On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:\n> \n>> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\n>> instead of taking detours through find_unique_abbrev() and its static\n>> buffer.  This is shorter in most cases and a bit more efficient.\n>>\n>> The changes here are not easily handled by a semantic patch because\n>> they involve removing temporary variables and deconstructing format\n>> strings for strbuf_addf().\n> \n> Yeah, the thing to look for here is whether the sha1 variable holds the\n> same value at both times.\n> \n> These all look OK to me. Mild rambling below.\n> \n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 5200d5c..9041c2f 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n>>  \t\tstrbuf_addf(&o->obuf, \"virtual %s\\n\",\n>>  \t\t\tmerge_remote_util(commit)->name);\n>>  \telse {\n>> -\t\tstrbuf_addf(&o->obuf, \"%s \",\n>> -\t\t\tfind_unique_abbrev(commit->object.oid.hash,\n>> -\t\t\t\tDEFAULT_ABBREV));\n>> +\t\tstrbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,\n>> +\t\t\t\t\t DEFAULT_ABBREV);\n>> +\t\tstrbuf_addch(&o->obuf, ' ');\n> \n> I've often wondered whether a big strbuf_addf() is more efficient than\n> several strbuf_addstrs. It amortizes the size-checks, certainly, though\n> those are probably not very big. It shouldn't matter much for amortizing\n> the cost of malloc, as it's very unlikely to have a case where:\n> \n>   strbuf_addf(\"%s%s\", foo, bar);\n> \n> would require one malloc, but:\n> \n>   strbuf_addstr(foo);\n>   strbuf_addstr(bar);\n> \n> would require two (one of the strings would have to be around the same\n> size as the ALLOC_GROW() doubling).\n> \n> So it probably doesn't matter much in practice (not that most of these\n> cases are very performance sensitive anyway). Mostly just something I've\n> pondered while tweaking strbuf invocations.\n\nGood question.  ALLOC_GROW() doesn't double exactly, but indeed the\nnumber of reallocations depends on the size of the added pieces.  I\nalways thought of strbuf_addf() as an expensive function for\nconvenience, but never timed it.\n\nNumbers vary a bit, but here's what I get for the crude test program\nat the end:\n\n$ time t/helper/test-strbuf strbuf_addf 123 123456789012345678901234567890\n123123456789012345678901234567890\n\nreal    0m0.168s\nuser    0m0.164s\nsys     0m0.000s\n$ time t/helper/test-strbuf strbuf_addstr 123 123456789012345678901234567890\n123123456789012345678901234567890\n\nreal    0m0.141s\nuser    0m0.140s\nsys     0m0.000s\n\nJust a data-point, but it confirms my bias, so I stop here. :)\n\n> Just thinking aloud, I've also wondered if strbuf_addoid() would be\n> handy.  We already have oid_to_hex_r(). In fact, you could write it\n> already as:\n> \n>   strbuf_add_unique_abbrev(sb, oidp->hash, 0);\n> \n> but that is probably not helping maintainability. ;)\n\nYes, a function for adding full hashes without using a static\nvariable is useful for library functions that may end up being\ncalled often or in parallel.  I'd call it strbuf_add_hash,\nthough.\n\n\n\ndiff --git a/Makefile b/Makefile\nindex 1aad150..ad5e98c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -628,6 +628,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sha1-array\n TEST_PROGRAMS_NEED_X += test-sigchain\n+TEST_PROGRAMS_NEED_X += test-strbuf\n TEST_PROGRAMS_NEED_X += test-string-list\n TEST_PROGRAMS_NEED_X += test-submodule-config\n TEST_PROGRAMS_NEED_X += test-subprocess\ndiff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c\nnew file mode 100644\nindex 0000000..177f3e2\n--- /dev/null\n+++ b/t/helper/test-strbuf.c\n@@ -0,0 +1,25 @@\n+#include \"cache.h\"\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint i = 1000000;\n+\n+\tif (argc == 4) {\n+\t\tif (!strcmp(argv[1], \"strbuf_addf\")) {\n+\t\t\twhile (i--) {\n+\t\t\t\tstrbuf_release(&sb);\n+\t\t\t\tstrbuf_addf(&sb, \"%s%s\", argv[2], argv[3]);\n+\t\t\t}\n+\t\t}\n+\t\tif (!strcmp(argv[1], \"strbuf_addstr\")) {\n+\t\t\twhile (i--) {\n+\t\t\t\tstrbuf_release(&sb);\n+\t\t\t\tstrbuf_addstr(&sb, argv[2]);\n+\t\t\t\tstrbuf_addstr(&sb, argv[3]);\n+\t\t\t}\n+\t\t}\n+\t\tputs(sb.buf);\n+\t}\n+\treturn 0;\n+}\n\n"},{"id":"303804","messageId":"20161010205243.fejhhvoos5tmqggh@sigill.intra.peff.net","threadId":"44254","inReplyTo":"af55f6d7-e1b1-272b-4fbe-a6eb2422b3be@web.de","subject":"Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T20:52:44Z","receivedAt":"2016-10-10T20:52:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 10:46:21PM +0200, René Scharfe wrote:\n\n> Good question.  ALLOC_GROW() doesn't double exactly, but indeed the\n> number of reallocations depends on the size of the added pieces.  I\n> always thought of strbuf_addf() as an expensive function for\n> convenience, but never timed it.\n> \n> Numbers vary a bit, but here's what I get for the crude test program\n> at the end:\n> \n> $ time t/helper/test-strbuf strbuf_addf 123 123456789012345678901234567890\n> 123123456789012345678901234567890\n> \n> real    0m0.168s\n> user    0m0.164s\n> sys     0m0.000s\n> $ time t/helper/test-strbuf strbuf_addstr 123 123456789012345678901234567890\n> 123123456789012345678901234567890\n> \n> real    0m0.141s\n> user    0m0.140s\n> sys     0m0.000s\n> \n> Just a data-point, but it confirms my bias, so I stop here. :)\n\nHeh. I'm surprised it's that big a difference, as processing simple\nprintf strings should be pretty quick. I guess what happens in your\nprogram is that your strings almost always require a re-allocation\n(because you've just released, and we start small), and we literally end\nup doing a partial copy via vsnprintf(), realizing we're out of space,\nreallocating, and then running it again.\n\nSo it's noticeably worse when we _do_ reallocate, but usually that\nshould be amortized across many calls (and if it isn't, then you are\npaying the much bigger price of lots of mallocs, and you should optimize\nthat first :) ).\n\nThat being said, it doesn't seem like it would be _worse_ to move from\naddf to multiple addstrs.\n\n-Peff\n"}]}