[PATCH 0/2] [GSOC] ref-filter: reuse output buffer

[PATCH 0/2] [GSOC] ref-filter: reuse output buffer

[ZheNing Hu via GitGitGadget]

 * Firstly, inlining show_ref_array_item().
 * Secondly, git for-each-ref reuse final buf for all refs output, the
   performance is slightly improved, this optimization is also applied to
   git tag -l and git branch -l.

Thanks.

ZheNing Hu (2):
  [GSOC] ref-filter: get rid of show_ref_array_item
  [GSOC] ref-filter: reuse output buffer

 builtin/branch.c       |  9 +++++----
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 5 files changed, 39 insertions(+), 25 deletions(-)


base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-935%2Fadlternative%2Fref-filter-opt-reuse-buf-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-935/adlternative/ref-filter-opt-reuse-buf-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/935
-- 
gitgitgadget

[PATCH 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Inlining the exported function `show_ref_array_item()`
which is not providing the right level of abstraction,
simplifies the API and can unlock improvements at the
former call sites.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index cb9c81a04606..8520008604e3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -80,8 +80,18 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
-	for (i = 0; i < maxcount; i++)
-		show_ref_array_item(array.items[i], &format);
+	for (i = 0; i < maxcount; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], &format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d403417b5625..d92d8e110b4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -63,8 +63,18 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format);
+	for (i = 0; i < array.nr; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	free(to_free);
 
diff --git a/ref-filter.c b/ref-filter.c
index f0bd32f71416..df67047fd615 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2435,27 +2435,22 @@ int format_ref_array_item(struct ref_array_item *info,
 	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
-			 const struct ref_format *format)
-{
-	struct strbuf final_buf = STRBUF_INIT;
-	struct strbuf error_buf = STRBUF_INIT;
-
-	if (format_ref_array_item(info, format, &final_buf, &error_buf))
-		die("%s", error_buf.buf);
-	fwrite(final_buf.buf, 1, final_buf.len, stdout);
-	strbuf_release(&error_buf);
-	strbuf_release(&final_buf);
-	putchar('\n');
-}
-
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
+
 	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
-	show_ref_array_item(ref_item, format);
+	if (format_ref_array_item(ref_item, format, &output, &err))
+		die("%s", err.buf);
+	fwrite(output.buf, 1, output.len, stdout);
+	putchar('\n');
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	free_array_item(ref_item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 19ea4c413409..baf72a718965 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,8 +119,6 @@ int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
 			  struct strbuf *final_buf,
 			  struct strbuf *error_buf);
-/*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */
-- 
gitgitgadget

[PATCH 2/2] [GSOC] ref-filter: reuse output buffer

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

When we use `git for-each-ref`, every ref will allocate
its own output strbuf. But we can reuse the final strbuf
for each step ref's output.

The performance for `git for-each-ref` on the Git repository
itself with performance testing tool `hyperfine` changes from
23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms. Optimization is relatively
minor.

At the same time, we apply this optimization to `git tag -l`
and `git branch -l`.

This approach is similar to the one used by 79ed0a5
(cat-file: use a single strbuf for all output, 2018-08-14)
to speed up the cat-file builtin.

Helped-by: Jeff King <peff@peff.net>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       |  9 +++++----
 builtin/for-each-ref.c | 12 ++++++------
 builtin/tag.c          | 12 ++++++------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index bcc00bcf182d..00081de1aed8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 {
 	int i;
 	struct ref_array array;
+	struct strbuf out = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
 	char *to_free = NULL;
@@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf out = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
+		strbuf_reset(&out);
 		if (format_ref_array_item(array.items[i], format, &out, &err))
 			die("%s", err.buf);
 		if (column_active(colopts)) {
@@ -452,10 +453,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 			fwrite(out.buf, 1, out.len, stdout);
 			putchar('\n');
 		}
-		strbuf_release(&err);
-		strbuf_release(&out);
 	}
 
+	strbuf_release(&err);
+	strbuf_release(&out);
 	ref_array_clear(&array);
 	free(to_free);
 }
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8520008604e3..bf24c595c526 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_array array;
 	struct ref_filter filter;
 	struct ref_format format = REF_FORMAT_INIT;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -81,17 +83,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
 	for (i = 0; i < maxcount; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], &format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d92d8e110b4d..592af1d154ea 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 		     struct ref_format *format)
 {
 	struct ref_array array;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	char *to_free = NULL;
 	int i;
 
@@ -64,17 +66,15 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	free(to_free);
 
-- 
gitgitgadget

Re: [PATCH 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

[Junio C Hamano]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> Inlining the exported function `show_ref_array_item()`
> which is not providing the right level of abstraction,
> simplifies the API and can unlock improvements at the
> former call sites.

I'll insert a comma right at the end of the first line while
queuing.

Thanks.

Re: [PATCH 2/2] [GSOC] ref-filter: reuse output buffer

[Junio C Hamano]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index bcc00bcf182d..00081de1aed8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  {
>  	int i;
>  	struct ref_array array;
> +	struct strbuf out = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
>  	int maxwidth = 0;
>  	const char *remote_prefix = "";
>  	char *to_free = NULL;
> @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  	ref_array_sort(sorting, &array);
>  
>  	for (i = 0; i < array.nr; i++) {
> -		struct strbuf out = STRBUF_INIT;
> -		struct strbuf err = STRBUF_INIT;
> +		strbuf_reset(&out);
>  		if (format_ref_array_item(array.items[i], format, &out, &err))
>  			die("%s", err.buf);

This change relies on the fact that format_ref_array_item() will
never touch error when it returns 0 (success); otherwise, we'd end
up accumulating err from multiple calls to it in the loop until it
returns non-zero (failure), at which point we emit a single "fatal:"
prefix to show multiple error messages.  Which leans me ...

> @@ -452,10 +453,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  			fwrite(out.buf, 1, out.len, stdout);
>  			putchar('\n');
>  		}
> -		strbuf_release(&err);
> -		strbuf_release(&out);
>  	}
>  
> +	strbuf_release(&err);
> +	strbuf_release(&out);

... to suspect that the _release() of err will always be a no-op.

It may be easier to follow if err is _reset() always where out is
_reset(), from code cleanliness's perspective.  Then nobody has to
wonder why we do not reset err inside loop even though we release
at the end.

It also is OK to document more clearly that we assume that the loop
will not exit without calling die() when err is not empty.  If we
take that route, we may want to drop _release(&err) at the end.

I do not know which of the two is better, but the code presented
which is halfway between these two does not quite look easy to
reason about.

Thanks.

Re: [PATCH 2/2] [GSOC] ref-filter: reuse output buffer

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年4月20日周二 上午5:04写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index bcc00bcf182d..00081de1aed8 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
> >  {
> >       int i;
> >       struct ref_array array;
> > +     struct strbuf out = STRBUF_INIT;
> > +     struct strbuf err = STRBUF_INIT;
> >       int maxwidth = 0;
> >       const char *remote_prefix = "";
> >       char *to_free = NULL;
> > @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
> >       ref_array_sort(sorting, &array);
> >
> >       for (i = 0; i < array.nr; i++) {
> > -             struct strbuf out = STRBUF_INIT;
> > -             struct strbuf err = STRBUF_INIT;
> > +             strbuf_reset(&out);
> >               if (format_ref_array_item(array.items[i], format, &out, &err))
> >                       die("%s", err.buf);
>
> This change relies on the fact that format_ref_array_item() will
> never touch error when it returns 0 (success); otherwise, we'd end
> up accumulating err from multiple calls to it in the loop until it
> returns non-zero (failure), at which point we emit a single "fatal:"
> prefix to show multiple error messages.  Which leans me ...
>
> > @@ -452,10 +453,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
> >                       fwrite(out.buf, 1, out.len, stdout);
> >                       putchar('\n');
> >               }
> > -             strbuf_release(&err);
> > -             strbuf_release(&out);
> >       }
> >
> > +     strbuf_release(&err);
> > +     strbuf_release(&out);
>
> ... to suspect that the _release() of err will always be a no-op.
>

Yes, it's a no-op to _release(&err) In the present situation.

> It may be easier to follow if err is _reset() always where out is
> _reset(), from code cleanliness's perspective.  Then nobody has to
> wonder why we do not reset err inside loop even though we release
> at the end.
>
> It also is OK to document more clearly that we assume that the loop
> will not exit without calling die() when err is not empty.  If we
> take that route, we may want to drop _release(&err) at the end.
>
> I do not know which of the two is better, but the code presented
> which is halfway between these two does not quite look easy to
> reason about.
>

René Scharfe mention that it make leaks checking harder if we without
releasing this err. So on balance, adding err's _reset() in the loop seems
like a viable option. The change in performance will also be minimal too.

Even though we're using _release() in the loop in v1, and then Peff think that
we don't need to _release() err, but code cleanness wasn't a concern
at the time.

So I'll add _reset() to the loop in the next iteration.

> Thanks.
>

Thanks.
--
ZheNing Hu

[PATCH v2 0/2] [GSOC] ref-filter: reuse output buffer

[ZheNing Hu via GitGitGadget]

 * Firstly, inlining show_ref_array_item().
 * Secondly, git for-each-ref reuse final buf for all refs output, the
   performance is slightly improved, this optimization is also applied to
   git tag -l and git branch -l.

Changes made in v2: reset err buffer in loop for code cleanness.

Thanks.

ZheNing Hu (2):
  [GSOC] ref-filter: get rid of show_ref_array_item
  [GSOC] ref-filter: reuse output buffer

 builtin/branch.c       | 10 ++++++----
 builtin/for-each-ref.c | 15 +++++++++++++--
 builtin/tag.c          | 15 +++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 5 files changed, 42 insertions(+), 25 deletions(-)


base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-935%2Fadlternative%2Fref-filter-opt-reuse-buf-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-935/adlternative/ref-filter-opt-reuse-buf-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/935

Range-diff vs v1:

 1:  ec98f2177d90 ! 1:  7e378eef4b34 [GSOC] ref-filter: get rid of show_ref_array_item
     @@ Metadata
       ## Commit message ##
          [GSOC] ref-filter: get rid of show_ref_array_item
      
     -    Inlining the exported function `show_ref_array_item()`
     +    Inlining the exported function `show_ref_array_item()`,
          which is not providing the right level of abstraction,
          simplifies the API and can unlock improvements at the
          former call sites.
 2:  1c7a69ba072a ! 2:  a17949b9f809 [GSOC] ref-filter: reuse output buffer
     @@ Commit message
          [GSOC] ref-filter: reuse output buffer
      
          When we use `git for-each-ref`, every ref will allocate
     -    its own output strbuf. But we can reuse the final strbuf
     -    for each step ref's output.
     +    its own output strbuf and error strbuf. But we can reuse
     +    the final strbuf for each step ref's output. The error
     +    buffer will also be reused, despite the fact that the git
     +    will exit when `format_ref_array_item()` return a non-zero
     +    value and output the contents of the error buffer.
      
          The performance for `git for-each-ref` on the Git repository
          itself with performance testing tool `hyperfine` changes from
     @@ Commit message
          (cat-file: use a single strbuf for all output, 2018-08-14)
          to speed up the cat-file builtin.
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Helped-by: Jeff King <peff@peff.net>
          Helped-by: René Scharfe <l.s.r@web.de>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ builtin/branch.c: static void print_ref_list(struct ref_filter *filter, struct r
       	for (i = 0; i < array.nr; i++) {
      -		struct strbuf out = STRBUF_INIT;
      -		struct strbuf err = STRBUF_INIT;
     ++		strbuf_reset(&err);
      +		strbuf_reset(&out);
       		if (format_ref_array_item(array.items[i], format, &out, &err))
       			die("%s", err.buf);
     @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const
      -		struct strbuf output = STRBUF_INIT;
      -		struct strbuf err = STRBUF_INIT;
      -
     ++		strbuf_reset(&err);
      +		strbuf_reset(&output);
       		if (format_ref_array_item(array.items[i], &format, &output, &err))
       			die("%s", err.buf);
     @@ builtin/tag.c: static int list_tags(struct ref_filter *filter, struct ref_sortin
      -		struct strbuf err = STRBUF_INIT;
      -
      +		strbuf_reset(&output);
     ++		strbuf_reset(&err);
       		if (format_ref_array_item(array.items[i], format, &output, &err))
       			die("%s", err.buf);
       		fwrite(output.buf, 1, output.len, stdout);

-- 
gitgitgadget

[PATCH v2 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Inlining the exported function `show_ref_array_item()`,
which is not providing the right level of abstraction,
simplifies the API and can unlock improvements at the
former call sites.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index cb9c81a04606..8520008604e3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -80,8 +80,18 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
-	for (i = 0; i < maxcount; i++)
-		show_ref_array_item(array.items[i], &format);
+	for (i = 0; i < maxcount; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], &format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d403417b5625..d92d8e110b4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -63,8 +63,18 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format);
+	for (i = 0; i < array.nr; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	free(to_free);
 
diff --git a/ref-filter.c b/ref-filter.c
index f0bd32f71416..df67047fd615 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2435,27 +2435,22 @@ int format_ref_array_item(struct ref_array_item *info,
 	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
-			 const struct ref_format *format)
-{
-	struct strbuf final_buf = STRBUF_INIT;
-	struct strbuf error_buf = STRBUF_INIT;
-
-	if (format_ref_array_item(info, format, &final_buf, &error_buf))
-		die("%s", error_buf.buf);
-	fwrite(final_buf.buf, 1, final_buf.len, stdout);
-	strbuf_release(&error_buf);
-	strbuf_release(&final_buf);
-	putchar('\n');
-}
-
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
+
 	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
-	show_ref_array_item(ref_item, format);
+	if (format_ref_array_item(ref_item, format, &output, &err))
+		die("%s", err.buf);
+	fwrite(output.buf, 1, output.len, stdout);
+	putchar('\n');
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	free_array_item(ref_item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 19ea4c413409..baf72a718965 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,8 +119,6 @@ int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
 			  struct strbuf *final_buf,
 			  struct strbuf *error_buf);
-/*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */
-- 
gitgitgadget

[PATCH v2 2/2] [GSOC] ref-filter: reuse output buffer

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

When we use `git for-each-ref`, every ref will allocate
its own output strbuf and error strbuf. But we can reuse
the final strbuf for each step ref's output. The error
buffer will also be reused, despite the fact that the git
will exit when `format_ref_array_item()` return a non-zero
value and output the contents of the error buffer.

The performance for `git for-each-ref` on the Git repository
itself with performance testing tool `hyperfine` changes from
23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms. Optimization is relatively
minor.

At the same time, we apply this optimization to `git tag -l`
and `git branch -l`.

This approach is similar to the one used by 79ed0a5
(cat-file: use a single strbuf for all output, 2018-08-14)
to speed up the cat-file builtin.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       | 10 ++++++----
 builtin/for-each-ref.c | 13 +++++++------
 builtin/tag.c          | 13 +++++++------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index bcc00bcf182d..b23b1d1752af 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 {
 	int i;
 	struct ref_array array;
+	struct strbuf out = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
 	char *to_free = NULL;
@@ -440,8 +442,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf out = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
+		strbuf_reset(&err);
+		strbuf_reset(&out);
 		if (format_ref_array_item(array.items[i], format, &out, &err))
 			die("%s", err.buf);
 		if (column_active(colopts)) {
@@ -452,10 +454,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 			fwrite(out.buf, 1, out.len, stdout);
 			putchar('\n');
 		}
-		strbuf_release(&err);
-		strbuf_release(&out);
 	}
 
+	strbuf_release(&err);
+	strbuf_release(&out);
 	ref_array_clear(&array);
 	free(to_free);
 }
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8520008604e3..b529228c6239 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_array array;
 	struct ref_filter filter;
 	struct ref_format format = REF_FORMAT_INIT;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -81,17 +83,16 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
 	for (i = 0; i < maxcount; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&err);
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], &format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d92d8e110b4d..82fcfc098242 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 		     struct ref_format *format)
 {
 	struct ref_array array;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	char *to_free = NULL;
 	int i;
 
@@ -64,17 +66,16 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
+		strbuf_reset(&err);
 		if (format_ref_array_item(array.items[i], format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	free(to_free);
 
-- 
gitgitgadget