[PATCH 0/1] Make upload-pack pack write size configurable

[PATCH 0/1] Make upload-pack pack write size configurable

[Jacob Vosmaer]

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.

Having said all that, if the Git maintainers prefer the stdio
approach over this compile time constant, I am happy to submit a
patch series for that instead.

Thanks,

Jacob Vosmaer

Jacob Vosmaer (1):
  upload-pack.c: make output buffer size configurable

 upload-pack.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH 1/1] upload-pack.c: make output buffer size configurable

[Jacob Vosmaer]

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;
-- 
2.33.0

Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 13 2021, Jacob Vosmaer wrote:

> 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;

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?

Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable

[Jeff King]

On Tue, Dec 14, 2021 at 01:08:55PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > +#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?

I don't think we want to grow dynamically, because we don't want to hold
arbitrary amounts of data in memory. We're just passing it through. But
if there were a run-time config option, we could easily heap-allocate
the buffer up front.

That may be overkill, though I do agree this is kind of weirdly
undocumented. There are two other subtleties I notice:

  - This output_state struct does go on the stack, so something big like
    64k is questionable there (though on Linux, without recursion, it's
    usually OK).

  - we're relaying the data into pkt-lines. Do we run into problems when
    the buffer is larger than a packet? I think in send_sideband() we'll
    break it up as appropriate. But before we hit the PACK header, we
    send out packfile-uris directly with packet_write_fmt(). Those
    aren't likely to be long, but if they are, we'd die() in
    format_packet(). So something around LARGE_PACKET_DATA_MAX is
    probably the highest you'd want to set it anyway (and the most
    performant, since otherwise you have to write out extra partial
    packets).

So I kind of wonder if there is any real _harm_ in just always using
bigger packets, even if it does not always help. Given the subtle rules
about packet-max above, then we could just use that optimal value and
not worry about configurability.

-Peff

Re: [PATCH 0/1] Make upload-pack pack write size configurable

[Jeff King]

On Mon, Dec 13, 2021 at 02:23:44PM +0100, Jacob Vosmaer wrote:

> 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 suspect the big reason that it matters for the cache is that for
pack-objects, we're typically CPU bound computing the sha1 of the
outgoing pack (for its trailer).

I do suspect we could just always move to something closer to
LARGE_PACKET_DATA_MAX (probably minus 1 for the sideband marker). Even
if pack-objects only writes in 8k chunks, we may be able to pull several
in one read(), especially if the network is a bottleneck (because we'd
block writing to the client, and pack-objects would fill up the pipe
buffer).

I.e., it doesn't seem like there's any real downside to doing so.

> 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.

Yeah, I don't think that would help all that much. We really want to
size this based on pkt-line limits. That reduces syscalls, but also
shrinks the overall size a little (since larger packets minimizes the
framing overhead of the pkt-line headers).

-Peff

Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable

[Jeff King]

On Mon, Dec 13, 2021 at 02:23:45PM +0100, Jacob Vosmaer wrote:

> 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%.

I was curious to reproduce this locally, so I came up with:

  (
    printf "003fwant %s side-band-64k" $(git rev-parse HEAD)
    printf 0000
    echo '0009done'
  ) |
  git -c uploadpack.packobjectsHook='cat objects/pack/pack-*.pack; :' upload-pack . |
  pv >/dev/null

which hackily simulates the server side of your "cached" case. :) I ran
it on a fully-packed clone of linux.git.

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).

-Peff

---
diff --git a/sideband.c b/sideband.c
index 85bddfdcd4..d0945507a3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -5,6 +5,11 @@
 #include "help.h"
 #include "pkt-line.h"
 
+/* hack; should go in git-compat-util, and should provide compat
+ * wrapper around regular write()
+ */
+#include <sys/uio.h>
+
 struct keyword_entry {
 	/*
 	 * We use keyword as config key so it should be a single alphanumeric word.
@@ -257,6 +262,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 
 	while (sz) {
 		unsigned n;
+		struct iovec iov[2];
 		char hdr[5];
 
 		n = sz;
@@ -265,12 +271,17 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 		if (0 <= band) {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
 			hdr[4] = band;
-			write_or_die(fd, hdr, 5);
+			iov[0].iov_base = hdr;
+			iov[0].iov_len = 5;
 		} else {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
-			write_or_die(fd, hdr, 4);
+			iov[0].iov_base = hdr;
+			iov[0].iov_len = 4;
 		}
-		write_or_die(fd, p, n);
+		iov[1].iov_base = (void *)p;
+		iov[1].iov_len = n;
+		/* should check for errors, but also short writes and EINTR, etc */
+		writev(fd, iov, 2);
 		p += n;
 		sz -= n;
 	}
diff --git a/upload-pack.c b/upload-pack.c
index c78d55bc67..111de8c60c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -194,7 +194,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 }
 
 struct output_state {
-	char buffer[8193];
+	char buffer[LARGE_PACKET_DATA_MAX];
 	int used;
 	unsigned packfile_uris_started : 1;
 	unsigned packfile_started : 1;

[PATCH v2 0/1] upload-pack.c: increase output buffer size

[Jacob Vosmaer]

Thank you Ævar and Peff for your comments on the previous iteration
of this patch. I have made the buffer size fixed at the pktline
maximum, and I have changed the storage of output_state from stack
to heap.

As far as I can tell, all return paths from create_pack_file call
"die", except the happy path. So I only free the heap-allocated
output_state on the happy path. Is that OK, or should I use a "goto
out" function shape?

Jacob Vosmaer (1):
  upload-pack.c: increase output buffer size

 upload-pack.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.33.0

[PATCH v2 1/1] upload-pack.c: increase output buffer size

[Jacob Vosmaer]

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;
-- 
2.33.0

Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable

[Jacob Vosmaer]

On Tue, Dec 14, 2021 at 4:37 PM Jeff King <peff@peff.net> wrote:
>
> 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.

Thank you for validating this!

> 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).

Those double writes bug me too. Getting rid of them was one of the
things I liked about using stdio here. Curious now if writev will make
an impact in my benchmarks. As your experiments indicate, the double
writes may not be a problem in real life. Either way, I also feel it
is not something to rope into this patch set.

Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 14 2021, Jacob Vosmaer wrote:

> 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];

Since X-1+1 = X shouldn't we just say X ? :)

Maybe this fixup is better, maybe not:
	
	diff --git a/upload-pack.c b/upload-pack.c
	index 3b90fb69e6d..10849110229 100644
	--- a/upload-pack.c
	+++ b/upload-pack.c
	@@ -195,12 +195,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
	 
	 struct output_state {
	 	/*
	-	 * 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.
	+	 * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1,
	+	 * because with * sideband-64k the band designator takes up 1
	+	 * byte of space (see relay_pack_data() below). So
	+	 * LARGE_PACKET_DATA_MAX ends up being the right size.
	 	 */
	-	char buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];
	+	char buffer[LARGE_PACKET_DATA_MAX];
	 	int used;
	 	unsigned packfile_uris_started : 1;
	 	unsigned packfile_started : 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));

I don't know when/if we need to worry about 8k v.s. ~65k on the stack,
especially as recv_sideband() has:

	char buf[LARGE_PACKET_MAX + 1];

But maybe our stack is already quite big here, I don't know...

>  	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;

But this looks fine either way. And yes, in reply to the question in the
cover letter it's fine to ignore the memory leaks we have when we call
die().

Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[Jeff King]

On Tue, Dec 14, 2021 at 08:46:26PM +0100, Jacob Vosmaer wrote:

> 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%.

Thanks, this patch looks good to me.

>  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];

I don't have an opinion on the "-1 / +1" thing mentioned elsewhere. What
I care much more about is that the comment explains what is going on,
and what you wrote here is easy to understand.

> @@ -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));

I think this hunk is worth doing. As Ævar noted, we do sometimes have
pretty big stack variables elsewhere, so we might have been able to get
away with it here.  But running out of stack can be unpredictable and
annoying. Given how easy and low-cost it is to put it on the heap here,
I'm glad to just do it preemptively.

> @@ -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;

There's a "fail:" label just below here. But since it does not return,
and just calls die(), then I agree you don't need to worry about freeing
in that case (and there are no other returns from the function). So this
single free is sufficient.

-Peff

Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[Junio C Hamano]

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 this case, and allocating this on the heap (as long as we
correctly deallocate it when we are done) is the right thing.

Will queue.  Thanks.

RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[rsbecker]

On December 15, 2021 2:51 PM, Junio C Hamano wrote:
> 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
this
> case, and allocating this on the heap (as long as we correctly deallocate
it
> when we are done) is the right thing.

This is likely to break NonStop as our sideband packet size is less than
64K.
-Randall

Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[Jacob Vosmaer]

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?

RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[rsbecker]

On December 15, 2021 3:24 PM, Jacob Vosmaer wrote:
> 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?

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.
-Randall

Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[Jacob Vosmaer]

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.

RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size

[rsbecker]

On December 15, 2021 3:45 PM, Jacob Vosmaer wrote:
> 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.

In that case, it works for me. xread/xwrite know how to deal with the situation. Thanks for the clarification.
-Randall