From: Taylor Blau <me@ttaylorr.com>
To: Jacob Vosmaer <jacob@gitlab.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes
Date: Tue, 24 Aug 2021 17:07:55 -0400 [thread overview]
Message-ID: <YSVfq9lZGdSRCcP9@nand.local> (raw)
In-Reply-To: <20210824140259.89332-1-jacob@gitlab.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
next prev parent reply other threads:[~2021-08-24 21:07 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-24 14:02 [PATCH 1/1] upload-pack: buffer ref advertisement writes Jacob Vosmaer
2021-08-24 21:07 ` Taylor Blau [this message]
2021-08-24 21:42 ` Junio C Hamano
2021-08-25 0:44 ` Jeff King
2021-08-26 10:02 ` Jacob Vosmaer
2021-08-26 10:06 ` [PATCH 1/2] pkt-line: add packet_fwrite Jacob Vosmaer
2021-08-26 10:06 ` [PATCH 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-08-26 16:33 ` Junio C Hamano
2021-08-26 20:21 ` Junio C Hamano
2021-08-26 22:35 ` Taylor Blau
2021-08-26 23:24 ` Jeff King
2021-08-27 16:15 ` Junio C Hamano
2021-08-31 9:34 ` [PATCH v3 0/2] send_ref buffering Jacob Vosmaer
2021-08-31 9:34 ` [PATCH v3 1/2] pkt-line: add stdio packet write functions Jacob Vosmaer
2021-08-31 10:37 ` Jeff King
2021-08-31 18:13 ` Junio C Hamano
2021-09-01 12:54 ` [PATCH v4 0/2] send_ref buffering Jacob Vosmaer
2021-09-01 12:54 ` [PATCH v4 1/2] pkt-line: add stdio packet write functions Jacob Vosmaer
2021-09-01 12:54 ` [PATCH v4 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-09-02 9:18 ` [PATCH v4 0/2] send_ref buffering Jeff King
2021-08-31 9:34 ` [PATCH v3 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-08-31 10:25 ` [PATCH v3 0/2] send_ref buffering Jeff King
2021-08-31 13:08 ` Jacob Vosmaer
2021-08-31 17:44 ` Jacob Vosmaer
2021-09-01 0:15 ` Jeff King
2021-08-26 23:32 ` [PATCH 2/2] upload-pack: use stdio in send_ref callbacks Jeff King
2021-08-26 16:33 ` [PATCH 1/2] pkt-line: add packet_fwrite Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YSVfq9lZGdSRCcP9@nand.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=jacob@gitlab.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).