Re: [PATCH 3/4] t/perf: avoid testing bitmaps without lookup table

From: Jeff King <peff@peff.net>
Date: Apr 18, 2025, 10:02 UTC
Message-ID: <20250418100246.GC10441@coredump.intra.peff.net>
In-Reply-To: <xmqqzfgeaygh.fsf@gitster.g>

On Thu, Apr 17, 2025 at 09:24:46PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Junio C Hamano <gitster@pobox.com> 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