On Thu, Apr 17, 2025 at 09:24:46PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > I somehow have a feeling that removal of these "performance" tests
> > is less worrysome than removing correctness tests, but as long as we
> > claim to support both configurations (i.e. with and without lookup
> > tables), it feels a bit premature to remove tests for one of them.
>
> In case the implication was missed, I was hinting that in the longer
> term, once one variant proves to be better than the other variant(s)
> in any and all aspects, it would be a great move to remove the other
> one(s). It is exactly what is happening on the recursive-ort front.
>
> Once we become so confident about correctness and performance with
> the configuration with lookup tables that we are willing to lose an
> escape hatch to operate without them, we can obviously remove these
> tests for configuration without lookup tables. If we are not there
> yet, and still rely on the "escape hatch" value of the configuration
> that does not use the lookup tables, we want to make sure that the
> escape hatch still functions, right ;-)?
I think the perf tests differ from the correctness tests in that a
single run is not all that interesting. You can see how long something
takes, but that's meaningless without a baseline.
The interesting results come from comparing two versions. So in this
case:
- running the simplified test script comparing an old version (where
lookup tables were not the default) with one where they are (i.e.,
one with the first patch from this series). That will show off the
perf improvement from the lookup tables (and in a better way than
the original, because we'll actually compute the time difference
between the two versions, rather than showing them as separate lines
which the perf suite does not realize are related).
- going forward, comparing two "new" versions will show us if the
operations regress in performance, using config both from Git's
defaults but also the repo.
So most of the time, you'd be testing the default case, where we do
generate the lookup tables (because they're now the default). But if
you have a particular repo or config setup you care about, you can
provide a repo with pack.writeBitmapLookupTable set as you like.
That does create a blind spot if no developers running the perf suite
ever do the "you can provide..." step. But I think that is the tip of
the iceberg in terms of repo and config blind spots in the perf suite.
There are so many possible combinations, and it's expensive to test them
all. I don't think we have any particular reason to think that the
non-lookup-table code path would significantly regress in performance.
You asked earlier how much these tests cost to run. It's basically
doubling the cost of each script, since we're running everything twice.
So p5310 using linux.git, for instance, just took ~10 minutes after this
patch. And that's with PERF_REPEAT_COUNT set to 1! The default would
have been 30 minutes (and thus prior to this patch, 60 minutes total).
That's a lot of minutes.
-Peff
