ref-filter: use single strbuf for all output

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

[...]

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

---

builtin/for-each-ref.c +3 -1

@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
        struct ref_format format = REF_FORMAT_INIT;
+       struct strbuf final_buf = STRBUF_INIT;
+       struct strbuf error_buf = STRBUF_INIT;
 
        for (i = 0; i < maxcount; i++)
-               show_ref_array_item(array.items[i], &format);
+               show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);
        ref_array_clear(&array);
        return 0;

}

ref-filter.c +3 -5

@@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,
 void show_ref_array_item(struct ref_array_item *info,
-                        const struct ref_format *format)
+                        const struct ref_format *format,
+                        struct strbuf *final_buf,
+                        struct strbuf *error_buf)
 {
-       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);

Quoted 7 lines · ZheNing Hu via GitGitGadget

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

[...]

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

---

Was there a discussion leading up to this change? If so, it may be a good idea to provide a link to it in the mailing list here under the "---" line.

Some comments below...

Quoted 11 lines

builtin/for-each-ref.c +3 -1

@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
        struct ref_format format = REF_FORMAT_INIT;
+       struct strbuf final_buf = STRBUF_INIT;
+       struct strbuf error_buf = STRBUF_INIT;
 
        for (i = 0; i < maxcount; i++)
-               show_ref_array_item(array.items[i], &format);
+               show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);
        ref_array_clear(&array);
        return 0;

Quoted 1 line

}

The call to strbuf_reset() within show_ref_array_item() does not free the memory from the strbufs, so the memory is being leaked. Therefore, at the end of this function, you should have:

    strbuf_release(final_buf);
    strbuf_release(error_buf);

builtin/tag.c static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, · line 46

Quoted 11 lines

builtin/tag.c +3 -1

@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
        struct ref_array array;
+       struct strbuf final_buf = STRBUF_INIT;
+       struct strbuf error_buf = STRBUF_INIT;
 
        for (i = 0; i < array.nr; i++)
-               show_ref_array_item(array.items[i], format);
+               show_ref_array_item(array.items[i], format, &final_buf, &error_buf);
        ref_array_clear(&array);
        free(to_free);

Leaking final_buf and error_buf.

Quoted 13 lines

ref-filter.c +3 -5

@@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,
 void show_ref_array_item(struct ref_array_item *info,
-                        const struct ref_format *format)
+                        const struct ref_format *format,
+                        struct strbuf *final_buf,
+                        struct strbuf *error_buf)
 {
-       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);

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2448

Quoted 10 lines

ref-filter.c +5 -3

-       fwrite(final_buf.buf, 1, final_buf.len, stdout);
-       strbuf_release(&error_buf);
-       strbuf_release(&final_buf);
+       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_reset(error_buf);
+       strbuf_reset(final_buf);
        putchar('\n');
 }

A couple comments:

It is especially ugly that error_buf needs to be passed in by the caller since it is only ever used in case of an error, at which point the program will die() anyhow, so it's not on a critical, speed-sensitive path. The initialization with STRBUF_INIT is practically cost-free, so this variable could easily stay local to this function without cost-penalty rather than forcing the caller to pass it in. (This assumes that none of the consumers of error_buf down the line insert into the buffer unnecessarily, which is probably a reasonable assumption.)

It is an unsafe assumption to only call strbuf_reset() at the end of the function. For this to be robust, you can't assume that the caller has given you an empty strbuf. Instead, you must ensure it by calling strbuf_reset() at the start. (It doesn't hurt to also call strbuf_reset() at the end, but that is not critical to correct operation, so could be omitted.)

ref-filter.c void pretty_print_ref(const char *name, const struct object_id *oid, · line 2461

Quoted 11 lines

ref-filter.c +4 -1

@@ -2453,9 +2453,12 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
        struct ref_array_item *ref_item;
+       struct strbuf final_buf = STRBUF_INIT;
+       struct strbuf error_buf = 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);
+       show_ref_array_item(ref_item, format, &final_buf, &error_buf);
        free_array_item(ref_item);
 }

Leaking final_buf and error_buf.

ref-filter.h int format_ref_array_item(struct ref_array_item *info, · line 124

Quoted 8 lines

ref-filter.h +4 -1

@@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,
 /*  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);
+void show_ref_array_item(struct ref_array_item *info,
+                        const struct ref_format *format,
+                        struct strbuf *final_buf,
+                        struct strbuf *error_buf);

It is not clear to the person reading this what these two new arguments are for or what should be provided, so the comment above the function deserves an update explaining what these arguments are for and how to provide them. This is especially important since this is a public function.

If this function was merely internal to some builtin command, this sort of change for the sake of optimization might not be so bad, but as a public function, it comes across as rather ugly. In general, we don't necessarily want to pollute an otherwise clean API with changes which make the API uglier merely for the sake of tiny optimizations like this (IMHO). The extra burden placed on callers by this change, coupled with the ugliness this introduces into the API, makes the change seem less than desirable.

One way you might be able to mitigate the undesirableness would be to have two versions of this function. For instance:

    /* Print the ref using the given format and quote_style */
    show_ref_array_item(...);
   /* This is like show_ref_array_item() but ... */
    show_ref_array_item_optim(...);

The comment of the second function would, of course, need to explain that it is similar to show_ref_array_item() but more optimal because <some reasons> and that the caller is responsible for allocating and releasing the strbufs, and that the strbufs are used only for temporary storage, thus the caller should not assume anything about them.

This way, callers which don't invoke show_ref_array_item() in a tight loop don't need to be burdened by the new arguments (and won't have to remember to release them), and callers with a tight loop can take advantage of the optimization with the bit of extra work of having to declare and release the strbufs.

So, having said all that, it's not clear that the ugliness and extra work are worth the gain. However, if it is decided that the change is worthwhile, then the commit message probably should explain cases in which such an optimization will be really beneficial.

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

to:

2.89s user
4.00s system
34% cpu
19.741 total

The performance has been slightly improved.

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

---
[GSOC] ref-filter: use single strbuf for all output

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

Quoted 6 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

s/two single strbuf/two buffers/

Quoted 18 lines

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

to:

2.89s user
4.00s system
34% cpu
19.741 total

Quoted 1 line

The performance has been slightly improved.

This is a nice amount of time! Perhaps simplify the presentation:

The performance for 'git for-each-ref' on the Git repository itself with X references changes from 3.2 seconds to 2.9 seconds.

Quoted 7 lines

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

---
[GSOC] ref-filter: use single strbuf for all output

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

This part of the cover letter could be put into the commit message itself. Use the correct formatting, though:

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.

I found the code change to be correct. Thanks!
-Stolee

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

to:

2.89s user
4.00s system
34% cpu
19.741 total

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

Quoted 16 lines · ZheNing Hu via GitGitGadget

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

Quoted 6 lines

to:

2.89s user
4.00s system
34% cpu
19.741 total

That's a bigger performance improvement than I'd expect from this. I'm having trouble reproducing it here (I get the same time before and after). I also notice that you don't seem to be CPU bound, and we spend most of our time on system CPU (so program startup stuff, not the loop you're optimizing).

I think a more interesting test is timing a single invocation with a large number of refs. If you are planning to do a lot of work on the formatting code, it might be worth adding such a test into t/perf (both to show off results, but also to catch any regressions after we make things faster).

Quoted 3 lines

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

I do think this is a good direction (for all the reasons laid out in 79ed0a5), though it wasn't actually the part I was most worried about for ref-filter performance. The bigger issue in ref-filter is that each individual atom will generally have its own allocated string, and then we'll format all of the atoms into the final strbuf output. In most cases we could avoid those intermediate copies entirely.

I do think this would be a useful optimization to have in addition, though. As for the patch itself, I looked over the review that Eric gave, and he already said everything I would have. :)

-Peff

But this is the first time I use t/perf/* and there is a little problem. It seem like whatever I run single script like sh ./p0007-write-cache.sh or just make or ./run ${HOME}/git -- ./p0002-read-cache.sh , these tests will fail.

Quoted 5 lines · ZheNing Hu <[email protected]> 于2021年4月6日周二 下午5:49写道：

But this is the first time I use t/perf/* and there is a little problem. It seem like whatever I run single script like sh ./p0007-write-cache.sh or just make or ./run ${HOME}/git -- ./p0002-read-cache.sh , these tests will fail.

It's because I don't have /usr/bin/time, solved after installation. So best have this:

Quoted 12 lines · ZheNing Hu

Quoted 5 lines · ZheNing Hu <[email protected]> 于2021年4月6日周二 下午5:49写道：

But this is the first time I use t/perf/* and there is a little problem. It seem like whatever I run single script like sh ./p0007-write-cache.sh or just make or ./run ${HOME}/git -- ./p0002-read-cache.sh , these tests will fail.

It's because I don't have /usr/bin/time, solved after installation. So best have this:

t/perf/perf-lib.sh immediate=t · line 158

Quoted 7 lines

t/perf/perf-lib.sh +4 -0

@@ -152,6 +152,10 @@ immediate=t
 # Perf tests require GNU time
 case "$(uname -s)" in Darwin) GTIME="${GTIME:-gtime}";; esac
 GTIME="${GTIME:-/usr/bin/time}"
+if ! test -f "$GTIME"
+then
+       error "command not found: "$GTIME""
+fi

This patch would create problems when we expect to find the value of $GTIME in the $PATH e.g., you can see in the Darwin case it is set to just "gtime", not an absolute path).

I am sympathetic to helping people see what's wrong, but I think in this case we're better off pointing people to using "-v". E.g.:

  $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh
  perf 1 - rev-list --all:
  not ok 1 - rev-list --all
  #
  #             git rev-list --all >/dev/null
  #

Uh oh, that wasn't very informative. But how about this:

  $ GTIME=pretend-we-do-not-have-gtime ./p0001-rev-list.sh -v
  [...]
  perf 1 - rev-list --all:
  running:
        git rev-list --all >/dev/null

  ./p0001-rev-list.sh: 160: pretend-we-do-not-have-gtime: not found
  not ok 1 - rev-list --all
  #
  #             git rev-list --all >/dev/null
  #

which I think makes it reasonably clear.

-Peff

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

to:

2.89s user
4.00s system
34% cpu
19.741 total

The performance has been slightly improved.

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

---
[GSOC] ref-filter: use single strbuf for all output

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

So ref-filter can learn same thing: use single buffer: final_buf and error_buf for all refs output.

Thanks.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/927

 builtin/for-each-ref.c |  4 +++-
 builtin/tag.c          |  4 +++-
 ref-filter.c           | 21 ++++++++++++---------
 ref-filter.h           |  5 ++++-
 4 files changed, 22 insertions(+), 12 deletions(-)

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

index cb9c81a04606..9dc41f48bfa0 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 final_buf = STRBUF_INIT;
+	struct strbuf error_buf = STRBUF_INIT;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,

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

 	ref_array_clear(&array);
 	return 0;
 }

builtin/tag.c +2 -0

index d403417b5625..8a38b3e2de34 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 final_buf = STRBUF_INIT;
+	struct strbuf error_buf = STRBUF_INIT;
 	char *to_free = NULL;
 	int i;
 

builtin/tag.c +0 -0

 	ref_array_clear(&array);
 	free(to_free);
 

ref-filter.c +8 -8

index f0bd32f71416..51ff6af64ebc 100644
@@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,
 }
 
 void show_ref_array_item(struct ref_array_item *info,
-			 const struct ref_format *format)
+			 const struct ref_format *format,
+			 struct strbuf *final_buf,
+			 struct strbuf *error_buf)
 {
-	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);
+	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_reset(error_buf);
+	strbuf_reset(final_buf);
 	putchar('\n');
 }
 

ref-filter.c +0 -0

 	free_array_item(ref_item);
 }
 

ref-filter.h +0 -0

index 19ea4c413409..95498c9f4467 100644

ref-filter.h +0 -0

 /*  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 */

base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48

Am 05.04.21 um 16:01 schrieb ZheNing Hu via GitGitGadget:

Quoted 25 lines

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can provide two single strbuf:
final_buf and error_buf that get reused for each output.

When run it 100 times:

$ git for-each-ref

on git.git :

3.19s user
3.88s system
35% cpu
20.199 total

to:

2.89s user
4.00s system
34% cpu
19.741 total

Quoted 1 line

The performance has been slightly improved.

I like to use hyperfine (https://github.com/sharkdp/hyperfine) to get more stable benchmark numbers, incl. standard deviation. With three warmup runs I get the following results for running git for-each-ref on Git's own repo with the current master (2e36527f23):

Benchmark #1: ./git for-each-ref

    Time (mean ± σ):      18.8 ms ±   0.3 ms    [User: 12.7 ms, System: 5.6 ms]
    Range (min … max):    18.2 ms …  19.8 ms    148 runs

With your patch on top I get this:

Benchmark #1: ./git for-each-ref

    Time (mean ± σ):      18.5 ms ±   0.4 ms    [User: 12.3 ms, System: 5.6 ms]
    Range (min … max):    17.8 ms …  19.6 ms    147 runs

So there seems to be a slight improvement here, but it is within the noise.

I'm quite surprised how much longer this takes on your machine, however, and (like Peff already mentioned) how much of the total time it spends in system calls. Is an antivirus program or similar interferring? Or some kind of emulator or similar, e.g. Valgrind? Or has it been a long time since you ran "git gc"?

The benchmark certainly depends on the number of local and remote branches in the repo; my copy currently has 4304 according to "git for-each-ref | wc -l".

Quoted 38 lines

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

---
[GSOC] ref-filter: use single strbuf for all output

This patch learned Jeff King's optimization measures in git cat-file(79ed0a5): using a single strbuf for all objects output Instead of allocating a large number of small strbuf for every object.

So ref-filter can learn same thing: use single buffer: final_buf and error_buf for all refs output.

Thanks.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-927%2Fadlternative%2Fref-filter-single-buf-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-927/adlternative/ref-filter-single-buf-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/927

 builtin/for-each-ref.c |  4 +++-
 builtin/tag.c          |  4 +++-
 ref-filter.c           | 21 ++++++++++++---------
 ref-filter.h           |  5 ++++-
 4 files changed, 22 insertions(+), 12 deletions(-)

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

index cb9c81a04606..9dc41f48bfa0 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 final_buf = STRBUF_INIT;
+     struct strbuf error_buf = STRBUF_INIT;
 
      struct option opts[] = {
              OPT_BIT('s', "shell", &format.quote_style,

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 86

Quoted 5 lines

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

@@ -81,7 +83,7 @@ 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);
+             show_ref_array_item(array.items[i], &format, &final_buf, &error_buf);

This user of show_ref_array_item() calls it in a loop on an array.

Quoted 17 lines

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

      ref_array_clear(&array);
      return 0;
 }

builtin/tag.c +2 -0

index d403417b5625..8a38b3e2de34 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 final_buf = STRBUF_INIT;
+     struct strbuf error_buf = STRBUF_INIT;
      char *to_free = NULL;
      int i;
 

builtin/tag.c static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, · line 69

Quoted 5 lines

builtin/tag.c +1 -1

@@ -64,7 +66,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
      ref_array_sort(sorting, &array);
 
      for (i = 0; i < array.nr; i++)
-             show_ref_array_item(array.items[i], format);
+             show_ref_array_item(array.items[i], format, &final_buf, &error_buf);

Dito.

Quoted 33 lines

builtin/tag.c +0 -0

      ref_array_clear(&array);
      free(to_free);
 

ref-filter.c +8 -8

index f0bd32f71416..51ff6af64ebc 100644
@@ -2436,16 +2436,16 @@ int format_ref_array_item(struct ref_array_item *info,
 }
 
 void show_ref_array_item(struct ref_array_item *info,
-                      const struct ref_format *format)
+                      const struct ref_format *format,
+                      struct strbuf *final_buf,
+                      struct strbuf *error_buf)
 {
-     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);
+     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_reset(error_buf);
+     strbuf_reset(final_buf);
      putchar('\n');
 }
 

ref-filter.c void pretty_print_ref(const char *name, const struct object_id *oid, · line 2461

Quoted 10 lines

ref-filter.c +4 -1

@@ -2453,9 +2453,12 @@ 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 final_buf = STRBUF_INIT;
+     struct strbuf error_buf = 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);
+     show_ref_array_item(ref_item, format, &final_buf, &error_buf);

This third and final caller works with a single item; there is no loop.

Quoted 14 lines

ref-filter.c +0 -0

      free_array_item(ref_item);
 }
 

ref-filter.h +2 -1

index 19ea4c413409..95498c9f4467 100644
@@ -120,7 +120,10 @@ int format_ref_array_item(struct ref_array_item *info,
                        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);
+void show_ref_array_item(struct ref_array_item *info,
+                      const struct ref_format *format,

ref-filter.h int format_ref_array_item(struct ref_array_item *info, · line 126

Quoted 2 lines

ref-filter.h +2 -0

+                      struct strbuf *final_buf,
+                      struct strbuf *error_buf);

This bring-your-own-buffer approach pushes responsibilities back to the callers, in exchange for improved performance. The number of users of this interface is low, so that's defensible. But that added effort is also non-trivial -- as you demonstrated by leaking the allocated memory. ;-)

How about offering to do more instead? In particular you could add a count parameter and have show_ref_array_item() handle an array of struct ref_array_item objects. It could reuse the buffers internally to get the same performance benefit, and would free callers from having to iterate loops themselves. Something like:

void show_ref_array_items(struct ref_array_item **info,
size_t n,
const struct ref_format *format);

Callers that deal with a single element can pass n = 1.

Perhaps the "format" parameter should go first, like with printf.

The double reference in "**info" is a bit ugly, though (array of pointers instead of a simple array of objects). That's dictated by struct ref_array_item containing a flexible array member, which seems to be hard to change.

Quoted 6 lines

ref-filter.h +0 -0

 /*  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 */

base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48

Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can reuse these two 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 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.

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.

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

---
[GSOC] ref-filter: use single strbuf for all output

Now git for-each-ref can reuse two buffers for all refs output, the performance is slightly improved.

Now there may be a question : Should the original interface show_ref_array_items be retained?
...

       /*  Callback function for parsing the sort option */

 builtin/for-each-ref.c |  4 +---
 ref-filter.c           | 20 ++++++++++++++++++++
 ref-filter.h           |  5 +++++
 3 files changed, 26 insertions(+), 3 deletions(-)

builtin/for-each-ref.c +0 -1

index cb9c81a04606..d630402230f3 100644
@@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {
 
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	int maxcount = 0, icase = 0;
 	struct ref_array array;

Quoted 1 line · ZheNing Hu via GitGitGadget

Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output

The implementation changed so much from the initial attempt, for which the above title may have been appropriate, that reusing single strbuf over and over is not the most important part of the change anymore, I am afraid. Besides, it uses TWO strbufs ;-)

Subject: [PATCH] ref-filter: introduce show_ref_array_items() helper

or something like that?

Quoted 22 lines

From: ZheNing Hu <[email protected]>

When we use git for-each-ref, every ref will call show_ref_array_item() and allocate its own final strbuf and error strbuf. Instead, we can reuse these two 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 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.

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.

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

---
[GSOC] ref-filter: use single strbuf for all output

Now git for-each-ref can reuse two buffers for all refs output, the performance is slightly improved.

Quoted 4 lines

Now there may be a question : Should the original interface show_ref_array_items be retained?
...

       /*  Callback function for parsing the sort option */

Again, not a very useful range-diff as the implementation changed so much.

Quoted 20 lines

 builtin/for-each-ref.c |  4 +---
 ref-filter.c           | 20 ++++++++++++++++++++
 ref-filter.h           |  5 +++++
 3 files changed, 26 insertions(+), 3 deletions(-)

builtin/for-each-ref.c +0 -1

index cb9c81a04606..d630402230f3 100644
@@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {
 
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
-     int i;
      struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
      int maxcount = 0, icase = 0;
      struct ref_array array;

@@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 82

Quoted 5 lines

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

      if (!maxcount || array.nr < maxcount)
              maxcount = array.nr;
-     for (i = 0; i < maxcount; i++)
-             show_ref_array_item(array.items[i], &format);
+     show_ref_array_items(array.items, &format, maxcount);

The intention of this call is to pass an array and the number of elements in the array as a pair to the function, right? When you design the API for a new helper function, do not split them apart by inserting an unrelated parameter in the middle.

Quoted 8 lines

ref-filter.c +0 -0

index f0bd32f71416..27bbf9b6c8ac 100644
@@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,
      return 0;
 }
 

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2440

Quoted 3 lines

ref-filter.c +3 -0

+void show_ref_array_items(struct ref_array_item **info,
+                      const struct ref_format *format,
+                      size_t n)

IOW,

void show_ref_array_items(const struct ref_format *format,
struct ref_array_item *info[], size_t n)

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2448

Quoted 8 lines

ref-filter.c +8 -0

+{
+     struct strbuf final_buf = STRBUF_INIT;
+     struct strbuf error_buf = STRBUF_INIT;
+     size_t i;
+
+     for (i = 0; i < n; i++) {
+             if (format_ref_array_item(info[i], format, &final_buf, &error_buf))
+                     die("%s", error_buf.buf);

OK, the contents of error_buf is already localized, so it is correct not to have _() around the "%s" here.

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2452

Quoted 4 lines

ref-filter.c +4 -0

+             fwrite(final_buf.buf, 1, final_buf.len, stdout);
+             strbuf_reset(&error_buf);
+             strbuf_reset(&final_buf);
+             putchar('\n');

This is inherited code, but splitting fwrite() and putchar() apart like this makes the code hard to follow. Perhaps clean it up later when nothing else is going on in the code as leftoverbits, outside the topic.

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2460

Quoted 8 lines

ref-filter.c +5 -0

+     }
+     strbuf_release(&error_buf);
+     strbuf_release(&final_buf);
+}
+
 void show_ref_array_item(struct ref_array_item *info,
                       const struct ref_format *format)
 {

Isn't the point of the new helper function so that this can become a thin wrapper around it, i.e.

        void show_ref_array_item(...)
        {
                show_ref_array_items(format, &info, 1);
        }

ref-filter.h int format_ref_array_item(struct ref_array_item *info, · line 127

Quoted 12 lines

ref-filter.h +4 -0

index 19ea4c413409..eb7e79a6676d 100644
@@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,
                        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);
+/*  Print the refs using the given format and quote_style and maxcount */
+void show_ref_array_items(struct ref_array_item **info,
+                      const struct ref_format *format,
+                      size_t n);

The inconsistency between "maxcount" vs "n" is irritating. Calling the parameter with a name that has the word "info" (because the new parameter is about that array) and a word like "nelem" to hint that it is the number of elements in the array) would be sensible.

void show_ref_array_items(const struct ref_format *format,

                          struct ref_array_item *info[], size_t info_count);

or something along the line, perhaps?

ref-filter.c +9 -0

index f0bd32f71416..27bbf9b6c8ac 100644
@@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,
 	return 0;
 }
 
+void show_ref_array_items(struct ref_array_item **info,
+			 const struct ref_format *format,
+			 size_t n)
+{
+	struct strbuf final_buf = STRBUF_INIT;
+	struct strbuf error_buf = STRBUF_INIT;
+	size_t i;
+
+	for (i = 0; i < n; i++) {

Quoted 17 lines · ZheNing Hu via GitGitGadget

ref-filter.c +9 -0

index f0bd32f71416..27bbf9b6c8ac 100644
@@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,
      return 0;
 }
 
+void show_ref_array_items(struct ref_array_item **info,
+                      const struct ref_format *format,
+                      size_t n)
+{
+     struct strbuf final_buf = STRBUF_INIT;
+     struct strbuf error_buf = STRBUF_INIT;
+     size_t i;
+
+     for (i = 0; i < n; i++) {

ref-filter.c int format_ref_array_item(struct ref_array_item *info, · line 2456

Quoted 10 lines

ref-filter.c +10 -0

+             if (format_ref_array_item(info[i], format, &final_buf, &error_buf))
+                     die("%s", error_buf.buf);
+             fwrite(final_buf.buf, 1, final_buf.len, stdout);
+             strbuf_reset(&error_buf);
+             strbuf_reset(&final_buf);
+             putchar('\n');
+     }
+     strbuf_release(&error_buf);
+     strbuf_release(&final_buf);
+}

I think this is a reasonable direction to take the solution: wrapping the loop so that the reuse of the buffers can be included there.

But I do wonder if we should go the opposite direction, and get rid of show_ref_array_item() entirely. It only has two callers, both of which could just write the loop themselves. That is more code, but perhaps it would make it more clear what is going on in those callers, and to give them more flexibility.

I notice there's a third user of the ref-filter.c code in builtin/branch.c that does not use show_ref_array_item(). Instead, it loops itself and calls format_ref_array_item(). I think this is because it is sometimes columnizing the results. Perhaps git-tag and for-each-ref would want to learn the same trick, in which case they'd be happy to have the open-coded loop.

Either way, it probably makes sense to introduce the same optimization to the case in builtin/branch.c.

-Peff

Quoted 9 lines

I think this is a reasonable direction to take the solution: wrapping the loop so that the reuse of the buffers can be included there.

But I do wonder if we should go the opposite direction, and get rid of show_ref_array_item() entirely. It only has two callers, both of which could just write the loop themselves. That is more code, but perhaps it would make it more clear what is going on in those callers, and to give them more flexibility.

Indeed. I think pretty_print_ref() is proof that we may need to keep show_ref_array_item() because If it modified to show_ref_array_items(...,1); it will look very strange.

Quoted 10 lines · ZheNing Hu

Quoted 9 lines

I think this is a reasonable direction to take the solution: wrapping the loop so that the reuse of the buffers can be included there.

But I do wonder if we should go the opposite direction, and get rid of show_ref_array_item() entirely. It only has two callers, both of which could just write the loop themselves. That is more code, but perhaps it would make it more clear what is going on in those callers, and to give them more flexibility.

Quoted 3 lines

Indeed. I think pretty_print_ref() is proof that we may need to keep show_ref_array_item() because If it modified to show_ref_array_items(...,1); it will look very strange.

What I meant was that we should get rid of show_ref_array_items(), as well, and just use format_ref_array_item() everywhere. This whole wrapper is only saving us a few lines, and it makes it harder to see what the function is doing. Likewise for pretty-print ref. But I dunno. Maybe that is all going too far.

-Peff

Quoted 6 lines

What I meant was that we should get rid of show_ref_array_items(), as well, and just use format_ref_array_item() everywhere. This whole wrapper is only saving us a few lines, and it makes it harder to see what the function is doing. Likewise for pretty-print ref. But I dunno. Maybe that is all going too far.

Ok... so you mean we just use a loop like in branch.c, and get rid of show_ref_array_items() and show_ref_array_item().
(We can still use the optimization of reuse bufs)

Quoted 10 lines · ZheNing Hu

Quoted 6 lines

What I meant was that we should get rid of show_ref_array_items(), as well, and just use format_ref_array_item() everywhere. This whole wrapper is only saving us a few lines, and it makes it harder to see what the function is doing. Likewise for pretty-print ref. But I dunno. Maybe that is all going too far.

Ok... so you mean we just use a loop like in branch.c, and get rid of show_ref_array_items() and show_ref_array_item().
(We can still use the optimization of reuse bufs)

Yes, something like this:

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

index cb9c81a046..55297fe297 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,

@@ -80,8 +82,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++)
-               show_ref_array_item(array.items[i], &format);
+
+       for (i = 0; i < maxcount; i++) {
+               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(&output);
        ref_array_clear(&array);
        return 0;
 }

It is dropping a few lines by assuming that the error buf is only touched when we return an error (which IMHO is a reasonable assumption to make).

-Peff