Re: [PATCH] fetch-pack: grow stateless RPC windows exponentially

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 18, 2016, 18:55 UTC
Message-ID: <20160718185527.GB29326@google.com>
In-Reply-To: <1468867019-13086-1-git-send-email-jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

Show 13 quoted lines

> When updating large repositories, the LARGE_FLUSH limit (that is, the
> limit at which the window growth strategy switches from exponential to
> linear) is reached quite quickly. Use a conservative exponential growth
> strategy when that limit is reached instead.
>
> This optimization is only applied during stateless RPCs to avoid the
> issue raised and fixed in commit
> 44d8dc54e73e8010c4bdf57a422fc8d5ce709029.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 2 ++
>  1 file changed, 2 insertions(+)

Yay, thanks for this.

When this condition triggers (count >= 10240), we have already experienced 10 rounds of negotiation. Negotiation ought to have finished by then. So this is a pretty conservative change to try to salvage an already bad situation.

The condition ensures that the exponential growth will go faster than the previous heuristic of linear growth.

Memory usage grows with the number of 'have's to be sent. Linear growth didn't bound memory usage. This exponential growth makes memory usage increase faster, but not aggressively so and the unbounded memory usage is already something we'd want to address separately to handle hostile servers.

All in all, this looks likely to allow negotiation to finish in fewer rounds, speeding up fetch, without much downside, so for what it's worth,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I'd expect us to need more aggressive improvements to negotiation in the end (e.g. finding a way to order SHA-1s sent as 'have's to finish in fewer rounds). But this is a good start. Thanks for writing it.

Show 13 quoted lines

> diff --git a/fetch-pack.c b/fetch-pack.c
> index b501d5c..3fcbda2 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -251,6 +251,8 @@ static int next_flush(struct fetch_pack_args *args, int count)
>  
>  	if (count < flush_limit)
>  		count <<= 1;
> +	else if (args->stateless_rpc && count >= flush_limit * 10)
> +		count = count * 11 / 10;
>  	else
>  		count += flush_limit;
>  	return count;