Re: [PATCH v2 4/4] Bump rename limit defaults (yet again)
From: Elijah Newren <hidden>
Date: 2021-07-14 17:33:09
On Wed, Jul 14, 2021 at 9:43 AM Jeff King [off-list ref] wrote:
On Wed, Jul 14, 2021 at 01:12:33AM +0000, Elijah Newren via GitGitGadget wrote:quoted
The combined effect of the above is that the file size used in past calculations was likely about 5x too large. Combine that with a CPU performance improvement of ~30%, and we can increase the limits by a factor of sqrt(5/(1-.3)) = 2.67, while keeping the original stated time limits.It's slightly sad that we only got a 30% CPU improvement in the past 10 years. Are you just counting clock speed as a short-hand here? I think that doesn't tell the whole story. But all of this is a side-note anyway. What I care about is your actual timings. :)
I'm using shorthand when discussing file sizes above (though I used actual measurements when picking new values below). But the 30% came from measuring the timings with the exact same sample file as you and using a lightly modified version of your original script (tweaked to also change file basenames) on an AWS c5xlarge instance. My timings showed they were only about 30% faster than what you got when you last bumped the limits. All my laptops and even my work machine are pretty old, so I picked an AWS c5xlarge instance hoping it'd represent a relatively new machine (since your benchmarks when you bumped were using a new machine at the time). Maybe the stock c5xlarge instance I picked wasn't a good choice (wasn't new enough? had a low relative clock speed? affected by Spectre patches? had slow disks? something else?). Or maybe single processor speed really did just hit up against Moore's law and nearly all gains have shifted to having more cores available which don't benefit us here since our algorithm is serial.
(It also seems like this rename detection is ripe for parallelization, but obviously that's a totally separate topic).
True.
quoted
Using the original time limit of 2s for diff.renameLimit, and bumping merge.renameLimit from 10s to 60s, I found the following timings using the simple script at the end of this commit message (on an AWS c5.xlarge which reports as "Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz"): N Timing 1300 1.995s 7100 59.973s So let's round down to nice even numbers and bump the limits from 400->1000, and from 1000->7000.Sounds good. Thanks for thoroughly measuring and updating.