ref-filter: reuse output buffer

PATCH Superseded ZheNing Hu via GitGitGadget 3 participants 9 messages Apr 19, 2021 – Apr 20, 2021

Share & tools

ZheNing Hu via GitGitGadget

author

2021-04-19 11:28 UTC #0 lore raw

cover ref-filter: reuse output buffer

* 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

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-04-19 11:28 UTC #1 lore raw

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

From: ZheNing Hu <[email protected]>

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 <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 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(-)

builtin/for-each-ref.c +12 -2

index cb9c81a04606..8520008604e3 100644
@@ -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;
 }

builtin/tag.c +12 -2

index d403417b5625..d92d8e110b4d 100644
@@ -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);

ref-filter.c +10 ~~-15~~

index f0bd32f71416..df67047fd615 100644
@@ -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);
 }

ref-filter.h +0 -2

index 19ea4c413409..baf72a718965 100644
@@ -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 */

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-04-19 20:55 UTC #2 lore raw

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

Quoted 6 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>
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.

ZheNing Hu via GitGitGadget

author

2021-04-19 11:28 UTC #3 lore raw

PATCH 2/2 ref-filter: reuse output buffer

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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 <[email protected]>

Helped-by René Scharfe <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/branch.c       |  9 +++++----
 builtin/for-each-ref.c | 12 ++++++------
 builtin/tag.c          | 12 ++++++------
 3 files changed, 17 insertions(+), 16 deletions(-)

builtin/branch.c +5 -4

index bcc00bcf182d..00081de1aed8 100644
@@ -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);
 }

builtin/for-each-ref.c +6 -6

index 8520008604e3..bf24c595c526 100644
@@ -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;
 }

builtin/tag.c +6 -6

index d92d8e110b4d..592af1d154ea 100644
@@ -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);

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-04-19 21:04 UTC #4 lore raw

Quoted 14 lines · ZheNing Hu via GitGitGadget

builtin/branch.c +2 -0

index bcc00bcf182d..00081de1aed8 100644
@@ -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;

builtin/branch.c static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin · line 447

Quoted 8 lines

builtin/branch.c +1 -2

@@ -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 ...

builtin/branch.c static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin · line 459

Quoted 10 lines

builtin/branch.c +2 -2

@@ -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.

ZheNing Hu

2021-04-20 06:05 UTC #5 lore raw

Quoted 40 lines · Junio C Hamano <[email protected]> 于2021年4月20日周二上午5:04写道：

Quoted 14 lines · ZheNing Hu via GitGitGadget

builtin/branch.c +2 -0

index bcc00bcf182d..00081de1aed8 100644
@@ -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;

builtin/branch.c static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin · line 447

Quoted 8 lines

builtin/branch.c +1 -2

@@ -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);

Quoted 8 lines

builtin/branch.c +0 -2

@@ -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);
      }

Quoted 5 lines

builtin/branch.c static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin · line 459
Quoted 2 lines
builtin/branch.c +2 -0
+     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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

Thanks.

Thanks.
--
ZheNing Hu

ZheNing Hu via GitGitGadget

author

2021-04-20 16:52 UTC #6 lore raw

v2 cover ref-filter: reuse output buffer

In reply to ZheNing Hu via GitGitGadget

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 <[email protected]>

Helped-by Jeff King <[email protected]>

Helped-by René Scharfe <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

@@ 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);

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-04-20 16:52 UTC #7 lore raw

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

From: ZheNing Hu <[email protected]>

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 <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 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(-)

builtin/for-each-ref.c +12 -2

index cb9c81a04606..8520008604e3 100644
@@ -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;
 }

builtin/tag.c +12 -2

index d403417b5625..d92d8e110b4d 100644
@@ -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);

ref-filter.c +10 ~~-15~~

index f0bd32f71416..df67047fd615 100644
@@ -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);
 }

ref-filter.h +0 -2

index 19ea4c413409..baf72a718965 100644
@@ -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 */

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-04-20 16:52 UTC #8 lore raw

PATCH v2 2/2 ref-filter: reuse output buffer

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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 <[email protected]>

Helped-by Jeff King <[email protected]>

Helped-by René Scharfe <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/branch.c       | 10 ++++++----
 builtin/for-each-ref.c | 13 +++++++------
 builtin/tag.c          | 13 +++++++------
 3 files changed, 20 insertions(+), 16 deletions(-)

builtin/branch.c +6 -4

index bcc00bcf182d..b23b1d1752af 100644
@@ -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);
 }

builtin/for-each-ref.c +7 -6

index 8520008604e3..b529228c6239 100644
@@ -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;
 }

builtin/tag.c +7 -6

index d92d8e110b4d..82fcfc098242 100644
@@ -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);

Signature · 1 line

gitgitgadget