patchMake upload-pack pack write size configurable

> Add a new compile time constant UPLOAD_PACK_BUFFER_SIZE that makes the
> size of the static buffer in output_state configurable.
>
> The current size of the buffer is 8192+1. The '+1' is a technical
> detail orthogonal to this change. On GitLab.com we use GitLab's
> pack-objects cache which does writes of 65515 bytes. Because of the
> default 8KB buffer size, propagating these cache writes requires 8
> pipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads
> from Gitaly (our Git RPC service). If we increase the size of the
> buffer to the maximum Git packet size, we need only 1 pipe read and 1
> pipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer
> the same amount of data. In benchmarks with a pure fetch and 100%
> cache hit rate workload we are seeing CPU utilization reductions of
> over 30%.
>
> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>
> ---
>  upload-pack.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index c78d55bc67..b799fbe628 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -42,6 +42,10 @@
>  #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \
>  		NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)
>  
> +#ifndef UPLOAD_PACK_BUFFER_SIZE
> +#define UPLOAD_PACK_BUFFER_SIZE 8192
> +#endif
> +
>  /* Enum for allowed unadvertised object request (UOR) */
>  enum allow_uor {
>  	/* Allow specifying sha1 if it is a ref tip. */
> @@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
>  }
>  
>  struct output_state {
> -	char buffer[8193];
> +	char buffer[UPLOAD_PACK_BUFFER_SIZE+1];
>  	int used;
>  	unsigned packfile_uris_started : 1;
>  	unsigned packfile_started : 1;

> > +#ifndef UPLOAD_PACK_BUFFER_SIZE
> > +#define UPLOAD_PACK_BUFFER_SIZE 8192
> > +#endif
> > +
> >  /* Enum for allowed unadvertised object request (UOR) */
> >  enum allow_uor {
> >  	/* Allow specifying sha1 if it is a ref tip. */
> > @@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> >  }
> >  
> >  struct output_state {
> > -	char buffer[8193];
> > +	char buffer[UPLOAD_PACK_BUFFER_SIZE+1];
> >  	int used;
> >  	unsigned packfile_uris_started : 1;
> >  	unsigned packfile_started : 1;
> 
> Making this configurable obviousl has big impact in some cases, but I'm
> a bit iffy on the faciltity to do so + it not being documented.
> 
> I don't think that the "static buffer" part here is important to anyone,
> but the write() size is clearly important.
> 
> So doesn't it make more sense to have a uploadPack.bufferSize=8k
> variable we can tweak, just make this "buffer" a "struct strbuf" instead
> (i.e. it'll by dynamically grown), and then just flush it whenever we
> hit the configured buffer size?

> When serving a fetch, git upload-pack copies data from a git
> pack-objects stdout pipe to its stdout. This commit increases the size
> of the buffer used for that copying from 8192 to 65515, the maximum
> sideband-64k packet size.
>
> Previously, this buffer was allocated on the stack. Because the new
> buffer size is nearly 64KB, we switch this to a heap allocation.
>
> On GitLab.com we use GitLab's pack-objects cache which does writes of
> 65515 bytes. Because of the default 8KB buffer size, propagating these
> cache writes requires 8 pipe reads and 8 pipe writes from
> git-upload-pack, and 8 pipe reads from Gitaly (our Git RPC service).
> If we increase the size of the buffer to the maximum Git packet size,
> we need only 1 pipe read and 1 pipe write in git-upload-pack, and 1
> pipe read in Gitaly to transfer the same amount of data. In benchmarks
> with a pure fetch and 100% cache hit rate workload we are seeing CPU
> utilization reductions of over 30%.
>
> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>
> ---
>  upload-pack.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index c78d55bc67..3b90fb69e6 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -194,7 +194,13 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
>  }
>  
>  struct output_state {
> -	char buffer[8193];
> +	/*
> +	 * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with
> +	 * sideband-64k the band designator takes up 1 byte of space. Because
> +	 * relay_pack_data keeps the last byte to itself, we make the buffer 1
> +	 * byte bigger than the intended maximum write size.
> +	 */
> +	char buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];

>  	int used;
>  	unsigned packfile_uris_started : 1;
>  	unsigned packfile_started : 1;
> @@ -269,7 +275,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  			     const struct string_list *uri_protocols)
>  {
>  	struct child_process pack_objects = CHILD_PROCESS_INIT;
> -	struct output_state output_state = { { 0 } };
> +	struct output_state *output_state = xcalloc(1, sizeof(struct output_state));

>  	char progress[128];
>  	char abort_msg[] = "aborting due to possible repository "
>  		"corruption on the remote side.";
> @@ -404,7 +410,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  		}
>  		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
>  			int result = relay_pack_data(pack_objects.out,
> -						     &output_state,
> +						     output_state,
>  						     pack_data->use_sideband,
>  						     !!uri_protocols);
>  
> @@ -438,11 +444,12 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  	}
>  
>  	/* flush the data */
> -	if (output_state.used > 0) {
> -		send_client_data(1, output_state.buffer, output_state.used,
> +	if (output_state->used > 0) {
> +		send_client_data(1, output_state->buffer, output_state->used,
>  				 pack_data->use_sideband);
>  		fprintf(stderr, "flushed.\n");
>  	}
> +	free(output_state);
>  	if (pack_data->use_sideband)
>  		packet_flush(1);
>  	return;

> When serving a fetch, git upload-pack copies data from a git
> pack-objects stdout pipe to its stdout. This commit increases the size
> of the buffer used for that copying from 8192 to 65515, the maximum
> sideband-64k packet size.
> 
> Previously, this buffer was allocated on the stack. Because the new
> buffer size is nearly 64KB, we switch this to a heap allocation.
> 
> On GitLab.com we use GitLab's pack-objects cache which does writes of
> 65515 bytes. Because of the default 8KB buffer size, propagating these
> cache writes requires 8 pipe reads and 8 pipe writes from
> git-upload-pack, and 8 pipe reads from Gitaly (our Git RPC service).
> If we increase the size of the buffer to the maximum Git packet size,
> we need only 1 pipe read and 1 pipe write in git-upload-pack, and 1
> pipe read in Gitaly to transfer the same amount of data. In benchmarks
> with a pure fetch and 100% cache hit rate workload we are seeing CPU
> utilization reductions of over 30%.

>  struct output_state {
> -	char buffer[8193];
> +	/*
> +	 * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with
> +	 * sideband-64k the band designator takes up 1 byte of space. Because
> +	 * relay_pack_data keeps the last byte to itself, we make the buffer 1
> +	 * byte bigger than the intended maximum write size.
> +	 */
> +	char buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];

> @@ -269,7 +275,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  			     const struct string_list *uri_protocols)
>  {
>  	struct child_process pack_objects = CHILD_PROCESS_INIT;
> -	struct output_state output_state = { { 0 } };
> +	struct output_state *output_state = xcalloc(1, sizeof(struct output_state));

> @@ -438,11 +444,12 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  	}
>  
>  	/* flush the data */
> -	if (output_state.used > 0) {
> -		send_client_data(1, output_state.buffer, output_state.used,
> +	if (output_state->used > 0) {
> +		send_client_data(1, output_state->buffer, output_state->used,
>  				 pack_data->use_sideband);
>  		fprintf(stderr, "flushed.\n");
>  	}
> +	free(output_state);
>  	if (pack_data->use_sideband)
>  		packet_flush(1);
>  	return;

> Jacob Vosmaer <jacob@gitlab.com> writes:
> 
> > When serving a fetch, git upload-pack copies data from a git
> > pack-objects stdout pipe to its stdout. This commit increases the size
> > of the buffer used for that copying from 8192 to 65515, the maximum
> > sideband-64k packet size.
> 
> Thanks.  I agree with others that extra configurability is not needed in

> To: rsbecker@nexbridge.com
> Cc: Junio C Hamano <gitster@pobox.com>; git@vger.kernel.org;
> avarab@gmail.com; peff@peff.net
> Subject: Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size
> 
> On Wed, Dec 15, 2021 at 8:59 PM <rsbecker@nexbridge.com> wrote:
> 
> > This is likely to break NonStop as our sideband packet size is less
> > than 64K.
> 
> Could you elaborate on what you expect to break?

> On Wed, Dec 15, 2021 at 9:38 PM <rsbecker@nexbridge.com> wrote:
> >
> > The maximum I/O size on the platform is 56Kb. Anything larger will fail.
> That's why we use xread and xwrite for non-buffered I/O.
> 
> That sounds like it's orthogonal to what this patch is changing.
> Regardless of this patch, the actual writes are performed by write_or_die,
> which calls write_in_full. In turn, write_in_full calls xwrite however often is
> necessary. We are not making any assumptions anywhere about how large
> individual write syscalls turn out. Git packets use userspace buffers so we can
> read and write them no matter what the underlying IO sizes are.

> The current size of the buffer is 8192+1. The '+1' is a technical
> detail orthogonal to this change. On GitLab.com we use GitLab's
> pack-objects cache which does writes of 65515 bytes. Because of the
> default 8KB buffer size, propagating these cache writes requires 8
> pipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads
> from Gitaly (our Git RPC service). If we increase the size of the
> buffer to the maximum Git packet size, we need only 1 pipe read and 1
> pipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer
> the same amount of data. In benchmarks with a pure fetch and 100%
> cache hit rate workload we are seeing CPU utilization reductions of
> over 30%.

>
> It gets about 2.3GB/s with the tip of 'master' and 3.2GB/s with the
> equivalent of your patch (using LARGE_PACKET_DATA_MAX). So definitely an
> improvement.
>
> Without the cached case (so actually running pack-objects, albeit a
> pretty quick one because of bitmaps and pack-reuse), the timings are
> about the same (171MB/s versus 174MB/s, but really it's just pegging a
> CPU running pack-objects). So it would be fine to just do this
> unconditionally, I think.

> Looking at strace, the other thing I notice is that we write() the
> packet header separately in send_sideband(), which doubles the number of
> syscalls. I hackily re-wrote this to use writev() instead (patch below),
> but it doesn't seem to actually help much (maybe a curiosity to explore
> further, but definitely not something to hold up your patch).

> When transfering packfile data, upload-pack.c uses an 8KB buffer.
> This is a reasonable size but when you transfer a lot of packfile
> data, like we do on GitLab.com, we find it is beneficial to use a
> larger buffer size.
> 
> Below you will find a commit where we make the size of this 8KB
> buffer configurable at compile time. It appears pack-objects always
> does 8KB writes so I don't think we should change the default. But
> for GitLab, where we have a cache for the output of pack-objects,
> it is beneficial to use a larger IO size because the cache does
> 64KB writes.

> I have also considered converting the packfile copying code to use
> stdio when writing to stdout, but that would be a bigger change
> because we have to be careful not to interleave stdio and stdlib
> writes. And we would have to make the stdout output buffer size
> configurable, because the default stdio buffer size is 4KB which
> is no better than the status quo. A final argument against the stdio
> approach is that it only reduces the number of writes from upload-pack,
> while a larger buffer size reduces both the number of reads and
> writes.