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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2016, 19:10 UTC
Message-ID: <xmqq37n6kbib.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160718185527.GB29326@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 25 quoted lines

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

Sorry, while I agree with the general sentiment that the windowing heuristics can be improved, from your description, I would have expected an updated curve goes like "aggressive exponential -> conservative exponential -> slow linear", but the new comparison reads the other way around, i.e. "aggressive exponential -> slow linear -> conservative exponential".

I'd understand if it were more like "aggressive exponential -> conservative exponential" without linear phase when stateless_rpc is in use, though. I just do not quite understand the justification behind the order of three phases introduced by this change.

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;