git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Jacob VosmaerNext: Junio C Hamano
Message 2 of 27 in “upload-pack: buffer ref advertisement writes”
  1. 1/1 upload-pack: buffer ref advertisement writesJacob Vosmaer, Aug 24, 2021
  2. Taylor BlauAug 24, 2021
  3. Junio C HamanoAug 24, 2021
  4. Jeff KingAug 25, 2021
  5. Jacob VosmaerAug 26, 2021
  6. 1/2 pkt-line: add packet_fwriteJacob Vosmaer, Aug 26, 2021
  7. 2/2 upload-pack: use stdio in send_ref callbacksJacob Vosmaer, Aug 26, 2021
  8. Junio C HamanoAug 26, 2021
  9. Junio C HamanoAug 26, 2021
  10. Taylor BlauAug 26, 2021
  11. Jeff KingAug 26, 2021
  12. Junio C HamanoAug 27, 2021
  13. 0/2 send_ref bufferingJacob Vosmaer, Aug 31, 2021
  14. 1/2 pkt-line: add stdio packet write functionsJacob Vosmaer, Aug 31, 2021
  15. Jeff KingAug 31, 2021
  16. Junio C HamanoAug 31, 2021
  17. 0/2 send_ref bufferingJacob Vosmaer, Sep 1, 2021
  18. 2/2 upload-pack: use stdio in send_ref callbacksJacob Vosmaer, Sep 1, 2021
  19. 1/2 pkt-line: add stdio packet write functionsJacob Vosmaer, Sep 1, 2021
  20. Jeff KingSep 2, 2021
  21. 2/2 upload-pack: use stdio in send_ref callbacksJacob Vosmaer, Aug 31, 2021
  22. Jeff KingAug 31, 2021
  23. Jacob VosmaerAug 31, 2021
  24. Jacob VosmaerAug 31, 2021
  25. Jeff KingSep 1, 2021
  26. Jeff KingAug 26, 2021
  27. Junio C HamanoAug 26, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.