Re: [PATCH v2] Bump core.deltaBaseCacheLimit to 128MiB

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2014, 22:11 UTC
Message-ID: <xmqqlhw5260l.fsf@gitster.dls.corp.google.com>
In-Reply-To: <87ob11g9st.fsf@fencepost.gnu.org>

David Kastrup <dak@gnu.org> writes:

Show 36 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> The default of 16MiB causes serious thrashing for large delta chains
>>> combined with large files.
>>>
>>> Signed-off-by: David Kastrup <dak@gnu.org>
>>> ---
>>
>> Is that a good argument?  Wouldn't the default of 128MiB burden
>> smaller machines with bloated processes?
>
> The default file size before Git forgets about delta compression is
> 512MiB.  Unpacking 500MiB files with 16MiB of delta storage is going to
> be uglier.
>
> ...
>
> Documentation/config.txt states:
>
>     core.deltaBaseCacheLimit::
>             Maximum number of bytes to reserve for caching base objects
>             that may be referenced by multiple deltified objects.  By storing the
>             entire decompressed base objects in a cache Git is able
>             to avoid unpacking and decompressing frequently used base
>             objects multiple times.
>     +
>     Default is 16 MiB on all platforms.  This should be reasonable
>     for all users/operating systems, except on the largest projects.
>     You probably do not need to adjust this value.
>
> I've seen this seriously screwing performance in several projects of
> mine that don't really count as "largest projects".
>
> So the description in combination with the current setting is clearly wrong.

That is a good material for proposed log message, and I think you are onto something here.

I know that the 512MiB default for the bitFileThreashold (aka "forget about delta compression") came out of thin air. It was just "1GB is always too huge for anybody, so let's cut it in half and declare that value the initial version of a sane threashold", nothing more.

So it might be that the problem is 512MiB is still too big, relative to the 16MiB of delta base cache, and the former may be what needs to be tweaked. If a blob close to but below 512MiB is a problem for 16MiB delta base cache, it would still be too big to cause the same problem for 128MiB delta base cache---it would evict all the other objects and then end up not being able to fit in the limit itself, busting the limit immediately, no?

I would understand if the change were to update the definition of deltaBaseCacheLimit and link it to the value of bigFileThreashold, for example. With the presented discussion, I am still not sure if we can say that bumping deltaBaseCacheLimit is the right solution to the "description with the current setting is clearly wrong" (which is a real issue).