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

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

[Jeff King]

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.

  - 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.

Signed-off-by: Jeff King <peff@peff.net>
---
I happened to be running p5310 today and was annoyed by this.

The other really expensive part is the actual "git repack" to generate
the bitmaps. We can't avoid running that (since we need the bitmaps it
generates), but it's mildly annoying that it runs all three times.
However, I think it's worth keeping as a timed test, as it would help us
find a performance regression in the bitmap generation code. It's too
bad we have to do the full repack for each trial, but we don't have a
way to just write bitmaps for an existing pack (it's conceptually
simple, but the code is pretty tied into pack-objects).

 t/perf/p5310-pack-bitmaps.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index 7743f4f4c9..80c53edca7 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -31,10 +31,6 @@ test_perf 'simulated fetch' '
 	} | git pack-objects --revs --stdout >/dev/null
 '
 
-test_perf 'pack to file' '
-	git pack-objects --all pack1 </dev/null >/dev/null
-'
-
 test_perf 'pack to file (bitmap)' '
 	git pack-objects --use-bitmap-index --all pack1b </dev/null >/dev/null
 '
-- 
2.26.0.576.gb87790c3c1

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

[Derrick Stolee]

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

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

[Jeff King]

On Thu, Mar 26, 2020 at 06:48:37AM -0400, Derrick Stolee wrote:

> >   - 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.

I think that's the right direction, but it would require tests knowing
how to make use of those extra parameters. What I'd _really_ like is a
way to parameterize a whole bunch of things:

 - test repo
 - git version
 - particular config settings

do runs with whatever combinations of parameters you choose, and then
create tables showing differences across various settings of a
parameter. Some parameters would be automatically set up, but test
scripts would have to advertise other parameters they know about.

I don't think it's conceptually hard, but the existing perf code is
pretty hacked-together and relies on the filesystem for storage. So the
notion that we'd only ever compare different versions runs deep; that's
how the on-disk storage is organized.

> > 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.

Yeah, I should have noted that we're not actually testing the
performance of a stock repack anywhere else. It wouldn't belong in
p5310, but in theory it might be of interest in another script. But I
agree for the reasons you gave that it's not all that interesting a
timing.

-Peff