storage: re-introduce dual buffer for iterator bounds#79993
storage: re-introduce dual buffer for iterator bounds#79993erikgrinaker wants to merge 2 commits intocockroachdb:mvcc-range-tombstonesfrom
Conversation
|
This change is |
0c0dd89 to
06b997e
Compare
|
Performance diff when reading from a batch (reusing iterators) is negligible, and within error margins (c.f. benchmarks in #79291): |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @jbowens, and @nicktrav)
-- commits, line 5 at r2:
Can you look into why TestPebbleIterBoundSliceStabilityAndNoop did not fail?
Is it because it was removed since SetBounds is no more? We need a test that would have caught this bug.
pkg/storage/pebble_iterator.go, line 214 at r2 (raw file):
// require either Prefix, UpperBound, or LowerBound to be set, so one of these // won't match the zero value of a new iterator. optsChanged := opts.Prefix != p.prefix ||
I didn't spot a new test for this noop optimization. If there isn't one, please add.
From past experience, this is the kind of thing that results in extremely long and painful roachtest failure investigations.
6a1756c to
3bd8605
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, sumeerbhola wrote…
Can you look into why TestPebbleIterBoundSliceStabilityAndNoop did not fail?
Is it because it was removed since SetBounds is no more? We need a test that would have caught this bug.
Ah, yes, it was removed. Sloppy of me not to look closer here.
pkg/storage/pebble_iterator.go, line 214 at r2 (raw file):
Previously, sumeerbhola wrote…
I didn't spot a new test for this noop optimization. If there isn't one, please add.
From past experience, this is the kind of thing that results in extremely long and painful roachtest failure investigations.
Let's see where we land on cockroachdb/pebble#1640 first. I think it'd be beneficial to move this into Pebble, since it'd allow for more fine-grained optimization and less coupling.
492183c to
e72c044
Compare
3bd8605 to
2c06ce4
Compare
e72c044 to
a9963be
Compare
2c06ce4 to
cb77b2e
Compare
a9963be to
0226f8b
Compare
cb77b2e to
49aacac
Compare
0226f8b to
bdc0ee9
Compare
49aacac to
e5bd560
Compare
This is necessary to avoid mutating the bounds in use by `pebble.Iterator`, which can interfere with seek optimizations. Release note: None
Since we now use dual buffers for bounds encoding, we can fully generate the new `pebble.IterOptions` during `setOptions()` and compare them directly with the existing options, which simplifies the code. Release note: None
bdc0ee9 to
5214cf3
Compare
|
Superseded by #81242. |
For the
mvcc-range-tombstonesbranch. Undoes a change in #79291 and #79744. Will be squashed into the original commit once merged.storage: re-introduce dual buffer for iterator bounds
This is necessary to avoid mutating the bounds in use by
pebble.Iterator, which can interfere with seek optimizations.Release note: None
storage: simplify
pebbleIteratoroptions comparisonsSince we now use dual buffers for bounds encoding, we can fully generate
the new
pebble.IterOptionsduringsetOptions()and compare themdirectly with the existing options, which simplifies the code.
Release note: None