Re: [PATCH] p5310: stop timing non-bitmap pack-to-disk

[Derrick Stolee <stolee@gmail.com>]

On 3/26/2020 3:54 AM, Jeff King wrote:
> Commit 645c432d61 (pack-objects: use reachability bitmap index when
> generating non-stdout pack, 2016-09-10) added two timing tests for
> packing to an on-disk file, both with and without bitmaps. However, the
> non-bitmap one isn't interesting to have as part of p5310's regression
> suite. It _could_ be used as a baseline to show off the improvement in
> the bitmap case, but:
> 
>   - the point of the t/perf suite is to find performance regressions,
>     and it won't help with that. We don't compare the numbers between
>     two tests (which the perf suite has no idea are even related), and
>     any change in its numbers would have nothing to do with bitmaps.

This does make me think if there is a way to adjust "./run" to test two
different config settings or command-line options instead of just two
different build versions. Perhaps something like

 ./run "HEAD -c core.commitGraph=true" "HEAD -c core.commitGraph=false" -- p4200-line-log.sh

But that's just musing on my part.

>   - it did show off the improvement in the commit message of 645c432d61,
>     but it wasn't even necessary there. The bitmap case already shows an
>     improvement (because before the patch, it behaved the same as the
>     non-bitmap case), and the perf suite is even able to show the
>     difference between the before and after measurements.
> 
> On top of that, it's one of the most expensive tests in the suite,
> clocking in around 60s for linux.git on my machine (as compared to 16s
> for the bitmapped version). And by default when using "./run", we'd run
> it three times!
> 
> So let's just drop it. It's not useful and is adding minutes to perf
> runs.

I agree with your reasoning. This is not a critical path for clients,
and all servers should be using bitmaps.

Thanks,
-Stolee