Re: [PATCH 2/4] p5312: removed duplicate performance test script

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 18, 2025, 21:57 UTC
Message-ID: <aALK0JlOZDVPUHUR@nand.local>
In-Reply-To: <xmqqh62me8zo.fsf@gitster.g>

On Thu, Apr 17, 2025 at 03:08:59PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
> > Subject: Re: [PATCH 2/4] p5312: removed duplicate performance test script
>
> "removed" -> "remove"???
>
> > When the reachability bitmap format learned to read and write a lookup
> > table containing the set of commits which received reachability bitmaps,
> > commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup
> > table, 2022-08-14) added that mirrored p5310 but with reverse indexes
> > enabled.
>
> "added that" -> "added a <something> that"???

I am embarrassed. These are both awful. Here's the relevant portion of the range-diff:

2:  51c4604e16 ! 2:  a80a7b5e60 p5312: removed duplicate performance test script
    @@ Metadata
     Author: Taylor Blau <me@ttaylorr.com>

      ## Commit message ##
    -    p5312: removed duplicate performance test script
    +    p5312: remove duplicate performance test script

         When the reachability bitmap format learned to read and write a lookup
         table containing the set of commits which received reachability bitmaps,
         commit 761416ef91 (bitmap-lookup-table: add performance tests for lookup
    -    table, 2022-08-14) added that mirrored p5310 but with reverse indexes
    -    enabled.
    +    table, 2022-08-14) added a new performance test script mirroring p5310
    +    but with reverse indexes enabled.

         Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by
         default, 2023-04-12), we enabled reverse indexes by default, which made

Show 8 quoted lines

> > Later on in a8dd7e05b1 (config: enable `pack.writeReverseIndex` by
> > default, 2023-04-12), we enabled reverse indexes by default, which made
> > these two tests indistinguishable from one another. Commit a8dd7e05b1
> > should have removed p5312 as a duplicate, but didn't do so.
>
> Or to retain the same coverage, it should have explicitly disabled
> reverse index in one of the tests, while allowing the other to use
> the reverse index enabled by default, perhaps?

I don't think we necessarily would benefit from having two variants of this performance test. It is a little annoying to maintain, but that isn't the main reason that I proposed removing it here.

I think that the pair of performance tests were useful in proving out the lookup table extension as useful to bitmaps' performance characteristics by comparison to the non-lookup table version. In that sense, I think the pair of performance tests were useful as a contrast to one another. Since we have evidence of their usefulness, the contrast is less important IMHO.

I think we still want to keep the "lookup table enabled" version to prevent regressions, though.

Thanks, Taylor