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

From: Jeff King <peff@peff.net>
Date: Dec 15, 2021, 16:30 UTC
Message-ID: <YboYM7YKcRx1RuJY@coredump.intra.peff.net>
In-Reply-To: <20211214194626.33814-2-jacob@gitlab.com>

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

Show 17 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 15 quoted lines

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