| Message ID | 20200326075436.GA2199958@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | p5310: stop timing non-bitmap pack-to-disk | expand |
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
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
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 '
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(-)