Re: [PATCH v2 4/4] Bump rename limit defaults (yet again)

From: Elijah Newren <newren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 4/4] Bump rename limit defaults (yet again)
Date: Wed, 14 Jul 2021 10:32:56 -0700	[thread overview]
Message-ID: <CABPp-BG7T2QP+6uP57NuE1Htr-vBzozL-aDxU4FsyQO+ELQ9Uw@mail.gmail.com> (raw)
In-Reply-To: <YO8UPtFr4wRhVTXE@coredump.intra.peff.net>

On Wed, Jul 14, 2021 at 9:43 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jul 14, 2021 at 01:12:33AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > 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.

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