Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] p5310: stop timing non-bitmap pack-to-disk
@ 2020-03-26  7:54 Jeff King
  2020-03-26 10:48 ` Derrick Stolee
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2020-03-26  7:54 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] p5310: stop timing non-bitmap pack-to-disk
  2020-03-26  7:54 [PATCH] p5310: stop timing non-bitmap pack-to-disk Jeff King
@ 2020-03-26 10:48 ` Derrick Stolee
  2020-03-26 11:12   ` Jeff King
  0 siblings, 1 reply; 3+ messages in thread
From: Derrick Stolee @ 2020-03-26 10:48 UTC (permalink / raw)
  To: Jeff King, git

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] p5310: stop timing non-bitmap pack-to-disk
  2020-03-26 10:48 ` Derrick Stolee
@ 2020-03-26 11:12   ` Jeff King
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2020-03-26 11:12 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: git

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26  7:54 [PATCH] p5310: stop timing non-bitmap pack-to-disk Jeff King
2020-03-26 10:48 ` Derrick Stolee
2020-03-26 11:12   ` Jeff King

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git