Re: [PATCH] ls-refs: reuse buffer when sending refs

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 25, 2021, 14:50 UTC
Message-ID: <705fee2c-7711-ac99-f692-ab08cd7b4e26@gmail.com>
In-Reply-To: <ccd03e685af0f5cf25c68272a758fc88d115e37a.1629899211.git.ps@pks.im>

On 8/25/2021 9:49 AM, Patrick Steinhardt wrote:

Show 5 quoted lines

> In the initial reference advertisement, the Git server will first
> announce all of its references to the client. The logic is handled in
> `send_ref()`, which will allocate a new buffer for each refline it is
> about to send. This is quite wasteful: instead of allocating a new
> buffer each time, we can just reuse a buffer.

Reusing a buffer makes perfect sense and is a clear improvement.

Show 23 quoted lines

> Improve this by passing in a buffer via the `ls_refs_data` struct which
> is then reused on each reference. In a repository with about 2.3M refs,
> this speeds up local mirror fetches by about 2%:
> 
>     Benchmark #1: HEAD~: git-fetch
>       Time (mean ± σ):     25.415 s ±  0.131 s    [User: 22.722 s, System: 4.740 s]
>       Range (min … max):   25.240 s … 25.543 s    5 runs
> 
>     Benchmark #2: HEAD: git-fetch
>       Time (mean ± σ):     24.922 s ±  0.110 s    [User: 22.404 s, System: 4.476 s]
>       Range (min … max):   24.825 s … 25.081 s    5 runs
> 
>     Summary
>       'HEAD: git-fetch' ran
>         1.02 ± 0.01 times faster than 'HEAD~: git-fetch'
> 
> Signed-off-by: Patrick Steinhardt <ps@kps.im>
> ---
> 
> Note that while this topic applies on top of "master", I've done the
> benchmark on top of my other optimizations for fetches. It's cheating a
> bit, but it's easier to see that the optimization does something when
> the remaining constant part is lower.

I don't mind demonstrating an optimization using the other work.

Perhaps this would be better grouped with those other changes? I know that the text is independent and merges cleanly without it, but it can be helpful to think about the effort as one unified topic instead of juggling multiple, especially because I don't see the other one needing many revisions.

> -	struct strbuf refline = STRBUF_INIT;
> +
> +	strbuf_reset(&data->buf);

It's nice that this is the only _real_ change, and everything else is a find-and-replace.

Show 15 quoted lines

> @@ -145,6 +146,7 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  
>  	memset(&data, 0, sizeof(data));
>  	strvec_init(&data.prefixes);
> +	strbuf_init(&data.buf, 0);
>  
>  	ensure_config_read();
>  	git_config(ls_refs_config, NULL);
> @@ -173,6 +175,7 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  				     send_ref, &data, 0);
>  	packet_flush(1);
>  	strvec_clear(&data.prefixes);
> +	strbuf_release(&data.buf);
>  	return 0;
>  }

Except, of course, these two lines.

I think this patch is good to go!

Thanks, -Stolee