storage: Add rocksdb-vs-pebble benchmark for ExportToSst by itsbilal · Pull Request #49721 · cockroachdb/cockroach

itsbilal · 2020-05-29T22:01:33Z

As part of the investigation into #49710, this change adds a
benchmark for ExportToSst that tests both RocksDB and Pebble.

Here are some example runs without contention (old = rocksdb,
new = pebble):

name                                                   old time/op  new time/op  delta
ExportToSst/rocksdb/numKeys=64/numRevisions=1-12       43.9µs ± 3%  34.5µs ± 4%  -21.33%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=64/numRevisions=10-12       281µs ± 3%   169µs ± 6%  -39.89%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=64/numRevisions=100-12     1.82ms ±22%  1.17ms ± 1%  -35.73%  (p=0.000 n=10+9)
ExportToSst/rocksdb/numKeys=512/numRevisions=1-12       212µs ± 6%   111µs ± 3%  -47.77%  (p=0.000 n=10+9)
ExportToSst/rocksdb/numKeys=512/numRevisions=10-12     1.91ms ± 1%  1.19ms ± 8%  -37.65%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=512/numRevisions=100-12    13.7ms ± 3%  10.1ms ±12%  -26.21%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=1024/numRevisions=1-12      390µs ± 1%   215µs ±12%  -44.94%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=1024/numRevisions=10-12    4.01ms ± 6%  2.40ms ±16%  -40.13%  (p=0.000 n=10+9)
ExportToSst/rocksdb/numKeys=1024/numRevisions=100-12   27.9ms ± 2%  20.8ms ± 2%  -25.48%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=8192/numRevisions=1-12     2.97ms ± 2%  1.42ms ± 5%  -52.24%  (p=0.000 n=9+10)
ExportToSst/rocksdb/numKeys=8192/numRevisions=10-12    32.8ms ± 7%  19.1ms ± 3%  -41.59%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=8192/numRevisions=100-12    224ms ± 3%   169ms ±25%  -24.64%  (p=0.000 n=9+10)
ExportToSst/rocksdb/numKeys=65536/numRevisions=1-12    23.7ms ± 4%  13.4ms ±20%  -43.65%  (p=0.000 n=9+10)
ExportToSst/rocksdb/numKeys=65536/numRevisions=10-12    264ms ± 4%   201ms ±24%  -23.92%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=65536/numRevisions=100-12   1.88s ± 6%   1.23s ± 8%  -34.70%  (p=0.000 n=10+8)

And some with contention=true:

name                                                                   old time/op  new time/op  delta
ExportToSst/rocksdb/numKeys=65536/numRevisions=10/contention=true-12    362ms ± 7%   168ms ± 3%  -53.60%  (p=0.000 n=10+10)
ExportToSst/rocksdb/numKeys=65536/numRevisions=100/contention=true-12   2.24s ± 6%   1.24s ±10%  -44.50%  (p=0.000 n=10+10)

Release note: None.

cockroach-teamcity · 2020-05-29T22:01:40Z

This change is

petermattis

Thanks for getting to this so quickly. Too bad it didn't reveal a smoking gun. You should include RocksDB vs Pebble comparison numbers in the PR/commit messsage.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @pbardea, and @petermattis)

pkg/storage/bench_test.go, line 1009 at r1 (raw file):

		for j := 0; j < numRevisions; j++ {
			err := batch.Put(MVCCKey{key, hlc.Timestamp{int64(j), 1}}, []byte("foobar"))

In practice, most hlc.Timestamps have a 0 logical time.

pkg/storage/bench_test.go, line 1044 at r1 (raw file):

							}
						}
						engine.Flush()

This is a lot of flushing! I'm not quite sure what this contention bit is testing. Given that it isn't changing the results, perhaps we should just remove it.

pkg/storage/bench_test.go, line 1065 at r1 (raw file):

	b.StopTimer()
	if contention {
		closeCh <- struct{}{}

You can close(closeCh) once, and all the waiters will read from the closed channel. This allows you to change the creation to an unbuffered channel: closeCh := make(chan struct{}).

itsbilal

TFTR! Added some benchstat runs to the commit message and PR description.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pbardea and @petermattis)

pkg/storage/bench_test.go, line 1009 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

In practice, most hlc.Timestamps have a 0 logical time.

Done.

pkg/storage/bench_test.go, line 1044 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is a lot of flushing! I'm not quite sure what this contention bit is testing. Given that it isn't changing the results, perhaps we should just remove it.

Done.

pkg/storage/bench_test.go, line 1065 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You can close(closeCh) once, and all the waiters will read from the closed channel. This allows you to change the creation to an unbuffered channel: closeCh := make(chan struct{}).

Done.

petermattis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @pbardea)

pkg/storage/bench_test.go, line 1044 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Done.

Ah, I meant that I think we could get rid of the whole contention section. It doesn't seem to be showing us anything beyond the non-contended case. I'm glad you made an attempt to see if "contention" had any impact on ExportToSst performance, but just because the code is written doesn't mean we need to keep it.

pbardea

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @pbardea)

pkg/storage/bench_test.go, line 1055 at r2 (raw file):

		startTS := hlc.Timestamp{WallTime: int64(numRevisions / 2)}
		endTS := hlc.Timestamp{WallTime: int64(numRevisions + 2)}
		_, _, _, err := engine.ExportToSst(roachpb.KeyMin, roachpb.KeyMax, startTS, endTS, exportAllRevisions, 0, 0, IterOptions{

nit: I think comments around the 0 targetSize and maxSize args here would be helpful for readability

As part of the investigation into cockroachdb#49710, this change adds a benchmark for ExportToSst that tests both RocksDB and Pebble. Here are some example runs (old = rocksdb, new = pebble): name old time/op new time/op delta ExportToSst/rocksdb/numKeys=64/numRevisions=1-12 43.9µs ± 3% 34.5µs ± 4% -21.33% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=64/numRevisions=10-12 281µs ± 3% 169µs ± 6% -39.89% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=64/numRevisions=100-12 1.82ms ±22% 1.17ms ± 1% -35.73% (p=0.000 n=10+9) ExportToSst/rocksdb/numKeys=512/numRevisions=1-12 212µs ± 6% 111µs ± 3% -47.77% (p=0.000 n=10+9) ExportToSst/rocksdb/numKeys=512/numRevisions=10-12 1.91ms ± 1% 1.19ms ± 8% -37.65% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=512/numRevisions=100-12 13.7ms ± 3% 10.1ms ±12% -26.21% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=1024/numRevisions=1-12 390µs ± 1% 215µs ±12% -44.94% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=1024/numRevisions=10-12 4.01ms ± 6% 2.40ms ±16% -40.13% (p=0.000 n=10+9) ExportToSst/rocksdb/numKeys=1024/numRevisions=100-12 27.9ms ± 2% 20.8ms ± 2% -25.48% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=8192/numRevisions=1-12 2.97ms ± 2% 1.42ms ± 5% -52.24% (p=0.000 n=9+10) ExportToSst/rocksdb/numKeys=8192/numRevisions=10-12 32.8ms ± 7% 19.1ms ± 3% -41.59% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=8192/numRevisions=100-12 224ms ± 3% 169ms ±25% -24.64% (p=0.000 n=9+10) ExportToSst/rocksdb/numKeys=65536/numRevisions=1-12 23.7ms ± 4% 13.4ms ±20% -43.65% (p=0.000 n=9+10) ExportToSst/rocksdb/numKeys=65536/numRevisions=10-12 264ms ± 4% 201ms ±24% -23.92% (p=0.000 n=10+10) ExportToSst/rocksdb/numKeys=65536/numRevisions=100-12 1.88s ± 6% 1.23s ± 8% -34.70% (p=0.000 n=10+8) Release note: None.

itsbilal

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @petermattis)

pkg/storage/bench_test.go, line 1044 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ah, I meant that I think we could get rid of the whole contention section. It doesn't seem to be showing us anything beyond the non-contended case. I'm glad you made an attempt to see if "contention" had any impact on ExportToSst performance, but just because the code is written doesn't mean we need to keep it.

Done. Removed.

pkg/storage/bench_test.go, line 1055 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: I think comments around the 0 targetSize and maxSize args here would be helpful for readability

Done.

petermattis

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal)

itsbilal · 2020-06-03T15:46:21Z

TFTRs!

bors r+

craig · 2020-06-03T17:00:27Z

Build succeeded

GitHub CI (Cockroach)

A previous change (cockroachdb#49721) adding a benchmark for ExportToSst left in some extra unused variables in a helper method. This change cleans that up. Release note: None.

49852: storage: Remove extra vars from runExportToSst r=itsbilal a=itsbilal A previous change (#49721) adding a benchmark for ExportToSst left in some extra unused variables in a helper method. This change cleans that up. Release note: None. Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>

itsbilal requested review from pbardea and petermattis May 29, 2020 22:01

itsbilal self-assigned this May 29, 2020

petermattis reviewed May 29, 2020

View reviewed changes

itsbilal force-pushed the benchmark-exporttosst branch 2 times, most recently from 2c50d7b to 0568f68 Compare June 1, 2020 16:29

itsbilal commented Jun 1, 2020

View reviewed changes

itsbilal force-pushed the benchmark-exporttosst branch from 0568f68 to 7dca685 Compare June 1, 2020 16:31

petermattis reviewed Jun 2, 2020

View reviewed changes

pbardea approved these changes Jun 2, 2020

View reviewed changes

itsbilal force-pushed the benchmark-exporttosst branch from 7dca685 to 1d6fb2a Compare June 2, 2020 22:52

itsbilal force-pushed the benchmark-exporttosst branch from 1d6fb2a to 16cbd80 Compare June 2, 2020 22:53

itsbilal commented Jun 2, 2020

View reviewed changes

petermattis approved these changes Jun 2, 2020

View reviewed changes

craig bot merged commit c8808ad into cockroachdb:master Jun 3, 2020

itsbilal mentioned this pull request Jun 3, 2020

storage: Remove extra vars from runExportToSst #49852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Add rocksdb-vs-pebble benchmark for ExportToSst#49721

storage: Add rocksdb-vs-pebble benchmark for ExportToSst#49721
craig[bot] merged 1 commit intocockroachdb:masterfrom
itsbilal:benchmark-exporttosst

itsbilal commented May 29, 2020 •

edited

Loading

Uh oh!

cockroach-teamcity commented May 29, 2020

Uh oh!

petermattis left a comment

Uh oh!

itsbilal left a comment

Uh oh!

petermattis left a comment

Uh oh!

pbardea left a comment

Uh oh!

itsbilal left a comment

Uh oh!

petermattis left a comment

Uh oh!

itsbilal commented Jun 3, 2020

Uh oh!

craig bot commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

itsbilal commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented May 29, 2020

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

itsbilal left a comment

Choose a reason for hiding this comment

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

pbardea left a comment

Choose a reason for hiding this comment

Uh oh!

itsbilal left a comment

Choose a reason for hiding this comment

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

itsbilal commented Jun 3, 2020

Uh oh!

craig bot commented Jun 3, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itsbilal commented May 29, 2020 •

edited

Loading