Re: [PATCH] gc: default aggressive depth to 50

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2016, 18:52 UTC
Message-ID: <xmqqmvkj16lc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160811172050.44abuvwcn6gmcgk7@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 37 quoted lines

> On Thu, Aug 11, 2016 at 12:13:09PM -0400, Jeff King wrote:
>
>> Here are the numbers for linux.git:
>> 
>>    depth |  size |  %    | rev-list |  %     | log -Sfoo |   %
>>   -------+-------+-------+----------+--------+-----------+-------
>>     250  | 967MB |  n/a  | 48.159s  |   n/a  | 378.088   |   n/a
>>     100  | 971MB | +0.4% | 41.471s  | -13.9% | 342.060   |  -9.5%
>>      50  | 979MB | +1.2% | 37.778s  | -21.6% | 311.040s  | -17.7%
>>      10  | 1.1GB | +6.6% | 32.518s  | -32.5% | 279.890s  | -25.9%
>> [...]
>> 
>> You can see that that the CPU savings for regular operations improves as we
>> decrease the depth. The savings are less for "rev-list" on a smaller repository
>> than they are for blob-accessing operations, or even rev-list on a larger
>> repository. This may mean that a larger delta cache would help (though setting
>> core.deltaBaseCacheLimit by itself doesn't).
>
> The problem with deltaBaseCacheLimit is that it only changes the memory
> parameter, but there are a fixed number of slots in the data structure.
> Bumping it like this:
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 02940f1..ca79703 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2073,7 +2073,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
>  	return buffer;
>  }
>  
> -#define MAX_DELTA_CACHE (256)
> +#define MAX_DELTA_CACHE (1024)
>  
>  static size_t delta_base_cached;
>
> along with the cache size does help (this was discussed a year or two
> ago, but nobody ever followed up with numbers or patches).

Yeah, and I also think Linus's "--depth=250 is just a sample; it will not perform well" already cited the number of delta-cache entries being the limiting factor.

> I don't think bumping MAX_DELTA_CACHE naively is a good idea, though. I
> seem to recall that it has scaling problems as it grows, so we may want
> a better data structure (but I haven't looked at it recently enough to
> say anything intelligent).

Me neither. In any case, I do think reducing the aggressive depth down to 50 is a very sensible move. I also suspect that window size may want to be a bit increased (or even made dynamic; the first time we need the window size determined is after to_pack.objects[] array is fully populated, so we could use the number of commits as one of the hint, for example), but that can be treated as a separate topic.