Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 24, 2021, 21:07 UTC
- Message-ID
- <YSVfq9lZGdSRCcP9@nand.local>
- In-Reply-To
- <20210824140259.89332-1-jacob@gitlab.com>
On Tue, Aug 24, 2021 at 04:02:59PM +0200, Jacob Vosmaer wrote:
Show 10 quoted lines
> 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)?
Show 5 quoted lines
> 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.
Show 37 quoted lines
> ---
> 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