On Thu, Apr 17, 2025 at 03:08:59PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
## 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
> > 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
