Re: [PATCH v3 03/10] upload-pack: prefer flushing data over sending keepalive

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> When using the sideband in git-upload-pack(1) we know to send out
> keepalive packets in case generating the pack takes too long. These
> keepalives take the form of a simple empty pktline.
>
> In the preceding commit we have adapted git-upload-pack(1) to buffer
> data more aggressively before sending it to the client. This creates an
> obvious optimization opportunity: when we hit the keepalive timeout
> while we still hold on to some buffered data, then it makes more sense
> to flush out the data instead of sending the empty keepalive packet.
>
> This is overall not going to be a significant win. Most keepalives will
> come before the pack data starts, and once pack-objects starts producing
> data, it tends to do so pretty consistently. And of course we can't send
> data before we see the PACK header, because the whole point is to buffer
> the early bit waiting for packfile URIs. But the optimization is easy
> enough to realize.
>
> Do so and flush out data instead of sending an empty pktline. While at
> it, drop the useless

Useless what?

> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  upload-pack.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index f6f380a601..7a165d226d 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -466,18 +466,27 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  		}
>  
>  		/*
> -		 * We hit the keepalive timeout without saying anything; send
> -		 * an empty message on the data sideband just to let the other
> -		 * side know we're still working on it, but don't have any data
> -		 * yet.
> +		 * We hit the keepalive timeout without saying anything. If we
> +		 * have pending data we flush it out to the caller now.
> +		 * Otherwise, we send an empty message on the data sideband
> +		 * just to let the other side know we're still working on it,
> +		 * but don't have any data yet.
>  		 *
>  		 * If we don't have a sideband channel, there's no room in the
>  		 * protocol to say anything, so those clients are just out of
>  		 * luck.
>  		 */
>  		if (!ret && pack_data->use_sideband) {
> -			static const char buf[] = "0005\1";
> -			write_or_die(1, buf, 5);
> +			if (output_state->packfile_started && output_state->used > 1) {
> +				send_client_data(1, output_state->buffer, output_state->used - 1,
> +						 pack_data->use_sideband);
> +				output_state->buffer[0] = output_state->buffer[output_state->used - 1];
> +				output_state->used = 1;
> +			} else {
> +				static const char buf[] = "0005\1";
> +				write_or_die(1, buf, 5);
> +			}
> +

OK.  The new part of the comment, "If we have pending data, flush
it", is what the new code is doing (i.e., flush all the bytes before
the final byte, and make the buffer hold only that final byte we
kept for ourselves).  Otherwise we give a keepalive packet (i.e., 0
byte thrown at the sideband #1) as before.

OK.

>  			last_sent_ms = now_ms;
>  		}
>  	}