Re: [PATCH] gc: default aggressive depth to 50
From: Junio C Hamano <hidden>
Date: 2016-08-11 18:52:30
Jeff King [off-list ref] writes:
quoted hunk
On Thu, Aug 11, 2016 at 12:13:09PM -0400, Jeff King wrote:quoted
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.