-
Notifications
You must be signed in to change notification settings - Fork 451
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
colblk: implement HideObsoletePoints in DataBlockIter #3982
colblk: implement HideObsoletePoints in DataBlockIter #3982
Conversation
8626db6
to
d926b52
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.
Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @RaduBerinde)
sstable/colblk/data_block.go
line 805 at r1 (raw file):
i.nextObsoletePoint = i.r.isObsolete.SeekSetBitGE(i.row) } else { i.nextObsoletePoint = i.maxRow + 1
if i.row == i.maxRow
, isn't it possible for i.r.isObsolete.At(i.row) == true
, in which case we want to set i.nextObsoletePoint = i.row
?
sstable/colblk/data_block.go
line 806 at r1 (raw file):
} else { i.nextObsoletePoint = i.maxRow + 1 }
would it make things cleaner if Seek[Unset|Set]BitGE(n)
where n >= BitmapBuilder.bitCount
was defined as BitmapBuilder.bitCount
? so that even if i.row == i.maxRow+1
, we can blindly call Seek[Unset|Set]BitGE
?
sstable/colblk/data_block.go
line 963 at r1 (raw file):
//gcassert:inline func (i *DataBlockIter) atObsoletePointForward() bool { return i.row == i.nextObsoletePoint && i.row <= i.maxRow
is there an invariant that if i.row != i.nextObsoletePoint
, i.row < i.nextObsoletePoint
? should we assert as much?
d926b52
to
78b7785
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!
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @jbowens)
sstable/colblk/data_block.go
line 805 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
if
i.row == i.maxRow
, isn't it possible fori.r.isObsolete.At(i.row) == true
, in which case we want to seti.nextObsoletePoint = i.row
?
Nice catch, added a test for this case.
sstable/colblk/data_block.go
line 806 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
would it make things cleaner if
Seek[Unset|Set]BitGE(n)
wheren >= BitmapBuilder.bitCount
was defined asBitmapBuilder.bitCount
? so that even ifi.row == i.maxRow+1
, we can blindly callSeek[Unset|Set]BitGE
?
I was afraid it might cause other paths (like prefixChanged.SeekSetBitGE()
to regress. I did it in a local method though, much better.
sstable/colblk/data_block.go
line 963 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
is there an invariant that if
i.row != i.nextObsoletePoint
,i.row < i.nextObsoletePoint
? should we assert as much?
Yes, added an assert here (I was afraid it would mess with the gcassert:inline check, but then I realized that the build for the check doesn't use the invariants tag).
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 2 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved
:'( |
So strange, |
78b7785
to
48dcc5e
Compare
Ok, moved the check to
|
Benchmark for the no-transform path before/after: ``` name old time/op new time/op delta CockroachDataBlockIterShort/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8/Next-10 12.2ns ± 0% 12.5ns ± 3% +2.38% (p=0.008 n=5+5) CockroachDataBlockIterShort/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8/SeekGE-10 113ns ± 0% 114ns ± 0% +0.80% (p=0.008 n=5+5) CockroachDataBlockIterShort/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128/Next-10 10.1ns ± 0% 10.3ns ± 0% +2.30% (p=0.016 n=5+4) CockroachDataBlockIterShort/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128/SeekGE-10 89.4ns ± 0% 90.3ns ± 0% +1.08% (p=0.008 n=5+5) ``` Transforms benchmarks: ``` name time/op CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8/Next-10 12.3ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8/SeekGE-10 113ns ± 0% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8,SynthSeqNum/Next-10 12.5ns ± 0% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8,SynthSeqNum/SeekGE-10 114ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8,HideObsolete/Next-10 13.0ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=8,Shared=4,ValueLen=8,HideObsolete/SeekGE-10 117ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128/Next-10 10.3ns ± 0% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128/SeekGE-10 90.5ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128,SynthSeqNum/Next-10 10.5ns ± 0% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128,SynthSeqNum/SeekGE-10 90.9ns ± 1% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128,HideObsolete/Next-10 10.8ns ± 0% CockroachDataBlockIterTransforms/AlphaLen=8,Prefix=128,Shared=64,ValueLen=128,HideObsolete/SeekGE-10 93.9ns ± 0% ```
48dcc5e
to
527e1ab
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: 3 of 6 files reviewed, all discussions resolved
TFTR! |
colblk: minor benchmark refactoring
Adding a "short" version for the iterator benchmarks. We will add
combinations of various transforms with these selected configs.
We also slightly improve the benchmark naming.
colblk: implement HideObsoletePoints in DataBlockIter
Benchmark for the no-transform path before/after:
Transforms benchmarks: