{"thread":{"id":"55435","subject":"[PATCH] [GSOC] ref-filter: use single strbuf for all output","startedAt":"2021-04-05T14:01:27Z","lastAt":"2021-04-08T15:12:38Z","messageCount":21,"participants":["ZheNing Hu via GitGitGadget","Eric Sunshine","Derrick Stolee","Jeff King","ZheNing Hu","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"420988","messageId":"pull.927.git.1617631280402.gitgitgadget@gmail.com","threadId":"55435","inReplyTo":null,"subject":"[PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-05T14:01:19Z","receivedAt":"2021-04-05T14:01:27Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen we use `git for-each-ref`, every ref will call\n`show_ref_array_item()` and allocate its own final strbuf\nand error strbuf. Instead, we can provide two single strbuf:\nfinal_buf and error_buf that get reused for each output.\n\nWhen run it 100 times:\n\n$ git for-each-ref\n\non git.git :\n\n3.19s user\n3.88s system\n35% cpu\n20.199 total\n\nto:\n\n2.89s user\n4.00s system\n34% cpu\n19.741 total\n\nThe performance has been slightly improved.\n\nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: use single strbuf for all output\n \n This patch learned Jeff King's optimization measures in git\n cat-file(79ed0a5): using a single strbuf for all objects output Instead\n of allocating a large number of small strbuf for every object.\n \n So ref-filter can learn same thing: use single buffer: final_buf and\n error_buf for all refs output.\n \n Thanks.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/927\n\n builtin/for-each-ref.c | 4 +++-\n builtin/tag.c | 4 +++-\n ref-filter.c | 21 ++++++++++++---------\n ref-filter.h | 5 ++++-\n 4 files changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a04606..9dc41f48bfa0 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT('s', \"shell\", &format.quote_style,\n@@ -81,7 +83,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n \tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], &format);\n+\t\tshow_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d403417b5625..8a38b3e2de34 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n \t\t struct ref_format *format)\n {\n \tstruct ref_array array;\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n \tchar *to_free = NULL;\n \tint i;\n \n@@ -64,7 +66,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++)\n-\t\tshow_ref_array_item(array.items[i], format);\n+\t\tshow_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n \tref_array_clear(&array);\n \tfree(to_free);\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex f0bd32f71416..51ff6af64ebc 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,\n }\n \n void show_ref_array_item(struct ref_array_item *info,\n-\t\t\t const struct ref_format *format)\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf)\n {\n-\tstruct strbuf final_buf = STRBUF_INIT;\n-\tstruct strbuf error_buf = STRBUF_INIT;\n \n-\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n-\t\tdie(\"%s\", error_buf.buf);\n-\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n-\tstrbuf_release(&error_buf);\n-\tstrbuf_release(&final_buf);\n+\tif (format_ref_array_item(info, format, final_buf, error_buf))\n+\t\tdie(\"%s\", error_buf->buf);\n+\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n+\tstrbuf_reset(error_buf);\n+\tstrbuf_reset(final_buf);\n \tputchar('\\n');\n }\n \n@@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n \t\t const struct ref_format *format)\n {\n \tstruct ref_array_item *ref_item;\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n+\n \tref_item = new_ref_array_item(name, oid);\n \tref_item->kind = ref_kind_from_refname(name);\n-\tshow_ref_array_item(ref_item, format);\n+\tshow_ref_array_item(ref_item, format, &final_buf, &error_buf);\n \tfree_array_item(ref_item);\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 19ea4c413409..95498c9f4467 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct strbuf *final_buf,\n \t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n+void show_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf);\n /* Parse a single sort specifier and add it to the list */\n void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n /* Callback function for parsing the sort option */\n\nbase-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48\n-- \ngitgitgadget\n"},{"id":"420993","messageId":"CAPig+cRzv3sPHpOY_ZGBu8mGp=gy6E+c9ige3-AHh2DM+YcKjw@mail.gmail.com","threadId":"55435","inReplyTo":"pull.927.git.1617631280402.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-05T17:05:36Z","receivedAt":"2021-04-05T17:05:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 5, 2021 at 10:01 AM ZheNing Hu via GitGitGadget\n wrote:\n> When we use `git for-each-ref`, every ref will call\n> `show_ref_array_item()` and allocate its own final strbuf\n> and error strbuf. Instead, we can provide two single strbuf:\n> final_buf and error_buf that get reused for each output.\n> [...]\n> Signed-off-by: ZheNing Hu \n> ---\n\nWas there a discussion leading up to this change? If so, it may be a\ngood idea to provide a link to it in the mailing list here under the\n\"---\" line.\n\nSome comments below...\n\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> @@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> struct ref_format format = REF_FORMAT_INIT;\n> + struct strbuf final_buf = STRBUF_INIT;\n> + struct strbuf error_buf = STRBUF_INIT;\n>\n> for (i = 0; i < maxcount; i++)\n> - show_ref_array_item(array.items[i], &format);\n> + show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n> ref_array_clear(&array);\n> return 0;\n> }\n\nThe call to strbuf_reset() within show_ref_array_item() does not free\nthe memory from the strbufs, so the memory is being leaked. Therefore,\nat the end of this function, you should have:\n\n strbuf_release(final_buf);\n strbuf_release(error_buf);\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> @@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> struct ref_array array;\n> + struct strbuf final_buf = STRBUF_INIT;\n> + struct strbuf error_buf = STRBUF_INIT;\n>\n> for (i = 0; i < array.nr; i++)\n> - show_ref_array_item(array.items[i], format);\n> + show_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n> ref_array_clear(&array);\n> free(to_free);\n\nLeaking `final_buf` and `error_buf`.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,\n> void show_ref_array_item(struct ref_array_item *info,\n> - const struct ref_format *format)\n> + const struct ref_format *format,\n> + struct strbuf *final_buf,\n> + struct strbuf *error_buf)\n> {\n> - struct strbuf final_buf = STRBUF_INIT;\n> - struct strbuf error_buf = STRBUF_INIT;\n>\n> - if (format_ref_array_item(info, format, &final_buf, &error_buf))\n> - die(\"%s\", error_buf.buf);\n> - fwrite(final_buf.buf, 1, final_buf.len, stdout);\n> - strbuf_release(&error_buf);\n> - strbuf_release(&final_buf);\n> + if (format_ref_array_item(info, format, final_buf, error_buf))\n> + die(\"%s\", error_buf->buf);\n> + fwrite(final_buf->buf, 1, final_buf->len, stdout);\n> + strbuf_reset(error_buf);\n> + strbuf_reset(final_buf);\n> putchar('\\n');\n> }\n\nA couple comments:\n\nIt is especially ugly that `error_buf` needs to be passed in by the\ncaller since it is only ever used in case of an error, at which point\nthe program will die() anyhow, so it's not on a critical,\nspeed-sensitive path. The initialization with STRBUF_INIT is\npractically cost-free, so this variable could easily stay local to\nthis function without cost-penalty rather than forcing the caller to\npass it in. (This assumes that none of the consumers of `error_buf`\ndown the line insert into the buffer unnecessarily, which is probably\na reasonable assumption.)\n\nIt is an unsafe assumption to only call strbuf_reset() at the end of\nthe function. For this to be robust, you can't assume that the caller\nhas given you an empty strbuf. Instead, you must ensure it by calling\nstrbuf_reset() at the start. (It doesn't hurt to also call\nstrbuf_reset() at the end, but that is not critical to correct\noperation, so could be omitted.)\n\n> @@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n> struct ref_array_item *ref_item;\n> + struct strbuf final_buf = STRBUF_INIT;\n> + struct strbuf error_buf = STRBUF_INIT;\n> +\n> ref_item = new_ref_array_item(name, oid);\n> ref_item->kind = ref_kind_from_refname(name);\n> - show_ref_array_item(ref_item, format);\n> + show_ref_array_item(ref_item, format, &final_buf, &error_buf);\n> free_array_item(ref_item);\n> }\n\nLeaking `final_buf` and `error_buf`.\n\n> diff --git a/ref-filter.h b/ref-filter.h\n> @@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,\n> /* Print the ref using the given format and quote_style */\n> -void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> +void show_ref_array_item(struct ref_array_item *info,\n> + const struct ref_format *format,\n> + struct strbuf *final_buf,\n> + struct strbuf *error_buf);\n\nIt is not clear to the person reading this what these two new\narguments are for or what should be provided, so the comment above the\nfunction deserves an update explaining what these arguments are for\nand how to provide them. This is especially important since this is a\npublic function.\n\nIf this function was merely internal to some builtin command, this\nsort of change for the sake of optimization might not be so bad, but\nas a public function, it comes across as rather ugly. In general, we\ndon't necessarily want to pollute an otherwise clean API with changes\nwhich make the API uglier merely for the sake of tiny optimizations\nlike this (IMHO). The extra burden placed on callers by this change,\ncoupled with the ugliness this introduces into the API, makes the\nchange seem less than desirable.\n\nOne way you might be able to mitigate the undesirableness would be to\nhave two versions of this function. For instance:\n\n /* Print the ref using the given format and quote_style */\n show_ref_array_item(...);\n /* This is like show_ref_array_item() but ... */\n show_ref_array_item_optim(...);\n\nThe comment of the second function would, of course, need to explain\nthat it is similar to show_ref_array_item() but more optimal because\n and that the caller is responsible for allocating and\nreleasing the strbufs, and that the strbufs are used only for\ntemporary storage, thus the caller should not assume anything about\nthem.\n\nThis way, callers which don't invoke show_ref_array_item() in a tight\nloop don't need to be burdened by the new arguments (and won't have to\nremember to release them), and callers with a tight loop can take\nadvantage of the optimization with the bit of extra work of having to\ndeclare and release the strbufs.\n\nSo, having said all that, it's not clear that the ugliness and extra\nwork are worth the gain. However, if it is decided that the change is\nworthwhile, then the commit message probably should explain cases in\nwhich such an optimization will be really beneficial.\n"},{"id":"421014","messageId":"50fce64e-d3f5-05e6-5e2a-645864211176@gmail.com","threadId":"55435","inReplyTo":"pull.927.git.1617631280402.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-05T21:02:53Z","receivedAt":"2021-04-05T21:03:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/5/2021 10:01 AM, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> \n> When we use `git for-each-ref`, every ref will call\n> `show_ref_array_item()` and allocate its own final strbuf\n> and error strbuf. Instead, we can provide two single strbuf:\n> final_buf and error_buf that get reused for each output.\n\ns/two single strbuf/two buffers/\n\n> When run it 100 times:\n> \n> $ git for-each-ref\n> \n> on git.git :\n> \n> 3.19s user\n> 3.88s system\n> 35% cpu\n> 20.199 total\n> \n> to:\n> \n> 2.89s user\n> 4.00s system\n> 34% cpu\n> 19.741 total\n> \n> The performance has been slightly improved.\n\nThis is a nice amount of time! Perhaps simplify the\npresentation:\n\n The performance for 'git for-each-ref' on the Git\n repository itself with X references changes from\n 3.2 seconds to 2.9 seconds.\n \n> Signed-off-by: ZheNing Hu \n> ---\n> [GSOC] ref-filter: use single strbuf for all output\n> \n> This patch learned Jeff King's optimization measures in git\n> cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> of allocating a large number of small strbuf for every object.\n\nThis part of the cover letter could be put into the\ncommit message itself. Use the correct formatting,\nthough:\n\n This approach is similar to the one used by 79ed0a5\n (cat-file: use a single strbuf for all output, 2018-08-14)\n to speed up the cat-file builtin.\n\nI found the code change to be correct. Thanks!\n-Stolee\n"},{"id":"421019","messageId":"YGuMaxoYgRkUR1sa@coredump.intra.peff.net","threadId":"55435","inReplyTo":"pull.927.git.1617631280402.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-05T22:17:15Z","receivedAt":"2021-04-05T22:17:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 05, 2021 at 02:01:19PM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> When we use `git for-each-ref`, every ref will call\n> `show_ref_array_item()` and allocate its own final strbuf\n> and error strbuf. Instead, we can provide two single strbuf:\n> final_buf and error_buf that get reused for each output.\n> \n> When run it 100 times:\n> \n> $ git for-each-ref\n> \n> on git.git :\n> \n> 3.19s user\n> 3.88s system\n> 35% cpu\n> 20.199 total\n> \n> to:\n> \n> 2.89s user\n> 4.00s system\n> 34% cpu\n> 19.741 total\n\nThat's a bigger performance improvement than I'd expect from this. I'm\nhaving trouble reproducing it here (I get the same time before and\nafter). I also notice that you don't seem to be CPU bound, and we spend\nmost of our time on system CPU (so program startup stuff, not the loop\nyou're optimizing).\n\nI think a more interesting test is timing a single invocation with a\nlarge number of refs. If you are planning to do a lot of work on the\nformatting code, it might be worth adding such a test into t/perf (both\nto show off results, but also to catch any regressions after we make\nthings faster).\n\n> This patch learned Jeff King's optimization measures in git\n> cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> of allocating a large number of small strbuf for every object.\n\nI do think this is a good direction (for all the reasons laid out in\n79ed0a5), though it wasn't actually the part I was most worried about\nfor ref-filter performance. The bigger issue in ref-filter is that each\nindividual atom will generally have its own allocated string, and then\nwe'll format all of the atoms into the final strbuf output. In most\ncases we could avoid those intermediate copies entirely.\n\nI do think this would be a useful optimization to have in addition,\nthough. As for the patch itself, I looked over the review that Eric\ngave, and he already said everything I would have. :)\n\n-Peff\n"},{"id":"421047","messageId":"CAOLTT8SoRPbcqCM33RkqQ0_rWmax7aAati0Q7L7x9JGBcVjPzA@mail.gmail.com","threadId":"55435","inReplyTo":"CAPig+cRzv3sPHpOY_ZGBu8mGp=gy6E+c9ige3-AHh2DM+YcKjw@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-06T08:53:15Z","receivedAt":"2021-04-06T08:53:33Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Eric Sunshine 于2021年4月6日周二 上午1:05写道：\n>\n> On Mon, Apr 5, 2021 at 10:01 AM ZheNing Hu via GitGitGadget\n> wrote:\n> > When we use `git for-each-ref`, every ref will call\n> > `show_ref_array_item()` and allocate its own final strbuf\n> > and error strbuf. Instead, we can provide two single strbuf:\n> > final_buf and error_buf that get reused for each output.\n> > [...]\n> > Signed-off-by: ZheNing Hu \n> > ---\n>\n> Was there a discussion leading up to this change? If so, it may be a\n> good idea to provide a link to it in the mailing list here under the\n> \"---\" line.\n>\n\nOkay, I will add them in cover-letter next time.\n\n> Some comments below...\n>\n> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> > @@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > struct ref_format format = REF_FORMAT_INIT;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> >\n> > for (i = 0; i < maxcount; i++)\n> > - show_ref_array_item(array.items[i], &format);\n> > + show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n> > ref_array_clear(&array);\n> > return 0;\n> > }\n>\n> The call to strbuf_reset() within show_ref_array_item() does not free\n> the memory from the strbufs, so the memory is being leaked. Therefore,\n> at the end of this function, you should have:\n>\n> strbuf_release(final_buf);\n> strbuf_release(error_buf);\n>\n\nThanks, I ignored this point.\n\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > @@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> > struct ref_array array;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> >\n> > for (i = 0; i < array.nr; i++)\n> > - show_ref_array_item(array.items[i], format);\n> > + show_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n> > ref_array_clear(&array);\n> > free(to_free);\n>\n> Leaking `final_buf` and `error_buf`.\n>\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > @@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,\n> > void show_ref_array_item(struct ref_array_item *info,\n> > - const struct ref_format *format)\n> > + const struct ref_format *format,\n> > + struct strbuf *final_buf,\n> > + struct strbuf *error_buf)\n> > {\n> > - struct strbuf final_buf = STRBUF_INIT;\n> > - struct strbuf error_buf = STRBUF_INIT;\n> >\n> > - if (format_ref_array_item(info, format, &final_buf, &error_buf))\n> > - die(\"%s\", error_buf.buf);\n> > - fwrite(final_buf.buf, 1, final_buf.len, stdout);\n> > - strbuf_release(&error_buf);\n> > - strbuf_release(&final_buf);\n> > + if (format_ref_array_item(info, format, final_buf, error_buf))\n> > + die(\"%s\", error_buf->buf);\n> > + fwrite(final_buf->buf, 1, final_buf->len, stdout);\n> > + strbuf_reset(error_buf);\n> > + strbuf_reset(final_buf);\n> > putchar('\\n');\n> > }\n>\n> A couple comments:\n>\n> It is especially ugly that `error_buf` needs to be passed in by the\n> caller since it is only ever used in case of an error, at which point\n> the program will die() anyhow, so it's not on a critical,\n> speed-sensitive path. The initialization with STRBUF_INIT is\n> practically cost-free, so this variable could easily stay local to\n> this function without cost-penalty rather than forcing the caller to\n> pass it in. (This assumes that none of the consumers of `error_buf`\n> down the line insert into the buffer unnecessarily, which is probably\n> a reasonable assumption.)\n>\n\nWhat you said makes sense. The `error_buf` may not need to be passed\nas a parameter, because errors are generally less.\n\n> It is an unsafe assumption to only call strbuf_reset() at the end of\n> the function. For this to be robust, you can't assume that the caller\n> has given you an empty strbuf. Instead, you must ensure it by calling\n> strbuf_reset() at the start. (It doesn't hurt to also call\n> strbuf_reset() at the end, but that is not critical to correct\n> operation, so could be omitted.)\n>\n\nWell, indeed, it would be better to use `strbuf_reset()` first, as it do in\n`cat-file.c`.\n\n> > @@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n> > struct ref_array_item *ref_item;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> > +\n> > ref_item = new_ref_array_item(name, oid);\n> > ref_item->kind = ref_kind_from_refname(name);\n> > - show_ref_array_item(ref_item, format);\n> > + show_ref_array_item(ref_item, format, &final_buf, &error_buf);\n> > free_array_item(ref_item);\n> > }\n>\n> Leaking `final_buf` and `error_buf`.\n>\n> > diff --git a/ref-filter.h b/ref-filter.h\n> > @@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,\n> > /* Print the ref using the given format and quote_style */\n> > -void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> > +void show_ref_array_item(struct ref_array_item *info,\n> > + const struct ref_format *format,\n> > + struct strbuf *final_buf,\n> > + struct strbuf *error_buf);\n>\n> It is not clear to the person reading this what these two new\n> arguments are for or what should be provided, so the comment above the\n> function deserves an update explaining what these arguments are for\n> and how to provide them. This is especially important since this is a\n> public function.\n>\n> If this function was merely internal to some builtin command, this\n> sort of change for the sake of optimization might not be so bad, but\n> as a public function, it comes across as rather ugly. In general, we\n> don't necessarily want to pollute an otherwise clean API with changes\n> which make the API uglier merely for the sake of tiny optimizations\n> like this (IMHO). The extra burden placed on callers by this change,\n> coupled with the ugliness this introduces into the API, makes the\n> change seem less than desirable.\n>\n\nWell, for the time being, there are relatively few places\nwhere`show_ref_array_item()`\nis used in the entire git repository. I may need to pay attention to this later:\nthe ease of use of public interfaces is also important.\n\n> One way you might be able to mitigate the undesirableness would be to\n> have two versions of this function. For instance:\n>\n> /* Print the ref using the given format and quote_style */\n> show_ref_array_item(...);\n> /* This is like show_ref_array_item() but ... */\n> show_ref_array_item_optim(...);\n>\n> The comment of the second function would, of course, need to explain\n> that it is similar to show_ref_array_item() but more optimal because\n> and that the caller is responsible for allocating and\n> releasing the strbufs, and that the strbufs are used only for\n> temporary storage, thus the caller should not assume anything about\n> them.\n>\n\nYes, this will ensure that this new public interface will not be misused.\n\n> This way, callers which don't invoke show_ref_array_item() in a tight\n> loop don't need to be burdened by the new arguments (and won't have to\n> remember to release them), and callers with a tight loop can take\n> advantage of the optimization with the bit of extra work of having to\n> declare and release the strbufs.\n>\n\nFor external calls, reasonable release of strbuf does require attention,\nwhich is indeed a disadvantage at some time.\n\n> So, having said all that, it's not clear that the ugliness and extra\n> work are worth the gain. However, if it is decided that the change is\n> worthwhile, then the commit message probably should explain cases in\n> which such an optimization will be really beneficial.\n\nI now estimate that the optimization brought here may appear in a more refs\ngit repo like `linux.git`. I have to experiment first.\n\nThanks, Eric.\n\n--\nZheNing Hu\n"},{"id":"421048","messageId":"CAOLTT8RB-Ky70Z_JOEAbKSpRwmvNwF9ecikD4ThOorZ=Ne-LTg@mail.gmail.com","threadId":"55435","inReplyTo":"50fce64e-d3f5-05e6-5e2a-645864211176@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-06T08:58:58Z","receivedAt":"2021-04-06T08:59:14Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Derrick Stolee 于2021年4月6日周二 上午5:02写道：\n>\n> On 4/5/2021 10:01 AM, ZheNing Hu via GitGitGadget wrote:\n> > From: ZheNing Hu \n> >\n> > When we use `git for-each-ref`, every ref will call\n> > `show_ref_array_item()` and allocate its own final strbuf\n> > and error strbuf. Instead, we can provide two single strbuf:\n> > final_buf and error_buf that get reused for each output.\n>\n> s/two single strbuf/two buffers/\n>\n> > When run it 100 times:\n> >\n> > $ git for-each-ref\n> >\n> > on git.git :\n> >\n> > 3.19s user\n> > 3.88s system\n> > 35% cpu\n> > 20.199 total\n> >\n> > to:\n> >\n> > 2.89s user\n> > 4.00s system\n> > 34% cpu\n> > 19.741 total\n> >\n> > The performance has been slightly improved.\n>\n> This is a nice amount of time! Perhaps simplify the\n> presentation:\n>\n\nThanks! But now it may still need more experimental examples\nto prove that this optimization is visible.\n\n> The performance for 'git for-each-ref' on the Git\n> repository itself with X references changes from\n> 3.2 seconds to 2.9 seconds.\n>\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > [GSOC] ref-filter: use single strbuf for all output\n> >\n> > This patch learned Jeff King's optimization measures in git\n> > cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> > of allocating a large number of small strbuf for every object.\n>\n> This part of the cover letter could be put into the\n> commit message itself. Use the correct formatting,\n> though:\n>\n> This approach is similar to the one used by 79ed0a5\n> (cat-file: use a single strbuf for all output, 2018-08-14)\n> to speed up the cat-file builtin.\n>\n\nOK, I will do it.\n\n> I found the code change to be correct. Thanks!\n> -Stolee\n\nThanks.\n--\nZheNing Hu\n"},{"id":"421050","messageId":"CAOLTT8TwjRyT6MK_ekEx9APBv7jn17JRKj=mJQMO5Sk-DgHA-A@mail.gmail.com","threadId":"55435","inReplyTo":"YGuMaxoYgRkUR1sa@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-06T09:49:47Z","receivedAt":"2021-04-06T09:50:05Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年4月6日周二 上午6:17写道：\n>\n> On Mon, Apr 05, 2021 at 02:01:19PM +0000, ZheNing Hu via GitGitGadget wrote:\n>\n> > When we use `git for-each-ref`, every ref will call\n> > `show_ref_array_item()` and allocate its own final strbuf\n> > and error strbuf. Instead, we can provide two single strbuf:\n> > final_buf and error_buf that get reused for each output.\n> >\n> > When run it 100 times:\n> >\n> > $ git for-each-ref\n> >\n> > on git.git :\n> >\n> > 3.19s user\n> > 3.88s system\n> > 35% cpu\n> > 20.199 total\n> >\n> > to:\n> >\n> > 2.89s user\n> > 4.00s system\n> > 34% cpu\n> > 19.741 total\n>\n> That's a bigger performance improvement than I'd expect from this. I'm\n> having trouble reproducing it here (I get the same time before and\n> after). I also notice that you don't seem to be CPU bound, and we spend\n> most of our time on system CPU (so program startup stuff, not the loop\n> you're optimizing).\n>\n> I think a more interesting test is timing a single invocation with a\n> large number of refs. If you are planning to do a lot of work on the\n> formatting code, it might be worth adding such a test into t/perf (both\n> to show off results, but also to catch any regressions after we make\n> things faster).\n>\n\nIt makes sense. A lot of refs can be convincing. Just like the number of\nobjects measured in `cat-files` is large enough.\n\nBut this is the first time I use `t/perf/*` and there is a little problem.\nIt seem like whatever I run single script like `sh ./p0007-write-cache.sh`\nor just `make` or `./run ${HOME}/git -- ./p0002-read-cache.sh` , these\ntests will fail.\n\n> > This patch learned Jeff King's optimization measures in git\n> > cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> > of allocating a large number of small strbuf for every object.\n>\n> I do think this is a good direction (for all the reasons laid out in\n> 79ed0a5), though it wasn't actually the part I was most worried about\n> for ref-filter performance. The bigger issue in ref-filter is that each\n> individual atom will generally have its own allocated string, and then\n> we'll format all of the atoms into the final strbuf output. In most\n> cases we could avoid those intermediate copies entirely.\n>\n\nYes! In `ref-filter` we set object info in `v->s` and then append them to\ncurrent `stack` buffer, and finally set in `final_buf`, the copy of the string\nis expensive. I don’t know if the optimization should start by removing the\nstack buffer?\n\n> I do think this would be a useful optimization to have in addition,\n> though. As for the patch itself, I looked over the review that Eric\n> gave, and he already said everything I would have. :)\n>\n\nI think it should be optimized, It will reduce the overhead of malloc and\nfree, but it is not obvious enough.\n\nYes, there is a lot of bad code in my patch.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"421053","messageId":"CAOLTT8SzDsoScFaGe5emQc6V5QDJD=osdE5OMAz2Xs8=AvrfVQ@mail.gmail.com","threadId":"55435","inReplyTo":"CAOLTT8TwjRyT6MK_ekEx9APBv7jn17JRKj=mJQMO5Sk-DgHA-A@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-06T10:35:57Z","receivedAt":"2021-04-06T10:36:17Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu 于2021年4月6日周二 下午5:49写道：\n> But this is the first time I use `t/perf/*` and there is a little problem.\n> It seem like whatever I run single script like `sh ./p0007-write-cache.sh`\n> or just `make` or `./run ${HOME}/git -- ./p0002-read-cache.sh` , these\n> tests will fail.\n>\nIt's because I don't have /usr/bin/time, solved after installation.\nSo best have this:\n\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -152,6 +152,10 @@ immediate=t\n # Perf tests require GNU time\n case \"$(uname -s)\" in Darwin) GTIME=\"${GTIME:-gtime}\";; esac\n GTIME=\"${GTIME:-/usr/bin/time}\"\n+if ! test -f \"$GTIME\"\n+then\n+ error \"command not found: \"$GTIME\"\"\n+fi\n\nThanks.\n--\nZheNing Hu\n"},{"id":"421062","messageId":"YGxpZ/vJ/MyS0tLB@coredump.intra.peff.net","threadId":"55435","inReplyTo":"CAOLTT8SzDsoScFaGe5emQc6V5QDJD=osdE5OMAz2Xs8=AvrfVQ@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-06T14:00:07Z","receivedAt":"2021-04-06T14:00:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 06, 2021 at 06:35:57PM +0800, ZheNing Hu wrote:\n\n> ZheNing Hu 于2021年4月6日周二 下午5:49写道：\n> > But this is the first time I use `t/perf/*` and there is a little problem.\n> > It seem like whatever I run single script like `sh ./p0007-write-cache.sh`\n> > or just `make` or `./run ${HOME}/git -- ./p0002-read-cache.sh` , these\n> > tests will fail.\n> >\n> It's because I don't have /usr/bin/time, solved after installation.\n> So best have this:\n> \n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -152,6 +152,10 @@ immediate=t\n> # Perf tests require GNU time\n> case \"$(uname -s)\" in Darwin) GTIME=\"${GTIME:-gtime}\";; esac\n> GTIME=\"${GTIME:-/usr/bin/time}\"\n> +if ! test -f \"$GTIME\"\n> +then\n> + error \"command not found: \"$GTIME\"\"\n> +fi\n\nThis patch would create problems when we expect to find the value of\n$GTIME in the $PATH e.g., you can see in the Darwin case it is set to\njust \"gtime\", not an absolute path).\n\nI am sympathetic to helping people see what's wrong, but I think in this\ncase we're better off pointing people to using \"-v\". E.g.:\n\n $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh \n perf 1 - rev-list --all:\n not ok 1 - rev-list --all\n #\t\n #\t\tgit rev-list --all >/dev/null\n #\t\n\nUh oh, that wasn't very informative. But how about this:\n\n $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh -v\n [...]\n perf 1 - rev-list --all:\n running: \n \tgit rev-list --all >/dev/null\n \n ./p0001-rev-list.sh: 160: pretend-we-do-not-have-gtime: not found\n not ok 1 - rev-list --all\n #\t\n #\t\tgit rev-list --all >/dev/null\n #\t\n\nwhich I think makes it reasonably clear.\n\n-Peff\n"},{"id":"421067","messageId":"CAOLTT8Q5kQh+8M3Ws4zjyid1+m3orBkootObxd0N2B6p8yNukg@mail.gmail.com","threadId":"55435","inReplyTo":"YGxpZ/vJ/MyS0tLB@coredump.intra.peff.net","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-06T14:35:17Z","receivedAt":"2021-04-06T14:35:37Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年4月6日周二 下午10:00写道：\n>\n> On Tue, Apr 06, 2021 at 06:35:57PM +0800, ZheNing Hu wrote:\n>\n> > ZheNing Hu 于2021年4月6日周二 下午5:49写道：\n> > > But this is the first time I use `t/perf/*` and there is a little problem.\n> > > It seem like whatever I run single script like `sh ./p0007-write-cache.sh`\n> > > or just `make` or `./run ${HOME}/git -- ./p0002-read-cache.sh` , these\n> > > tests will fail.\n> > >\n> > It's because I don't have /usr/bin/time, solved after installation.\n> > So best have this:\n> >\n> > --- a/t/perf/perf-lib.sh\n> > +++ b/t/perf/perf-lib.sh\n> > @@ -152,6 +152,10 @@ immediate=t\n> > # Perf tests require GNU time\n> > case \"$(uname -s)\" in Darwin) GTIME=\"${GTIME:-gtime}\";; esac\n> > GTIME=\"${GTIME:-/usr/bin/time}\"\n> > +if ! test -f \"$GTIME\"\n> > +then\n> > + error \"command not found: \"$GTIME\"\"\n> > +fi\n>\n> This patch would create problems when we expect to find the value of\n> $GTIME in the $PATH e.g., you can see in the Darwin case it is set to\n> just \"gtime\", not an absolute path).\n>\n> I am sympathetic to helping people see what's wrong, but I think in this\n> case we're better off pointing people to using \"-v\". E.g.:\n>\n> $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh\n> perf 1 - rev-list --all:\n> not ok 1 - rev-list --all\n> #\n> # git rev-list --all >/dev/null\n> #\n>\n> Uh oh, that wasn't very informative. But how about this:\n>\n> $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh -v\n> [...]\n> perf 1 - rev-list --all:\n> running:\n> git rev-list --all >/dev/null\n>\n> ./p0001-rev-list.sh: 160: pretend-we-do-not-have-gtime: not found\n> not ok 1 - rev-list --all\n> #\n> # git rev-list --all >/dev/null\n> #\n>\n> which I think makes it reasonably clear.\n>\n> -Peff\n\nI just make a small suggestion. ;)\nYou are right, \"-v\" is enough.\n\nIn addition, I found that the performance was basically\nunchanged after testing. It seems that this optimization is\nindeed too small, not as practical as in `cat-file`.\n\nThis shows that the performance bottleneck of `ref-filter`\nlies elsewhere. E.g. you mentioned \"intermediate copies\".\n\n--\nZheNing Hu\n"},{"id":"421081","messageId":"c70a7c17-650a-ae4d-9a90-66c3511f8371@web.de","threadId":"55435","inReplyTo":"pull.927.git.1617631280402.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-04-06T18:34:21Z","receivedAt":"2021-04-06T18:34:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.21 um 16:01 schrieb ZheNing Hu via GitGitGadget:\n> From: ZheNing Hu \n>\n> When we use `git for-each-ref`, every ref will call\n> `show_ref_array_item()` and allocate its own final strbuf\n> and error strbuf. Instead, we can provide two single strbuf:\n> final_buf and error_buf that get reused for each output.\n>\n> When run it 100 times:\n>\n> $ git for-each-ref\n>\n> on git.git :\n>\n> 3.19s user\n> 3.88s system\n> 35% cpu\n> 20.199 total\n>\n> to:\n>\n> 2.89s user\n> 4.00s system\n> 34% cpu\n> 19.741 total\n>\n> The performance has been slightly improved.\n\nI like to use hyperfine (https://github.com/sharkdp/hyperfine) to get\nmore stable benchmark numbers, incl. standard deviation. With three\nwarmup runs I get the following results for running git for-each-ref on\nGit's own repo with the current master (2e36527f23):\n\n Benchmark #1: ./git for-each-ref\n Time (mean ± σ): 18.8 ms ± 0.3 ms [User: 12.7 ms, System: 5.6 ms]\n Range (min … max): 18.2 ms … 19.8 ms 148 runs\n\nWith your patch on top I get this:\n\n Benchmark #1: ./git for-each-ref\n Time (mean ± σ): 18.5 ms ± 0.4 ms [User: 12.3 ms, System: 5.6 ms]\n Range (min … max): 17.8 ms … 19.6 ms 147 runs\n\nSo there seems to be a slight improvement here, but it is within the\nnoise.\n\nI'm quite surprised how much longer this takes on your machine, however,\nand (like Peff already mentioned) how much of the total time it spends\nin system calls. Is an antivirus program or similar interferring? Or\nsome kind of emulator or similar, e.g. Valgrind? Or has it been a long\ntime since you ran \"git gc\"?\n\nThe benchmark certainly depends on the number of local and remote\nbranches in the repo; my copy currently has 4304 according to\n\"git for-each-ref | wc -l\".\n\n>\n> Signed-off-by: ZheNing Hu \n> ---\n> [GSOC] ref-filter: use single strbuf for all output\n>\n> This patch learned Jeff King's optimization measures in git\n> cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> of allocating a large number of small strbuf for every object.\n>\n> So ref-filter can learn same thing: use single buffer: final_buf and\n> error_buf for all refs output.\n>\n> Thanks.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/927\n>\n> builtin/for-each-ref.c | 4 +++-\n> builtin/tag.c | 4 +++-\n> ref-filter.c | 21 ++++++++++++---------\n> ref-filter.h | 5 ++++-\n> 4 files changed, 22 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index cb9c81a04606..9dc41f48bfa0 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tstruct ref_array array;\n> \tstruct ref_filter filter;\n> \tstruct ref_format format = REF_FORMAT_INIT;\n> +\tstruct strbuf final_buf = STRBUF_INIT;\n> +\tstruct strbuf error_buf = STRBUF_INIT;\n>\n> \tstruct option opts[] = {\n> \t\tOPT_BIT('s', \"shell\", &format.quote_style,\n> @@ -81,7 +83,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \tif (!maxcount || array.nr < maxcount)\n> \t\tmaxcount = array.nr;\n> \tfor (i = 0; i < maxcount; i++)\n> -\t\tshow_ref_array_item(array.items[i], &format);\n> +\t\tshow_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n\nThis user of show_ref_array_item() calls it in a loop on an array.\n\n> \tref_array_clear(&array);\n> \treturn 0;\n> }\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index d403417b5625..8a38b3e2de34 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> \t\t struct ref_format *format)\n> {\n> \tstruct ref_array array;\n> +\tstruct strbuf final_buf = STRBUF_INIT;\n> +\tstruct strbuf error_buf = STRBUF_INIT;\n> \tchar *to_free = NULL;\n> \tint i;\n>\n> @@ -64,7 +66,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> \tref_array_sort(sorting, &array);\n>\n> \tfor (i = 0; i < array.nr; i++)\n> -\t\tshow_ref_array_item(array.items[i], format);\n> +\t\tshow_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n\nDito.\n\n> \tref_array_clear(&array);\n> \tfree(to_free);\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f0bd32f71416..51ff6af64ebc 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,\n> }\n>\n> void show_ref_array_item(struct ref_array_item *info,\n> -\t\t\t const struct ref_format *format)\n> +\t\t\t const struct ref_format *format,\n> +\t\t\t struct strbuf *final_buf,\n> +\t\t\t struct strbuf *error_buf)\n> {\n> -\tstruct strbuf final_buf = STRBUF_INIT;\n> -\tstruct strbuf error_buf = STRBUF_INIT;\n>\n> -\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n> -\t\tdie(\"%s\", error_buf.buf);\n> -\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n> -\tstrbuf_release(&error_buf);\n> -\tstrbuf_release(&final_buf);\n> +\tif (format_ref_array_item(info, format, final_buf, error_buf))\n> +\t\tdie(\"%s\", error_buf->buf);\n> +\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n> +\tstrbuf_reset(error_buf);\n> +\tstrbuf_reset(final_buf);\n> \tputchar('\\n');\n> }\n>\n> @@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n> \t\t const struct ref_format *format)\n> {\n> \tstruct ref_array_item *ref_item;\n> +\tstruct strbuf final_buf = STRBUF_INIT;\n> +\tstruct strbuf error_buf = STRBUF_INIT;\n> +\n> \tref_item = new_ref_array_item(name, oid);\n> \tref_item->kind = ref_kind_from_refname(name);\n> -\tshow_ref_array_item(ref_item, format);\n> +\tshow_ref_array_item(ref_item, format, &final_buf, &error_buf);\n\nThis third and final caller works with a single item; there is no loop.\n\n> \tfree_array_item(ref_item);\n> }\n>\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 19ea4c413409..95498c9f4467 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,\n> \t\t\t struct strbuf *final_buf,\n> \t\t\t struct strbuf *error_buf);\n> /* Print the ref using the given format and quote_style */\n> -void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> +void show_ref_array_item(struct ref_array_item *info,\n> +\t\t\t const struct ref_format *format,\n> +\t\t\t struct strbuf *final_buf,\n> +\t\t\t struct strbuf *error_buf);\n\nThis bring-your-own-buffer approach pushes responsibilities back to\nthe callers, in exchange for improved performance. The number of\nusers of this interface is low, so that's defensible. But that added\neffort is also non-trivial -- as you demonstrated by leaking the\nallocated memory. ;-)\n\nHow about offering to do more instead? In particular you could add\na count parameter and have show_ref_array_item() handle an array of\nstruct ref_array_item objects. It could reuse the buffers internally\nto get the same performance benefit, and would free callers from\nhaving to iterate loops themselves. Something like:\n\n\tvoid show_ref_array_items(struct ref_array_item **info,\n\t\t\t\t size_t n,\n\t\t\t\t const struct ref_format *format);\n\nCallers that deal with a single element can pass n = 1.\n\nPerhaps the \"format\" parameter should go first, like with printf.\n\nThe double reference in \"**info\" is a bit ugly, though (array of\npointers instead of a simple array of objects). That's dictated\nby struct ref_array_item containing a flexible array member, which\nseems to be hard to change.\n\n> /* Parse a single sort specifier and add it to the list */\n> void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n> /* Callback function for parsing the sort option */\n>\n> base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48\n>\n\n"},{"id":"421122","messageId":"CAOLTT8S4-ZAjU5qcfep8-bbw+BNM3f-khMXJvQP+an3H6emp8g@mail.gmail.com","threadId":"55435","inReplyTo":"c70a7c17-650a-ae4d-9a90-66c3511f8371@web.de","subject":"Re: [PATCH] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-07T13:57:00Z","receivedAt":"2021-04-07T13:57:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"René Scharfe 于2021年4月7日周三 上午2:34写道：\n>\n> Am 05.04.21 um 16:01 schrieb ZheNing Hu via GitGitGadget:\n> > From: ZheNing Hu \n> >\n> > When we use `git for-each-ref`, every ref will call\n> > `show_ref_array_item()` and allocate its own final strbuf\n> > and error strbuf. Instead, we can provide two single strbuf:\n> > final_buf and error_buf that get reused for each output.\n> >\n> > When run it 100 times:\n> >\n> > $ git for-each-ref\n> >\n> > on git.git :\n> >\n> > 3.19s user\n> > 3.88s system\n> > 35% cpu\n> > 20.199 total\n> >\n> > to:\n> >\n> > 2.89s user\n> > 4.00s system\n> > 34% cpu\n> > 19.741 total\n> >\n> > The performance has been slightly improved.\n>\n> I like to use hyperfine (https://github.com/sharkdp/hyperfine) to get\n> more stable benchmark numbers, incl. standard deviation. With three\n> warmup runs I get the following results for running git for-each-ref on\n> Git's own repo with the current master (2e36527f23):\n>\n\nYes, hyperfine is really easy to use!\n\n> Benchmark #1: ./git for-each-ref\n> Time (mean ± σ): 18.8 ms ± 0.3 ms [User: 12.7 ms, System: 5.6 ms]\n> Range (min … max): 18.2 ms … 19.8 ms 148 runs\n>\n> With your patch on top I get this:\n>\n> Benchmark #1: ./git for-each-ref\n> Time (mean ± σ): 18.5 ms ± 0.4 ms [User: 12.3 ms, System: 5.6 ms]\n> Range (min … max): 17.8 ms … 19.6 ms 147 runs\n>\n> So there seems to be a slight improvement here, but it is within the\n> noise.\n>\n\nYeah. I meet same noise when I do such test.\n\n> I'm quite surprised how much longer this takes on your machine, however,\n> and (like Peff already mentioned) how much of the total time it spends\n> in system calls. Is an antivirus program or similar interferring? Or\n> some kind of emulator or similar, e.g. Valgrind? Or has it been a long\n> time since you ran \"git gc\"?\n>\n\nYes, I haven't used `git gc` for a long time.\nIn addition, when I did the test before, I ran the network proxy software,\nso there have a bit notice.\n\n> The benchmark certainly depends on the number of local and remote\n> branches in the repo; my copy currently has 4304 according to\n> \"git for-each-ref | wc -l\".\n>\n\nYes i understand this point.\nBut In my git.git, the result of \"git for-each-ref | wc -l\" is 8716 refs.\n\n> >\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > [GSOC] ref-filter: use single strbuf for all output\n> >\n> > This patch learned Jeff King's optimization measures in git\n> > cat-file(79ed0a5): using a single strbuf for all objects output Instead\n> > of allocating a large number of small strbuf for every object.\n> >\n> > So ref-filter can learn same thing: use single buffer: final_buf and\n> > error_buf for all refs output.\n> >\n> > Thanks.\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/927\n> >\n> > builtin/for-each-ref.c | 4 +++-\n> > builtin/tag.c | 4 +++-\n> > ref-filter.c | 21 ++++++++++++---------\n> > ref-filter.h | 5 ++++-\n> > 4 files changed, 22 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> > index cb9c81a04606..9dc41f48bfa0 100644\n> > --- a/builtin/for-each-ref.c\n> > +++ b/builtin/for-each-ref.c\n> > @@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > struct ref_array array;\n> > struct ref_filter filter;\n> > struct ref_format format = REF_FORMAT_INIT;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> >\n> > struct option opts[] = {\n> > OPT_BIT('s', \"shell\", &format.quote_style,\n> > @@ -81,7 +83,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > if (!maxcount || array.nr < maxcount)\n> > maxcount = array.nr;\n> > for (i = 0; i < maxcount; i++)\n> > - show_ref_array_item(array.items[i], &format);\n> > + show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n>\n> This user of show_ref_array_item() calls it in a loop on an array.\n>\n> > ref_array_clear(&array);\n> > return 0;\n> > }\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index d403417b5625..8a38b3e2de34 100644\n> > --- a/builtin/tag.c\n> > +++ b/builtin/tag.c\n> > @@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> > struct ref_format *format)\n> > {\n> > struct ref_array array;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> > char *to_free = NULL;\n> > int i;\n> >\n> > @@ -64,7 +66,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n> > ref_array_sort(sorting, &array);\n> >\n> > for (i = 0; i < array.nr; i++)\n> > - show_ref_array_item(array.items[i], format);\n> > + show_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n>\n> Dito.\n>\n> > ref_array_clear(&array);\n> > free(to_free);\n> >\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index f0bd32f71416..51ff6af64ebc 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,\n> > }\n> >\n> > void show_ref_array_item(struct ref_array_item *info,\n> > - const struct ref_format *format)\n> > + const struct ref_format *format,\n> > + struct strbuf *final_buf,\n> > + struct strbuf *error_buf)\n> > {\n> > - struct strbuf final_buf = STRBUF_INIT;\n> > - struct strbuf error_buf = STRBUF_INIT;\n> >\n> > - if (format_ref_array_item(info, format, &final_buf, &error_buf))\n> > - die(\"%s\", error_buf.buf);\n> > - fwrite(final_buf.buf, 1, final_buf.len, stdout);\n> > - strbuf_release(&error_buf);\n> > - strbuf_release(&final_buf);\n> > + if (format_ref_array_item(info, format, final_buf, error_buf))\n> > + die(\"%s\", error_buf->buf);\n> > + fwrite(final_buf->buf, 1, final_buf->len, stdout);\n> > + strbuf_reset(error_buf);\n> > + strbuf_reset(final_buf);\n> > putchar('\\n');\n> > }\n> >\n> > @@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n> > const struct ref_format *format)\n> > {\n> > struct ref_array_item *ref_item;\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> > +\n> > ref_item = new_ref_array_item(name, oid);\n> > ref_item->kind = ref_kind_from_refname(name);\n> > - show_ref_array_item(ref_item, format);\n> > + show_ref_array_item(ref_item, format, &final_buf, &error_buf);\n>\n> This third and final caller works with a single item; there is no loop.\n>\n> > free_array_item(ref_item);\n> > }\n> >\n> > diff --git a/ref-filter.h b/ref-filter.h\n> > index 19ea4c413409..95498c9f4467 100644\n> > --- a/ref-filter.h\n> > +++ b/ref-filter.h\n> > @@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,\n> > struct strbuf *final_buf,\n> > struct strbuf *error_buf);\n> > /* Print the ref using the given format and quote_style */\n> > -void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> > +void show_ref_array_item(struct ref_array_item *info,\n> > + const struct ref_format *format,\n> > + struct strbuf *final_buf,\n> > + struct strbuf *error_buf);\n>\n> This bring-your-own-buffer approach pushes responsibilities back to\n> the callers, in exchange for improved performance. The number of\n> users of this interface is low, so that's defensible. But that added\n> effort is also non-trivial -- as you demonstrated by leaking the\n> allocated memory. ;-)\n>\n\nYes, this may be burden for the function caller.\n\n> How about offering to do more instead? In particular you could add\n> a count parameter and have show_ref_array_item() handle an array of\n> struct ref_array_item objects. It could reuse the buffers internally\n> to get the same performance benefit, and would free callers from\n> having to iterate loops themselves. Something like:\n>\n> void show_ref_array_items(struct ref_array_item **info,\n> size_t n,\n> const struct ref_format *format);\n>\n> Callers that deal with a single element can pass n = 1.\n>\n> Perhaps the \"format\" parameter should go first, like with printf.\n>\n> The double reference in \"**info\" is a bit ugly, though (array of\n> pointers instead of a simple array of objects). That's dictated\n> by struct ref_array_item containing a flexible array member, which\n> seems to be hard to change.\n>\n\nI personally think this idea is great.\nIn this way, there is no need to pass in two strbuf from the outside.\n\n+void show_ref_array_items(struct ref_array_item **info,\n+ const struct ref_format *format,\n+ size_t n)\n+{\n+ struct strbuf final_buf = STRBUF_INIT;\n+ struct strbuf error_buf = STRBUF_INIT;\n+ size_t i;\n+\n+ for (i = 0; i < n; i++) {\n+ if (format_ref_array_item(info[i], format, &final_buf,\n&error_buf))\n+ die(\"%s\", error_buf.buf);\n+ fwrite(final_buf.buf, 1, final_buf.len, stdout);\n+ strbuf_reset(&error_buf);\n+ strbuf_reset(&final_buf);\n+ putchar('\\n');\n+ }\n+ strbuf_release(&error_buf);\n+ strbuf_release(&final_buf);\n+}\n+\n\nAnd the result is here(close the network proxy program):\n\nHEAD~ result :\n\n(git)-[heads/master] % hyperfine \"./bin-wrappers/git for-each-ref\"\n--warmup=10\n Benchmark #1: ./bin-wrappers/git for-each-ref\n Time (mean ± σ): 18.7 ms ± 0.4 ms [User: 14.9 ms,\nSystem: 3.9 ms]\n Range (min … max): 18.1 ms … 19.8 ms 141 runs\n\nWith the new patch :\n (git)-[ref-filter-single-buf] % hyperfine \"./bin-wrappers/git\nfor-each-ref\" --warmup=10\n Benchmark #1: ./bin-wrappers/git for-each-ref\n Time (mean ± σ): 18.2 ms ± 0.3 ms [User: 14.1 ms, System:\n4.2 ms]\n Range (min … max): 17.4 ms … 19.2 ms 140 runs\n\nSeem like it does have some small advantages ;-)\n\n> > /* Parse a single sort specifier and add it to the list */\n> > void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n> > /* Callback function for parsing the sort option */\n> >\n> > base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48\n> >\n>\n\nA new iteration will be sent later.\n\nThanks!\n--\nZheNing Hu\n"},{"id":"421126","messageId":"pull.927.v2.git.1617809209164.gitgitgadget@gmail.com","threadId":"55435","inReplyTo":"pull.927.git.1617631280402.gitgitgadget@gmail.com","subject":"[PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-07T15:26:48Z","receivedAt":"2021-04-07T15:26:53Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen we use `git for-each-ref`, every ref will call\n`show_ref_array_item()` and allocate its own final strbuf\nand error strbuf. Instead, we can reuse these two strbuf\nfor each step ref's output.\n\nThe performance for `git for-each-ref` on the Git repository\nitself with performance testing tool `hyperfine` changes from\n18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.\n\nThis approach is similar to the one used by 79ed0a5\n(cat-file: use a single strbuf for all output, 2018-08-14)\nto speed up the cat-file builtin.\n\nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: use single strbuf for all output\n \n Now git for-each-ref can reuse two buffers for all refs output, the\n performance is slightly improved.\n \n Now there may be a question : Should the original interface\n show_ref_array_items be retained?\n \n Thanks.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/927\n\nRange-diff vs v1:\n\n 1: bcc3feb4de7c ! 1: 3aed12c4f5a8 [GSOC] ref-filter: use single strbuf for all output\n @@ Commit message\n \n When we use `git for-each-ref`, every ref will call\n `show_ref_array_item()` and allocate its own final strbuf\n - and error strbuf. Instead, we can provide two single strbuf:\n - final_buf and error_buf that get reused for each output.\n + and error strbuf. Instead, we can reuse these two strbuf\n + for each step ref's output.\n \n - When run it 100 times:\n + The performance for `git for-each-ref` on the Git repository\n + itself with performance testing tool `hyperfine` changes from\n + 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.\n \n - $ git for-each-ref\n -\n - on git.git :\n -\n - 3.19s user\n - 3.88s system\n - 35% cpu\n - 20.199 total\n -\n - to:\n -\n - 2.89s user\n - 4.00s system\n - 34% cpu\n - 19.741 total\n -\n - The performance has been slightly improved.\n + This approach is similar to the one used by 79ed0a5\n + (cat-file: use a single strbuf for all output, 2018-08-14)\n + to speed up the cat-file builtin.\n \n Signed-off-by: ZheNing Hu \n \n ## builtin/for-each-ref.c ##\n -@@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n - \tstruct ref_array array;\n - \tstruct ref_filter filter;\n - \tstruct ref_format format = REF_FORMAT_INIT;\n -+\tstruct strbuf final_buf = STRBUF_INIT;\n -+\tstruct strbuf error_buf = STRBUF_INIT;\n +@@ builtin/for-each-ref.c: static char const * const for_each_ref_usage[] = {\n \n - \tstruct option opts[] = {\n - \t\tOPT_BIT('s', \"shell\", &format.quote_style,\n + int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n + {\n +-\tint i;\n + \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n + \tint maxcount = 0, icase = 0;\n + \tstruct ref_array array;\n @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n + \n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n - \tfor (i = 0; i < maxcount; i++)\n +-\tfor (i = 0; i < maxcount; i++)\n -\t\tshow_ref_array_item(array.items[i], &format);\n -+\t\tshow_ref_array_item(array.items[i], &format, &final_buf, &error_buf);\n ++\tshow_ref_array_items(array.items, &format, maxcount);\n \tref_array_clear(&array);\n \treturn 0;\n }\n \n - ## builtin/tag.c ##\n -@@ builtin/tag.c: static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n - \t\t struct ref_format *format)\n - {\n - \tstruct ref_array array;\n -+\tstruct strbuf final_buf = STRBUF_INIT;\n -+\tstruct strbuf error_buf = STRBUF_INIT;\n - \tchar *to_free = NULL;\n - \tint i;\n - \n -@@ builtin/tag.c: static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n - \tref_array_sort(sorting, &array);\n - \n - \tfor (i = 0; i < array.nr; i++)\n --\t\tshow_ref_array_item(array.items[i], format);\n -+\t\tshow_ref_array_item(array.items[i], format, &final_buf, &error_buf);\n - \tref_array_clear(&array);\n - \tfree(to_free);\n - \n -\n ## ref-filter.c ##\n @@ ref-filter.c: int format_ref_array_item(struct ref_array_item *info,\n + \treturn 0;\n }\n \n - void show_ref_array_item(struct ref_array_item *info,\n --\t\t\t const struct ref_format *format)\n ++void show_ref_array_items(struct ref_array_item **info,\n +\t\t\t const struct ref_format *format,\n -+\t\t\t struct strbuf *final_buf,\n -+\t\t\t struct strbuf *error_buf)\n - {\n --\tstruct strbuf final_buf = STRBUF_INIT;\n --\tstruct strbuf error_buf = STRBUF_INIT;\n - \n --\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n --\t\tdie(\"%s\", error_buf.buf);\n --\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n --\tstrbuf_release(&error_buf);\n --\tstrbuf_release(&final_buf);\n -+\tif (format_ref_array_item(info, format, final_buf, error_buf))\n -+\t\tdie(\"%s\", error_buf->buf);\n -+\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n -+\tstrbuf_reset(error_buf);\n -+\tstrbuf_reset(final_buf);\n - \tputchar('\\n');\n - }\n - \n -@@ ref-filter.c: void pretty_print_ref(const char *name, const struct object_id *oid,\n - \t\t const struct ref_format *format)\n - {\n - \tstruct ref_array_item *ref_item;\n ++\t\t\t size_t n)\n ++{\n +\tstruct strbuf final_buf = STRBUF_INIT;\n +\tstruct strbuf error_buf = STRBUF_INIT;\n ++\tsize_t i;\n +\n - \tref_item = new_ref_array_item(name, oid);\n - \tref_item->kind = ref_kind_from_refname(name);\n --\tshow_ref_array_item(ref_item, format);\n -+\tshow_ref_array_item(ref_item, format, &final_buf, &error_buf);\n - \tfree_array_item(ref_item);\n - }\n - \n ++\tfor (i = 0; i < n; i++) {\n ++\t\tif (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n ++\t\t\tdie(\"%s\", error_buf.buf);\n ++\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n ++\t\tstrbuf_reset(&error_buf);\n ++\t\tstrbuf_reset(&final_buf);\n ++\t\tputchar('\\n');\n ++\t}\n ++\tstrbuf_release(&error_buf);\n ++\tstrbuf_release(&final_buf);\n ++}\n ++\n + void show_ref_array_item(struct ref_array_item *info,\n + \t\t\t const struct ref_format *format)\n + {\n \n ## ref-filter.h ##\n @@ ref-filter.h: int format_ref_array_item(struct ref_array_item *info,\n - \t\t\t struct strbuf *final_buf,\n \t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n --void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n -+void show_ref_array_item(struct ref_array_item *info,\n + void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n ++/* Print the refs using the given format and quote_style and maxcount */\n ++void show_ref_array_items(struct ref_array_item **info,\n +\t\t\t const struct ref_format *format,\n -+\t\t\t struct strbuf *final_buf,\n -+\t\t\t struct strbuf *error_buf);\n ++\t\t\t size_t n);\n ++\n /* Parse a single sort specifier and add it to the list */\n void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n /* Callback function for parsing the sort option */\n\n\n builtin/for-each-ref.c | 4 +---\n ref-filter.c | 20 ++++++++++++++++++++\n ref-filter.h | 5 +++++\n 3 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a04606..d630402230f3 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {\n \n int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n-\tint i;\n \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tint maxcount = 0, icase = 0;\n \tstruct ref_array array;\n@@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n-\tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], &format);\n+\tshow_ref_array_items(array.items, &format, maxcount);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f0bd32f71416..27bbf9b6c8ac 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,\n \treturn 0;\n }\n \n+void show_ref_array_items(struct ref_array_item **info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t size_t n)\n+{\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tif (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n+\t\t\tdie(\"%s\", error_buf.buf);\n+\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\t\tstrbuf_reset(&error_buf);\n+\t\tstrbuf_reset(&final_buf);\n+\t\tputchar('\\n');\n+\t}\n+\tstrbuf_release(&error_buf);\n+\tstrbuf_release(&final_buf);\n+}\n+\n void show_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format)\n {\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 19ea4c413409..eb7e79a6676d 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n+/* Print the refs using the given format and quote_style and maxcount */\n+void show_ref_array_items(struct ref_array_item **info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t size_t n);\n+\n /* Parse a single sort specifier and add it to the list */\n void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n /* Callback function for parsing the sort option */\n\nbase-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48\n-- \ngitgitgadget\n"},{"id":"421148","messageId":"xmqqczv5zvj3.fsf@gitster.g","threadId":"55435","inReplyTo":"pull.927.v2.git.1617809209164.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T20:31:28Z","receivedAt":"2021-04-07T20:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output\n\nThe implementation changed so much from the initial attempt, for\nwhich the above title may have been appropriate, that reusing single\nstrbuf over and over is not the most important part of the change\nanymore, I am afraid. Besides, it uses TWO strbufs ;-)\n\nSubject: [PATCH] ref-filter: introduce show_ref_array_items() helper\n\nor something like that?\n\n> From: ZheNing Hu \n>\n> When we use `git for-each-ref`, every ref will call\n> `show_ref_array_item()` and allocate its own final strbuf\n> and error strbuf. Instead, we can reuse these two strbuf\n> for each step ref's output.\n>\n> The performance for `git for-each-ref` on the Git repository\n> itself with performance testing tool `hyperfine` changes from\n> 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.\n>\n> This approach is similar to the one used by 79ed0a5\n> (cat-file: use a single strbuf for all output, 2018-08-14)\n> to speed up the cat-file builtin.\n>\n> Signed-off-by: ZheNing Hu \n> ---\n> [GSOC] ref-filter: use single strbuf for all output\n> \n> Now git for-each-ref can reuse two buffers for all refs output, the\n> performance is slightly improved.\n> \n> Now there may be a question : Should the original interface\n> show_ref_array_items be retained?\n> ...\n> /* Callback function for parsing the sort option */\n\nAgain, not a very useful range-diff as the implementation changed so much.\n\n\n> builtin/for-each-ref.c | 4 +---\n> ref-filter.c | 20 ++++++++++++++++++++\n> ref-filter.h | 5 +++++\n> 3 files changed, 26 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index cb9c81a04606..d630402230f3 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {\n> \n> int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> {\n> -\tint i;\n> \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n> \tint maxcount = 0, icase = 0;\n> \tstruct ref_array array;\n> @@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \n> \tif (!maxcount || array.nr < maxcount)\n> \t\tmaxcount = array.nr;\n> -\tfor (i = 0; i < maxcount; i++)\n> -\t\tshow_ref_array_item(array.items[i], &format);\n> +\tshow_ref_array_items(array.items, &format, maxcount);\n\nThe intention of this call is to pass an array and the number of\nelements in the array as a pair to the function, right? When you\ndesign the API for a new helper function, do not split them apart by\ninserting an unrelated parameter in the middle.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f0bd32f71416..27bbf9b6c8ac 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,\n> \treturn 0;\n> }\n> \n> +void show_ref_array_items(struct ref_array_item **info,\n> +\t\t\t const struct ref_format *format,\n> +\t\t\t size_t n)\n\nIOW,\n\n\tvoid show_ref_array_items(const struct ref_format *format,\n\t\t\t\t struct ref_array_item *info[], size_t n)\n\n> +{\n> +\tstruct strbuf final_buf = STRBUF_INIT;\n> +\tstruct strbuf error_buf = STRBUF_INIT;\n> +\tsize_t i;\n> +\n> +\tfor (i = 0; i < n; i++) {\n> +\t\tif (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n> +\t\t\tdie(\"%s\", error_buf.buf);\n\nOK, the contents of error_buf is already localized, so it is correct\nnot to have _() around the \"%s\" here.\n\n> +\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n> +\t\tstrbuf_reset(&error_buf);\n> +\t\tstrbuf_reset(&final_buf);\n> +\t\tputchar('\\n');\n\nThis is inherited code, but splitting fwrite() and putchar() apart\nlike this makes the code hard to follow. Perhaps clean it up later\nwhen nothing else is going on in the code as leftoverbits, outside\nthe topic.\n\n> +\t}\n> +\tstrbuf_release(&error_buf);\n> +\tstrbuf_release(&final_buf);\n> +}\n> +\n> void show_ref_array_item(struct ref_array_item *info,\n> \t\t\t const struct ref_format *format)\n> {\n\nIsn't the point of the new helper function so that this can become a\nthin wrapper around it, i.e.\n\n\tvoid show_ref_array_item(...)\n {\n\t\tshow_ref_array_items(format, &info, 1);\n\t}\n\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 19ea4c413409..eb7e79a6676d 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,\n> \t\t\t struct strbuf *error_buf);\n> /* Print the ref using the given format and quote_style */\n> void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> +/* Print the refs using the given format and quote_style and maxcount */\n> +void show_ref_array_items(struct ref_array_item **info,\n> +\t\t\t const struct ref_format *format,\n> +\t\t\t size_t n);\n\nThe inconsistency between \"maxcount\" vs \"n\" is irritating. Calling\nthe parameter with a name that has the word \"info\" (because the new\nparameter is about that array) and a word like \"nelem\" to hint that\nit is the number of elements in the array) would be sensible.\n\nvoid show_ref_array_items(const struct ref_format *format,\n\t\t\t struct ref_array_item *info[], size_t info_count);\n\nor something along the line, perhaps?\n\n"},{"id":"421156","messageId":"YG4jxKQ3z1R+8Jfz@coredump.intra.peff.net","threadId":"55435","inReplyTo":"pull.927.v2.git.1617809209164.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-07T21:27:32Z","receivedAt":"2021-04-07T21:27:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 07, 2021 at 03:26:48PM +0000, ZheNing Hu via GitGitGadget wrote:\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f0bd32f71416..27bbf9b6c8ac 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,\n> \treturn 0;\n> }\n> \n> +void show_ref_array_items(struct ref_array_item **info,\n> +\t\t\t const struct ref_format *format,\n> +\t\t\t size_t n)\n> +{\n> +\tstruct strbuf final_buf = STRBUF_INIT;\n> +\tstruct strbuf error_buf = STRBUF_INIT;\n> +\tsize_t i;\n> +\n> +\tfor (i = 0; i < n; i++) {\n> +\t\tif (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n> +\t\t\tdie(\"%s\", error_buf.buf);\n> +\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n> +\t\tstrbuf_reset(&error_buf);\n> +\t\tstrbuf_reset(&final_buf);\n> +\t\tputchar('\\n');\n> +\t}\n> +\tstrbuf_release(&error_buf);\n> +\tstrbuf_release(&final_buf);\n> +}\n\nI think this is a reasonable direction to take the solution: wrapping\nthe loop so that the reuse of the buffers can be included there.\n\nBut I do wonder if we should go the opposite direction, and get rid of\nshow_ref_array_item() entirely. It only has two callers, both of which\ncould just write the loop themselves. That is more code, but perhaps it\nwould make it more clear what is going on in those callers, and to give\nthem more flexibility.\n\nI notice there's a third user of the ref-filter.c code in\nbuiltin/branch.c that does not use show_ref_array_item(). Instead, it\nloops itself and calls format_ref_array_item(). I think this is because\nit is sometimes columnizing the results. Perhaps git-tag and\nfor-each-ref would want to learn the same trick, in which case they'd be\nhappy to have the open-coded loop.\n\nEither way, it probably makes sense to introduce the same optimization\nto the case in builtin/branch.c.\n\n-Peff\n"},{"id":"421204","messageId":"CAOLTT8RYjp1GKQpdPSzSNnn9o3+tuKNcGEUTp5HD9_Jut01JMQ@mail.gmail.com","threadId":"55435","inReplyTo":"xmqqczv5zvj3.fsf@gitster.g","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-08T12:05:39Z","receivedAt":"2021-04-08T12:06:02Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年4月8日周四 上午4:31写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output\n>\n> The implementation changed so much from the initial attempt, for\n> which the above title may have been appropriate, that reusing single\n> strbuf over and over is not the most important part of the change\n> anymore, I am afraid. Besides, it uses TWO strbufs ;-)\n>\n> Subject: [PATCH] ref-filter: introduce show_ref_array_items() helper\n>\n> or something like that?\n>\n\nYep, I may think that its core is still reusing strbufs, but\n\"introduce show_ref_array_items()\" will be more accurate.\n\n> > From: ZheNing Hu \n> >\n> > When we use `git for-each-ref`, every ref will call\n> > `show_ref_array_item()` and allocate its own final strbuf\n> > and error strbuf. Instead, we can reuse these two strbuf\n> > for each step ref's output.\n> >\n> > The performance for `git for-each-ref` on the Git repository\n> > itself with performance testing tool `hyperfine` changes from\n> > 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.\n> >\n> > This approach is similar to the one used by 79ed0a5\n> > (cat-file: use a single strbuf for all output, 2018-08-14)\n> > to speed up the cat-file builtin.\n> >\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > [GSOC] ref-filter: use single strbuf for all output\n> >\n> > Now git for-each-ref can reuse two buffers for all refs output, the\n> > performance is slightly improved.\n> >\n> > Now there may be a question : Should the original interface\n> > show_ref_array_items be retained?\n> > ...\n> > /* Callback function for parsing the sort option */\n>\n> Again, not a very useful range-diff as the implementation changed so much.\n>\n\nThis makes me wonder if I should give up GGG in the future.\nI also don’t want a rang-diff with a big difference.\n\n>\n> > builtin/for-each-ref.c | 4 +---\n> > ref-filter.c | 20 ++++++++++++++++++++\n> > ref-filter.h | 5 +++++\n> > 3 files changed, 26 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> > index cb9c81a04606..d630402230f3 100644\n> > --- a/builtin/for-each-ref.c\n> > +++ b/builtin/for-each-ref.c\n> > @@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {\n> >\n> > int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > {\n> > - int i;\n> > struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n> > int maxcount = 0, icase = 0;\n> > struct ref_array array;\n> > @@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> >\n> > if (!maxcount || array.nr < maxcount)\n> > maxcount = array.nr;\n> > - for (i = 0; i < maxcount; i++)\n> > - show_ref_array_item(array.items[i], &format);\n> > + show_ref_array_items(array.items, &format, maxcount);\n>\n> The intention of this call is to pass an array and the number of\n> elements in the array as a pair to the function, right? When you\n> design the API for a new helper function, do not split them apart by\n> inserting an unrelated parameter in the middle.\n>\n\nEh, are you saying that `maxcount` is irrelevant here? There should be\n`maxcount`, because we need to limit the number of iterations here.\n\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index f0bd32f71416..27bbf9b6c8ac 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,\n> > return 0;\n> > }\n> >\n> > +void show_ref_array_items(struct ref_array_item **info,\n> > + const struct ref_format *format,\n> > + size_t n)\n>\n> IOW,\n>\n> void show_ref_array_items(const struct ref_format *format,\n> struct ref_array_item *info[], size_t n)\n>\n\nYes, it will be more obvious in the form of an array.\n\n> > +{\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> > + size_t i;\n> > +\n> > + for (i = 0; i < n; i++) {\n> > + if (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n> > + die(\"%s\", error_buf.buf);\n>\n> OK, the contents of error_buf is already localized, so it is correct\n> not to have _() around the \"%s\" here.\n>\n> > + fwrite(final_buf.buf, 1, final_buf.len, stdout);\n> > + strbuf_reset(&error_buf);\n> > + strbuf_reset(&final_buf);\n> > + putchar('\\n');\n>\n> This is inherited code, but splitting fwrite() and putchar() apart\n> like this makes the code hard to follow. Perhaps clean it up later\n> when nothing else is going on in the code as leftoverbits, outside\n> the topic.\n>\n\nOk, swap the position of reset and putchar.\n\n> > + }\n> > + strbuf_release(&error_buf);\n> > + strbuf_release(&final_buf);\n> > +}\n> > +\n> > void show_ref_array_item(struct ref_array_item *info,\n> > const struct ref_format *format)\n> > {\n>\n> Isn't the point of the new helper function so that this can become a\n> thin wrapper around it, i.e.\n>\n> void show_ref_array_item(...)\n> {\n> show_ref_array_items(format, &info, 1);\n> }\n>\n\nMaybe it makes sense. But as Peff said, Maybe we can just delete it.\n\n> > diff --git a/ref-filter.h b/ref-filter.h\n> > index 19ea4c413409..eb7e79a6676d 100644\n> > --- a/ref-filter.h\n> > +++ b/ref-filter.h\n> > @@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,\n> > struct strbuf *error_buf);\n> > /* Print the ref using the given format and quote_style */\n> > void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n> > +/* Print the refs using the given format and quote_style and maxcount */\n> > +void show_ref_array_items(struct ref_array_item **info,\n> > + const struct ref_format *format,\n> > + size_t n);\n>\n> The inconsistency between \"maxcount\" vs \"n\" is irritating. Calling\n> the parameter with a name that has the word \"info\" (because the new\n> parameter is about that array) and a word like \"nelem\" to hint that\n> it is the number of elements in the array) would be sensible.\n>\n> void show_ref_array_items(const struct ref_format *format,\n> struct ref_array_item *info[], size_t info_count);\n>\n> or something along the line, perhaps?\n>\n\nAha, I guess this is the reason for the misunderstanding above.\nYes, `info_count` is the correct meaning and the meaning of `n` is\nwrong.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"421205","messageId":"CAOLTT8QvdLeWz=cDOoVFV8Lrk2QL2wf_jwDc6oK5j+6gup+Png@mail.gmail.com","threadId":"55435","inReplyTo":"YG4jxKQ3z1R+8Jfz@coredump.intra.peff.net","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-08T12:18:59Z","receivedAt":"2021-04-08T12:19:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年4月8日周四 上午5:27写道：\n>\n> On Wed, Apr 07, 2021 at 03:26:48PM +0000, ZheNing Hu via GitGitGadget wrote:\n>\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index f0bd32f71416..27bbf9b6c8ac 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,\n> > return 0;\n> > }\n> >\n> > +void show_ref_array_items(struct ref_array_item **info,\n> > + const struct ref_format *format,\n> > + size_t n)\n> > +{\n> > + struct strbuf final_buf = STRBUF_INIT;\n> > + struct strbuf error_buf = STRBUF_INIT;\n> > + size_t i;\n> > +\n> > + for (i = 0; i < n; i++) {\n> > + if (format_ref_array_item(info[i], format, &final_buf, &error_buf))\n> > + die(\"%s\", error_buf.buf);\n> > + fwrite(final_buf.buf, 1, final_buf.len, stdout);\n> > + strbuf_reset(&error_buf);\n> > + strbuf_reset(&final_buf);\n> > + putchar('\\n');\n> > + }\n> > + strbuf_release(&error_buf);\n> > + strbuf_release(&final_buf);\n> > +}\n>\n> I think this is a reasonable direction to take the solution: wrapping\n> the loop so that the reuse of the buffers can be included there.\n>\n> But I do wonder if we should go the opposite direction, and get rid of\n> show_ref_array_item() entirely. It only has two callers, both of which\n> could just write the loop themselves. That is more code, but perhaps it\n> would make it more clear what is going on in those callers, and to give\n> them more flexibility.\n>\n\nIndeed. I think `pretty_print_ref()` is proof that we may need to keep\n`show_ref_array_item()` because If it modified to `show_ref_array_items(...,1);`\nit will look very strange.\n\n> I notice there's a third user of the ref-filter.c code in\n> builtin/branch.c that does not use show_ref_array_item(). Instead, it\n> loops itself and calls format_ref_array_item(). I think this is because\n> it is sometimes columnizing the results. Perhaps git-tag and\n> for-each-ref would want to learn the same trick, in which case they'd be\n> happy to have the open-coded loop.\n>\n\nYes, a special judgment about colopts is used in the `print_ref_list()` of\n`branch.c`.\n\n> Either way, it probably makes sense to introduce the same optimization\n> to the case in builtin/branch.c.\n>\n\nI agree in `branch.c` here can learn this reusing bufs optimization.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"421221","messageId":"YG8UD7c4lChOQM5a@coredump.intra.peff.net","threadId":"55435","inReplyTo":"CAOLTT8QvdLeWz=cDOoVFV8Lrk2QL2wf_jwDc6oK5j+6gup+Png@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-08T14:32:47Z","receivedAt":"2021-04-08T14:32:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 08, 2021 at 08:18:59PM +0800, ZheNing Hu wrote:\n\n> > I think this is a reasonable direction to take the solution: wrapping\n> > the loop so that the reuse of the buffers can be included there.\n> >\n> > But I do wonder if we should go the opposite direction, and get rid of\n> > show_ref_array_item() entirely. It only has two callers, both of which\n> > could just write the loop themselves. That is more code, but perhaps it\n> > would make it more clear what is going on in those callers, and to give\n> > them more flexibility.\n> >\n> \n> Indeed. I think `pretty_print_ref()` is proof that we may need to keep\n> `show_ref_array_item()` because If it modified to `show_ref_array_items(...,1);`\n> it will look very strange.\n\nWhat I meant was that we should get rid of show_ref_array_items(), as\nwell, and just use format_ref_array_item() everywhere. This whole\nwrapper is only saving us a few lines, and it makes it harder to see\nwhat the function is doing. Likewise for pretty-print ref. But I dunno.\nMaybe that is all going too far.\n\n-Peff\n"},{"id":"421223","messageId":"CAOLTT8STy3Y_pLf0PDs-Uj1dXJYSjPD3FbKrUiSM4Uz-F7etSA@mail.gmail.com","threadId":"55435","inReplyTo":"YG8UD7c4lChOQM5a@coredump.intra.peff.net","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-08T14:43:54Z","receivedAt":"2021-04-08T14:44:12Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年4月8日周四 下午10:32写道：\n>\n> On Thu, Apr 08, 2021 at 08:18:59PM +0800, ZheNing Hu wrote:\n>\n> > > I think this is a reasonable direction to take the solution: wrapping\n> > > the loop so that the reuse of the buffers can be included there.\n> > >\n> > > But I do wonder if we should go the opposite direction, and get rid of\n> > > show_ref_array_item() entirely. It only has two callers, both of which\n> > > could just write the loop themselves. That is more code, but perhaps it\n> > > would make it more clear what is going on in those callers, and to give\n> > > them more flexibility.\n> > >\n> >\n> > Indeed. I think `pretty_print_ref()` is proof that we may need to keep\n> > `show_ref_array_item()` because If it modified to `show_ref_array_items(...,1);`\n> > it will look very strange.\n>\n> What I meant was that we should get rid of show_ref_array_items(), as\n> well, and just use format_ref_array_item() everywhere. This whole\n> wrapper is only saving us a few lines, and it makes it harder to see\n> what the function is doing. Likewise for pretty-print ref. But I dunno.\n> Maybe that is all going too far.\n>\n\nOk... so you mean we just use a loop like in branch.c, and get rid of\nshow_ref_array_items() and show_ref_array_item().\n(We can still use the optimization of reuse bufs)\n\n> -Peff\n\n--\nZheNing Hu\n"},{"id":"421224","messageId":"YG8Ya3Z0FGBm1vwf@coredump.intra.peff.net","threadId":"55435","inReplyTo":"CAOLTT8STy3Y_pLf0PDs-Uj1dXJYSjPD3FbKrUiSM4Uz-F7etSA@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-08T14:51:23Z","receivedAt":"2021-04-08T14:51:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 08, 2021 at 10:43:54PM +0800, ZheNing Hu wrote:\n\n> > What I meant was that we should get rid of show_ref_array_items(), as\n> > well, and just use format_ref_array_item() everywhere. This whole\n> > wrapper is only saving us a few lines, and it makes it harder to see\n> > what the function is doing. Likewise for pretty-print ref. But I dunno.\n> > Maybe that is all going too far.\n> >\n> \n> Ok... so you mean we just use a loop like in branch.c, and get rid of\n> show_ref_array_items() and show_ref_array_item().\n> (We can still use the optimization of reuse bufs)\n\nYes, something like this:\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a046..55297fe297 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n+\tstruct strbuf output = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT('s', \"shell\", &format.quote_style,\n@@ -80,8 +82,16 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n-\tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], &format);\n+\n+\tfor (i = 0; i < maxcount; i++) {\n+\t\tstrbuf_reset(&output);\n+\t\tif (format_ref_array_item(array.items[i], &format, &output, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tfwrite(output.buf, 1, output.len, stdout);\n+\t\tputchar('\\n');\n+\t}\n+\n+\tstrbuf_release(&output);\n \tref_array_clear(&array);\n \treturn 0;\n }\n\nIt is dropping a few lines by assuming that the error buf is only\ntouched when we return an error (which IMHO is a reasonable assumption\nto make).\n\n-Peff\n"},{"id":"421235","messageId":"CAOLTT8RBc0MLWZpRctxxMSnd5n6Fj6b6teZWz8kR_WebHxkvzQ@mail.gmail.com","threadId":"55435","inReplyTo":"YG8Ya3Z0FGBm1vwf@coredump.intra.peff.net","subject":"Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-08T15:12:21Z","receivedAt":"2021-04-08T15:12:38Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年4月8日周四 下午10:51写道：\n>\n> On Thu, Apr 08, 2021 at 10:43:54PM +0800, ZheNing Hu wrote:\n>\n> > > What I meant was that we should get rid of show_ref_array_items(), as\n> > > well, and just use format_ref_array_item() everywhere. This whole\n> > > wrapper is only saving us a few lines, and it makes it harder to see\n> > > what the function is doing. Likewise for pretty-print ref. But I dunno.\n> > > Maybe that is all going too far.\n> > >\n> >\n> > Ok... so you mean we just use a loop like in branch.c, and get rid of\n> > show_ref_array_items() and show_ref_array_item().\n> > (We can still use the optimization of reuse bufs)\n>\n> Yes, something like this:\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index cb9c81a046..55297fe297 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> struct ref_array array;\n> struct ref_filter filter;\n> struct ref_format format = REF_FORMAT_INIT;\n> + struct strbuf output = STRBUF_INIT;\n> + struct strbuf err = STRBUF_INIT;\n>\n> struct option opts[] = {\n> OPT_BIT('s', \"shell\", &format.quote_style,\n> @@ -80,8 +82,16 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n>\n> if (!maxcount || array.nr < maxcount)\n> maxcount = array.nr;\n> - for (i = 0; i < maxcount; i++)\n> - show_ref_array_item(array.items[i], &format);\n> +\n> + for (i = 0; i < maxcount; i++) {\n> + strbuf_reset(&output);\n> + if (format_ref_array_item(array.items[i], &format, &output, &err))\n> + die(\"%s\", err.buf);\n> + fwrite(output.buf, 1, output.len, stdout);\n> + putchar('\\n');\n> + }\n> +\n> + strbuf_release(&output);\n> ref_array_clear(&array);\n> return 0;\n> }\n>\n> It is dropping a few lines by assuming that the error buf is only\n> touched when we return an error (which IMHO is a reasonable assumption\n> to make).\n>\n\nI agree, err buffer does not need to be reused.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"}]}