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

From: Jeff King <peff@peff.net>
Date: Apr 5, 2021, 22:17 UTC
Message-ID: <YGuMaxoYgRkUR1sa@coredump.intra.peff.net>
In-Reply-To: <pull.927.git.1617631280402.gitgitgadget@gmail.com>

On Mon, Apr 05, 2021 at 02:01:19PM +0000, ZheNing Hu via GitGitGadget wrote:

Show 22 quoted lines

> 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

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

>     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