Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes
From: Taylor Blau <hidden>
Date: 2021-08-24 21:07:58
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.
quoted hunk ↗ jump to hunk
--- 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