-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: Add rocksdb-vs-pebble benchmark for ExportToSst #49721
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{})
.
2c50d7b
to
0568f68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0568f68
to
7dca685
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
7dca685
to
1d6fb2a
Compare
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.
1d6fb2a
to
16cbd80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onExportToSst
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal)
TFTRs! bors r+ |
Build succeeded |
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>
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):
And some with contention=true:
Release note: None.