{"thread":{"id":"55462","subject":"[PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","startedAt":"2021-04-09T13:35:56Z","lastAt":"2021-04-21T05:52:01Z","messageCount":5,"participants":["ZheNing Hu via GitGitGadget","ZheNing Hu","René Scharfe."],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"421387","messageId":"pull.928.git.1617975348494.gitgitgadget@gmail.com","threadId":"55462","inReplyTo":null,"subject":"[PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-09T13:35:47Z","receivedAt":"2021-04-09T13:35:56Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu <adlternative@gmail.com>\n\nWhen we use `git for-each-ref`, every ref will call\n`show_ref_array_item()` and allocate its own final strbuf.\nBut we can reuse the final strbuf for each step ref's output.\nSince `show_ref_array_item()` is not used in many places,\nwe can directly delete `show_ref_array_item()` and use the\nsame logic code to replace it. In this way, the caller can\nclearly see how the loop work.\n\nThe performance for `git for-each-ref` on the Git repository\nitself with performance testing tool `hyperfine` changes from\n23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms.\n\nAt the same time, we apply this optimization to `git tag -l`\nand `git branch -l`, the `git branch -l` performance upgrade\nfrom 5.8 ms ± 0.8 ms to 2.7 ms ± 0.2 ms and `git tag -l`\nperformance upgrade from 5.9 ms ± 0.4 ms to 5.4 ms ± 0.4 ms.\nSince the number of tags in git.git is much more than branches,\nso this shows that the optimization will be more obvious in\nthose repositories that contain a small number of objects.\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 <adlternative@gmail.com>\n---\n    [GSOC] ref-filter: get rid of show_ref_array_item\n    \n    Now git for-each-ref can reuse final buf for all refs output, the\n    performance is slightly improved, This optimization is also applied to\n    git tag -l and git branch -l.\n    \n    Thanks.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-928%2Fadlternative%2Fref-filter-reuse-buf-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-928/adlternative/ref-filter-reuse-buf-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/928\n\n builtin/branch.c       |  8 ++++----\n builtin/for-each-ref.c | 13 +++++++++++--\n builtin/tag.c          | 13 +++++++++++--\n ref-filter.c           | 24 +++++++++---------------\n ref-filter.h           |  2 --\n 5 files changed, 35 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex bcc00bcf182d..5c797e992aa4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n {\n \tint i;\n \tstruct ref_array array;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n \tchar *to_free = NULL;\n@@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++) {\n-\t\tstruct strbuf out = STRBUF_INIT;\n-\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstrbuf_reset(&out);\n \t\tif (format_ref_array_item(array.items[i], format, &out, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tif (column_active(colopts)) {\n@@ -452,10 +453,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n \t\t}\n-\t\tstrbuf_release(&err);\n-\t\tstrbuf_release(&out);\n \t}\n \n+\tstrbuf_release(&out);\n \tref_array_clear(&array);\n \tfree(to_free);\n }\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex cb9c81a04606..157cc8623949 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,15 @@ 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+\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 }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d403417b5625..b172f3861413 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 output = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tchar *to_free = NULL;\n \tint i;\n \n@@ -63,8 +65,15 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n \tfilter_refs(&array, filter, FILTER_REFS_TAGS);\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+\tfor (i = 0; i < array.nr; 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 \tfree(to_free);\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex f0bd32f71416..9e38e42fb7ae 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2435,27 +2435,21 @@ 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-{\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-\tputchar('\\n');\n-}\n-\n 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 output = STRBUF_INIT;\n+\tstruct strbuf err = 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+\tif (format_ref_array_item(ref_item, format, &output, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tfwrite(output.buf, 1, output.len, stdout);\n+\tputchar('\\n');\n+\n+\tstrbuf_release(&output);\n \tfree_array_item(ref_item);\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 19ea4c413409..baf72a718965 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -119,8 +119,6 @@ int format_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-/*  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 /*  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":"422087","messageId":"CAOLTT8Tis5Yjg8UR0c-i0BnqiFQvLXvDgxUQJ-WcP6jjQPu9cQ@mail.gmail.com","threadId":"55462","inReplyTo":"pull.928.git.1617975348494.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-16T11:28:28Z","receivedAt":"2021-04-16T11:28:53Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> 于2021年4月9日周五 下午9:35写道：\n>\n> From: ZheNing Hu <adlternative@gmail.com>\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> But we can reuse the final strbuf for each step ref's output.\n> Since `show_ref_array_item()` is not used in many places,\n> we can directly delete `show_ref_array_item()` and use the\n> same logic code to replace it. In this way, the caller can\n> clearly see how the loop work.\n>\n> The performance for `git for-each-ref` on the Git repository\n> itself with performance testing tool `hyperfine` changes from\n> 23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms.\n>\n> At the same time, we apply this optimization to `git tag -l`\n> and `git branch -l`, the `git branch -l` performance upgrade\n> from 5.8 ms ± 0.8 ms to 2.7 ms ± 0.2 ms and `git tag -l`\n> performance upgrade from 5.9 ms ± 0.4 ms to 5.4 ms ± 0.4 ms.\n> Since the number of tags in git.git is much more than branches,\n> so this shows that the optimization will be more obvious in\n> those repositories that contain a small number of objects.\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 <adlternative@gmail.com>\n> ---\n>     [GSOC] ref-filter: get rid of show_ref_array_item\n>\n>     Now git for-each-ref can reuse final buf for all refs output, the\n>     performance is slightly improved, This optimization is also applied to\n>     git tag -l and git branch -l.\n>\n>     Thanks.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-928%2Fadlternative%2Fref-filter-reuse-buf-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-928/adlternative/ref-filter-reuse-buf-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/928\n>\n>  builtin/branch.c       |  8 ++++----\n>  builtin/for-each-ref.c | 13 +++++++++++--\n>  builtin/tag.c          | 13 +++++++++++--\n>  ref-filter.c           | 24 +++++++++---------------\n>  ref-filter.h           |  2 --\n>  5 files changed, 35 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index bcc00bcf182d..5c797e992aa4 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>  {\n>         int i;\n>         struct ref_array array;\n> +       struct strbuf out = STRBUF_INIT;\n> +       struct strbuf err = STRBUF_INIT;\n>         int maxwidth = 0;\n>         const char *remote_prefix = \"\";\n>         char *to_free = NULL;\n> @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>         ref_array_sort(sorting, &array);\n>\n>         for (i = 0; i < array.nr; i++) {\n> -               struct strbuf out = STRBUF_INIT;\n> -               struct strbuf err = STRBUF_INIT;\n> +               strbuf_reset(&out);\n>                 if (format_ref_array_item(array.items[i], format, &out, &err))\n>                         die(\"%s\", err.buf);\n>                 if (column_active(colopts)) {\n> @@ -452,10 +453,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>                         fwrite(out.buf, 1, out.len, stdout);\n>                         putchar('\\n');\n>                 }\n> -               strbuf_release(&err);\n> -               strbuf_release(&out);\n>         }\n>\n> +       strbuf_release(&out);\n>         ref_array_clear(&array);\n>         free(to_free);\n>  }\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index cb9c81a04606..157cc8623949 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,15 @@ 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> +       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> diff --git a/builtin/tag.c b/builtin/tag.c\n> index d403417b5625..b172f3861413 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 output = STRBUF_INIT;\n> +       struct strbuf err = STRBUF_INIT;\n>         char *to_free = NULL;\n>         int i;\n>\n> @@ -63,8 +65,15 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n>         filter_refs(&array, filter, FILTER_REFS_TAGS);\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> +       for (i = 0; i < array.nr; 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>         free(to_free);\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f0bd32f71416..9e38e42fb7ae 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2435,27 +2435,21 @@ int format_ref_array_item(struct ref_array_item *info,\n>         return 0;\n>  }\n>\n> -void show_ref_array_item(struct ref_array_item *info,\n> -                        const struct ref_format *format)\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> -       putchar('\\n');\n> -}\n> -\n>  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 output = STRBUF_INIT;\n> +       struct strbuf err = 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> +       if (format_ref_array_item(ref_item, format, &output, &err))\n> +               die(\"%s\", err.buf);\n> +       fwrite(output.buf, 1, output.len, stdout);\n> +       putchar('\\n');\n> +\n> +       strbuf_release(&output);\n>         free_array_item(ref_item);\n>  }\n>\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 19ea4c413409..baf72a718965 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -119,8 +119,6 @@ int format_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> -/*  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>  /*  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> gitgitgadget\n\nThe patch seems to have fallen into the crack.\nJeff and Junio, willing to help?\n\nThanks!\n--\nZheNing Hu\n"},{"id":"422160","messageId":"4c4eded7-3bb3-7ae9-6455-468b9522978c@web.de","threadId":"55462","inReplyTo":"pull.928.git.1617975348494.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","fromName":"René Scharfe.","fromEmail":"l.s.r@web.de","sentAt":"2021-04-17T09:11:14Z","receivedAt":"2021-04-17T09:11:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.04.21 um 15:35 schrieb ZheNing Hu via GitGitGadget:\n> From: ZheNing Hu <adlternative@gmail.com>\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> But we can reuse the final strbuf for each step ref's output.\n> Since `show_ref_array_item()` is not used in many places,\n> we can directly delete `show_ref_array_item()` and use the\n> same logic code to replace it. In this way, the caller can\n> clearly see how the loop work.\n\nInlining an exported function that is not providing the right level of\nabstraction is a bold move that simplifies the API and can unlock\nimprovements at the former call sites, like the possibility to reuse an\nallocated buffer in this case.  OK.\n\n> The performance for `git for-each-ref` on the Git repository\n> itself with performance testing tool `hyperfine` changes from\n> 23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms.\n\nI see a speedup as well, but it's within the noise.\n\n> At the same time, we apply this optimization to `git tag -l`\n> and `git branch -l`, the `git branch -l` performance upgrade\n> from 5.8 ms ± 0.8 ms to 2.7 ms ± 0.2 ms and `git tag -l`\n> performance upgrade from 5.9 ms ± 0.4 ms to 5.4 ms ± 0.4 ms.\n\nOn my system there's no measurable change with these commands.\n\nNevertheless I think reusing the buffer across the loops is a good\nidea.\n\n> Since the number of tags in git.git is much more than branches,\n> so this shows that the optimization will be more obvious in\n> those repositories that contain a small number of objects.\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 <adlternative@gmail.com>\n> ---\n>     [GSOC] ref-filter: get rid of show_ref_array_item\n>\n>     Now git for-each-ref can reuse final buf for all refs output, the\n>     performance is slightly improved, This optimization is also applied to\n>     git tag -l and git branch -l.\n>\n>     Thanks.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-928%2Fadlternative%2Fref-filter-reuse-buf-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-928/adlternative/ref-filter-reuse-buf-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/928\n>\n>  builtin/branch.c       |  8 ++++----\n>  builtin/for-each-ref.c | 13 +++++++++++--\n>  builtin/tag.c          | 13 +++++++++++--\n>  ref-filter.c           | 24 +++++++++---------------\n>  ref-filter.h           |  2 --\n>  5 files changed, 35 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index bcc00bcf182d..5c797e992aa4 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>  {\n>  \tint i;\n>  \tstruct ref_array array;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n>  \tint maxwidth = 0;\n>  \tconst char *remote_prefix = \"\";\n>  \tchar *to_free = NULL;\n> @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>  \tref_array_sort(sorting, &array);\n>\n>  \tfor (i = 0; i < array.nr; i++) {\n> -\t\tstruct strbuf out = STRBUF_INIT;\n> -\t\tstruct strbuf err = STRBUF_INIT;\n> +\t\tstrbuf_reset(&out);\n>  \t\tif (format_ref_array_item(array.items[i], format, &out, &err))\n\nThis function didn't call show_ref_array_item() to begin with, so\nstrictly speaking it's not related to change in the title.  It is a\npreexisting example of show_ref_array_item() not being flexible enough,\nthough.  I think it makes sense to have separate patches for inlining\nthe function verbatim and reusing the output buffer when\nformat_ref_array_item() is called in a loop.\n\n>  \t\t\tdie(\"%s\", err.buf);\n>  \t\tif (column_active(colopts)) {\n> @@ -452,10 +453,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n>  \t\t\tfwrite(out.buf, 1, out.len, stdout);\n>  \t\t\tputchar('\\n');\n>  \t\t}\n> -\t\tstrbuf_release(&err);\n> -\t\tstrbuf_release(&out);\n>  \t}\n>\n> +\tstrbuf_release(&out);\n\nerr is no longer released, and it is also not reset in the loop.\nThat change is not mentioned in the commit message, but it should.\nWhy is it safe?  Probably because format_ref_array_item() only\npopulates it if it also returns non-zero and then we end up dying\nanyway.\n\nThat makes leak checking harder, though -- it's not easy to see if\nerr hasn't simply been forgotten to be released.  I'd just retain\nthe strbuf_release() call at the end of the function -- it\nshouldn't have a measurable performance impact and documents that\nthis function is cleaning up after itself.  Thoughts?\n\n>  \tref_array_clear(&array);\n>  \tfree(to_free);\n>  }\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index cb9c81a04606..157cc8623949 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,15 @@ 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> +\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> diff --git a/builtin/tag.c b/builtin/tag.c\n> index d403417b5625..b172f3861413 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 output = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n>  \tchar *to_free = NULL;\n>  \tint i;\n>\n> @@ -63,8 +65,15 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n>  \tfilter_refs(&array, filter, FILTER_REFS_TAGS);\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> +\tfor (i = 0; i < array.nr; 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>  \tfree(to_free);\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f0bd32f71416..9e38e42fb7ae 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2435,27 +2435,21 @@ 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> -{\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> -\tputchar('\\n');\n> -}\n> -\n>  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 output = STRBUF_INIT;\n> +\tstruct strbuf err = 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> +\tif (format_ref_array_item(ref_item, format, &output, &err))\n> +\t\tdie(\"%s\", err.buf);\n> +\tfwrite(output.buf, 1, output.len, stdout);\n> +\tputchar('\\n');\n> +\n> +\tstrbuf_release(&output);\n>  \tfree_array_item(ref_item);\n>  }\n>\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 19ea4c413409..baf72a718965 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -119,8 +119,6 @@ int format_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> -/*  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>  /*  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"},{"id":"422209","messageId":"CAOLTT8RY_8Ui7AAfU6K7O03DqbMTh0m5GJpmuwOyiV8Ghn+cQg@mail.gmail.com","threadId":"55462","inReplyTo":"4c4eded7-3bb3-7ae9-6455-468b9522978c@web.de","subject":"Re: [PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-18T11:22:05Z","receivedAt":"2021-04-18T11:22:24Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"René Scharfe. <l.s.r@web.de> 于2021年4月17日周六 下午5:11写道：\n>\n> Am 09.04.21 um 15:35 schrieb ZheNing Hu via GitGitGadget:\n> > From: ZheNing Hu <adlternative@gmail.com>\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> > But we can reuse the final strbuf for each step ref's output.\n> > Since `show_ref_array_item()` is not used in many places,\n> > we can directly delete `show_ref_array_item()` and use the\n> > same logic code to replace it. In this way, the caller can\n> > clearly see how the loop work.\n>\n> Inlining an exported function that is not providing the right level of\n> abstraction is a bold move that simplifies the API and can unlock\n> improvements at the former call sites, like the possibility to reuse an\n> allocated buffer in this case.  OK.\n>\n> > The performance for `git for-each-ref` on the Git repository\n> > itself with performance testing tool `hyperfine` changes from\n> > 23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms.\n>\n> I see a speedup as well, but it's within the noise.\n>\n\nYes, the performance improvement is very small under a large number\nof refs. It was almost completely drowned out by the noise.\n\n> > At the same time, we apply this optimization to `git tag -l`\n> > and `git branch -l`, the `git branch -l` performance upgrade\n> > from 5.8 ms ± 0.8 ms to 2.7 ms ± 0.2 ms and `git tag -l`\n> > performance upgrade from 5.9 ms ± 0.4 ms to 5.4 ms ± 0.4 ms.\n>\n> On my system there's no measurable change with these commands.\n>\n\nIn our case, git branch -l has made obvious progress, but it may be because\nthe number of branches is far less than tags.\n\n> Nevertheless I think reusing the buffer across the loops is a good\n> idea.\n>\n> > Since the number of tags in git.git is much more than branches,\n> > so this shows that the optimization will be more obvious in\n> > those repositories that contain a small number of objects.\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 <adlternative@gmail.com>\n> > ---\n> >     [GSOC] ref-filter: get rid of show_ref_array_item\n> >\n> >     Now git for-each-ref can reuse final buf for all refs output, the\n> >     performance is slightly improved, This optimization is also applied to\n> >     git tag -l and git branch -l.\n> >\n> >     Thanks.\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-928%2Fadlternative%2Fref-filter-reuse-buf-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-928/adlternative/ref-filter-reuse-buf-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/928\n> >\n> >  builtin/branch.c       |  8 ++++----\n> >  builtin/for-each-ref.c | 13 +++++++++++--\n> >  builtin/tag.c          | 13 +++++++++++--\n> >  ref-filter.c           | 24 +++++++++---------------\n> >  ref-filter.h           |  2 --\n> >  5 files changed, 35 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > index bcc00bcf182d..5c797e992aa4 100644\n> > --- a/builtin/branch.c\n> > +++ b/builtin/branch.c\n> > @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n> >  {\n> >       int i;\n> >       struct ref_array array;\n> > +     struct strbuf out = STRBUF_INIT;\n> > +     struct strbuf err = STRBUF_INIT;\n> >       int maxwidth = 0;\n> >       const char *remote_prefix = \"\";\n> >       char *to_free = NULL;\n> > @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n> >       ref_array_sort(sorting, &array);\n> >\n> >       for (i = 0; i < array.nr; i++) {\n> > -             struct strbuf out = STRBUF_INIT;\n> > -             struct strbuf err = STRBUF_INIT;\n> > +             strbuf_reset(&out);\n> >               if (format_ref_array_item(array.items[i], format, &out, &err))\n>\n> This function didn't call show_ref_array_item() to begin with, so\n> strictly speaking it's not related to change in the title.  It is a\n> preexisting example of show_ref_array_item() not being flexible enough,\n> though.  I think it makes sense to have separate patches for inlining\n> the function verbatim and reusing the output buffer when\n> format_ref_array_item() is called in a loop.\n>\n\nI agree with you. I will divide this into a separate patch.\n\n> >                       die(\"%s\", err.buf);\n> >               if (column_active(colopts)) {\n> > @@ -452,10 +453,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n> >                       fwrite(out.buf, 1, out.len, stdout);\n> >                       putchar('\\n');\n> >               }\n> > -             strbuf_release(&err);\n> > -             strbuf_release(&out);\n> >       }\n> >\n> > +     strbuf_release(&out);\n>\n> err is no longer released, and it is also not reset in the loop.\n> That change is not mentioned in the commit message, but it should.\n> Why is it safe?  Probably because format_ref_array_item() only\n> populates it if it also returns non-zero and then we end up dying\n> anyway.\n>\n> That makes leak checking harder, though -- it's not easy to see if\n> err hasn't simply been forgotten to be released.  I'd just retain\n> the strbuf_release() call at the end of the function -- it\n> shouldn't have a measurable performance impact and documents that\n> this function is cleaning up after itself.  Thoughts?\n>\n\nMakes sense. Perhaps future changes will forget the release of err buffer.\nI will add `strbuf_release()` here.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"422527","messageId":"CAOLTT8TQfFjgcwQrf88SgMOYubwy1TsX7qgN2TbLZp=daTZ2Bg@mail.gmail.com","threadId":"55462","inReplyTo":"xmqqeef47s5k.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: get rid of show_ref_array_item","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-04-21T05:51:44Z","receivedAt":"2021-04-21T05:52:01Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 于2021年4月21日周三 上午2:00写道：\n>\n> René Scharfe. <l.s.r@web.de> writes:\n>\n> >> @@ -452,10 +453,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n> >>                      fwrite(out.buf, 1, out.len, stdout);\n> >>                      putchar('\\n');\n> >>              }\n> >> -            strbuf_release(&err);\n> >> -            strbuf_release(&out);\n> >>      }\n> >>\n> >> +    strbuf_release(&out);\n> >\n> > err is no longer released, and it is also not reset in the loop.\n> > That change is not mentioned in the commit message, but it should.\n> > Why is it safe?  Probably because format_ref_array_item() only\n> > populates it if it also returns non-zero and then we end up dying\n> > anyway.\n> >\n> > That makes leak checking harder, though -- it's not easy to see if\n> > err hasn't simply been forgotten to be released.  I'd just retain\n> > the strbuf_release() call at the end of the function -- it\n> > shouldn't have a measurable performance impact and documents that\n> > this function is cleaning up after itself.  Thoughts?\n>\n> I should have responded to this comment before it was too late,\n> sorry.\n>\n> I am OK with documenting the assumption that we will die when err\n> gets populated without coming out of the loop and not releasing at\n> the end (because we would not have anything to release when we got\n> there).  I am also OK with resetting(err) in the loop (which will\n> be a no-op as err.len would always be 0 at that point, or we would\n> have died) and releasing(err) at the end.  I found it a bit funny\n> to be not resetting in the loop and releasing at the end, without\n> a comment that explains the thought behind it.\n>\n\nSo a better solution is \"without err buffer _reset() and _release()\" and explain\nthe reason for \"not needing to be cleaned up\" in the commit message?\n\nThanks.\n--\nZheNing Hu\n"}]}