Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 24, 2021 at 04:02:59PM +0200, Jacob Vosmaer wrote:
> In both protocol v0 and v2, upload-pack writes one pktline packet per
> advertised ref to stdout. That means one or two write(2) syscalls per
> ref. This is problematic if these writes become network sends with
> high overhead.
>
> This change adds a strbuf buffer to the send_ref callbacks of the v0
> ref advertisement and v2's ls-refs. Instead of writing directly to
> stdout, send_ref now writes into the buffer, and then checks if there
> are more than 32768 bytes in the buffer. If so we flush the buffer to
> stdout.

Are there any consumers that rely on having information early (where
buffering would be a detriment to them)?

> To give an example: I set up a single-threaded loop that calls
> ls-remote (with HTTP and protocol v2) on a local GitLab instance, on a
> repository with 11K refs. When I switch from Git v2.32.0 to this
> patch, I see a 50% reduction in CPU time for Git, and 75% for Gitaly
> (GitLab's Git RPC service).

Hmm. Seeing a reduction in CPU time is surprising to me: do you have an
explanation for why the time-savings isn't coming purely from "system"
(i.e., any blocking I/O)?

> So having these buffers not only saves syscalls in upload-pack, it
> also saves time in things that consume upload-pack's output.

I don't think this explains it, since any time spent waiting on
upload-pack output should be counted against the system bucket, not CPU
time.

> ---
>  ls-refs.c     | 13 ++++++++++++-
>  upload-pack.c | 18 +++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/ls-refs.c b/ls-refs.c
> index 88f6c3f60d..7b9d5813bf 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -7,6 +7,8 @@
>  #include "pkt-line.h"
>  #include "config.h"
>
> +#define OUT_WRITE_SIZE 32768
> +
>  static int config_read;
>  static int advertise_unborn;
>  static int allow_unborn;
> @@ -65,6 +67,7 @@ struct ls_refs_data {
>  	unsigned peel;
>  	unsigned symrefs;
>  	struct strvec prefixes;
> +	struct strbuf out;
>  	unsigned unborn : 1;
>  };
>
> @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
>  	}
>
>  	strbuf_addch(&refline, '\n');
> -	packet_write(1, refline.buf, refline.len);
> +
> +	packet_buf_write_len(&data->out, refline.buf, refline.len);
> +	if (data->out.len >= OUT_WRITE_SIZE) {
> +		write_or_die(1, data->out.buf, data->out.len);
> +		strbuf_reset(&data->out);
> +	}

None of this looks wrong to me, but it might be nice to teach the
packet writer a buffered mode that would handle this for us. It would be
especially nice to bolt the final flush onto packet_flush(), since it is
otherwise easy to forget to do.

Thanks,
Taylor