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
