-
Notifications
You must be signed in to change notification settings - Fork 468
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
db: optimize levelIter for non-matching bloom filter #1091
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.
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @petermattis)
level_iter.go, line 453 at r1 (raw file):
// likely that the next key will also be contained in the current file. if n := l.split(l.iterFile.Largest.UserKey); l.cmp(prefix, l.iterFile.Largest.UserKey[:n]) < 0 { return nil, nil
these 2 lines are the only real change.
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.
Reviewed 6 of 9 files at r1.
Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @petermattis)
When SeekPrefixGE on the underlying file returns false due to a bloom filter non-match, levelIter would skip to the next file. This is wasteful if the upper bound of the file is beyond the prefix. Additionally, it defeats the optimization for sparse key spaces like CockroachDB's lock table, where we try to reuse the current position of the iterator -- by skipping to the next file the subsequent SeekPrefixGE will again need to reload the previous file. This behavior was first noticed when diagnosing tpcc slowness in CockroacbDB, where almost half the overhead of seeking in the lock table could be attributed to this (see cockroachdb/cockroach#62078 for details). The benchmark numbers for bloom=true/with-tombstone=false are the ones intended to benefit from this change. name old time/op new time/op delta IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=false-16 441ns ± 9% 445ns ± 7% ~ (p=0.332 n=19+20) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=true-16 299ns ± 3% 300ns ± 3% ~ (p=0.455 n=20+20) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=false-16 3.73µs ± 8% 0.82µs ± 2% -78.02% (p=0.000 n=20+16) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=true-16 1.78µs ±73% 1.21µs ± 7% -32.15% (p=0.000 n=20+20) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=false-16 484ns ±27% 427ns ± 2% -11.83% (p=0.000 n=19+19) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=true-16 320ns ± 7% 300ns ± 3% -6.11% (p=0.000 n=16+19) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=false-16 5.07µs ±41% 0.82µs ± 2% -83.84% (p=0.000 n=20+18) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=true-16 1.76µs ±37% 1.21µs ± 9% -30.92% (p=0.000 n=20+20) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=false-16 439ns ± 4% 436ns ± 6% ~ (p=0.109 n=20+20) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=true-16 435ns ±29% 307ns ± 5% -29.40% (p=0.000 n=20+19) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=false-16 5.63µs ±19% 0.82µs ± 2% -85.40% (p=0.000 n=20+19) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=true-16 1.87µs ±36% 1.24µs ± 8% -33.66% (p=0.000 n=20+20) name old alloc/op new alloc/op delta IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=true-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=true-16 271B ± 0% 271B ± 0% ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=true-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=true-16 271B ± 0% 271B ± 0% ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=true-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=false-16 0.00B 0.00B ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=true-16 271B ± 0% 271B ± 0% ~ (all equal) name old allocs/op new allocs/op delta IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=false/with-tombstone=true-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=1/bloom=true/with-tombstone=true-16 1.00 ± 0% 1.00 ± 0% ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=false/with-tombstone=true-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=2/bloom=true/with-tombstone=true-16 1.00 ± 0% 1.00 ± 0% ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=false/with-tombstone=true-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=false-16 0.00 0.00 ~ (all equal) IteratorSeqSeekPrefixGENotFound/skip=4/bloom=true/with-tombstone=true-16 1.00 ± 0% 1.00 ± 0% ~ (all equal)
f369318
to
9065117
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!
I've added a test case to TestLevelIterSeek
and run the metamorphic test for 2h.
Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @petermattis)
When SeekPrefixGE on the underlying file returns false
due to a bloom filter non-match, levelIter would skip
to the next file. This is wasteful if the upper bound
of the file is beyond the prefix. Additionally, it
defeats the optimization for sparse key spaces like
CockroachDB's lock table, where we try to reuse the
current position of the iterator -- by skipping to the
next file the subsequent SeekPrefixGE will again need
to reload the previous file.
This behavior was first noticed when diagnosing tpcc
slowness in CockroacbDB, where almost half the
overhead of seeking in the lock table could be
attributed to this (see
cockroachdb/cockroach#62078
for details).
The benchmark numbers for bloom=true/with-tombstone=false
are the ones intended to benefit from this change.