db: exclude empty batch range deletion, range key iterators#1749
db: exclude empty batch range deletion, range key iterators#1749jbowens merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
jbowens
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion
iterator.go line 1969 at r1 (raw file):
i.batch.initRangeKeyIter(&i.opts, &i.batchRangeKeyIter, nextBatchSeqNum) i.invalidate() }
The go cover tool confirms the existing TestIndexedBatchMutation exercises all of these branches.
nicktrav
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens and @nicktrav)
iterator.go line 1934 at r1 (raw file):
i.batchSeqNum = nextBatchSeqNum if i.pointIter != nil { if prevCount := i.batchRangeDelIter.Count(); int(i.batch.countRangeDels) == prevCount {
I think the answer to this is "yes", but just confirming my understanding - the counts are monotonically increasing? It's not possible to, say, have the batch decrease by n then increase by n?
Thinking of the contrived case where n=m; m>0, then the batch is reset, so n=0, then m new range dels are added.
iterator.go line 1967 at r1 (raw file):
i.rangeKey = nil } else { i.batch.initRangeKeyIter(&i.opts, &i.batchRangeKeyIter, nextBatchSeqNum)
nit: similar comment to the one for range dels here too, for completeness? "New range keys have been added ... can update the iterator in place"
Since e32e94d Pebble has unconditionally included batch rangedel iterators and batch rangekey iterators in the iterator stack. This was done to allow SetOptions to update these batch iterators if rangedels or rangekeys were added to the batch between iterator construction and the call to SetOptions. In this commit, we update iterator construction to again exclude these empty iterators. To compensate, SetOptions must now test whether any range deletions or range keys existed at iterator construction, and recreate the point/range key iterator stacks if necessary. Cases that require iterator reconstruction are expected to be rare, whereas empty batch rangedel and rangekey iterators are expected to be very common. ``` name old time/op new time/op delta IteratorSeekNoRangeKeys/batch=false-10 2.90µs ± 4% 2.93µs ± 0% +1.25% (p=0.000 n=20+17) IteratorSeekNoRangeKeys/batch=true-10 5.37µs ± 1% 4.61µs ± 1% -14.14% (p=0.000 n=20+20) name old alloc/op new alloc/op delta IteratorSeekNoRangeKeys/batch=false-10 16.0B ± 0% 16.0B ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 128B ± 0% 32B ± 0% -75.00% (p=0.000 n=20+20) name old allocs/op new allocs/op delta IteratorSeekNoRangeKeys/batch=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 5.00 ± 0% 2.00 ± 0% -60.00% (p=0.000 n=20+20) ```
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nicktrav)
iterator.go line 1934 at r1 (raw file):
Previously, nicktrav (Nick Travers) wrote…
I think the answer to this is "yes", but just confirming my understanding - the counts are monotonically increasing? It's not possible to, say, have the batch decrease by
nthen increase byn?Thinking of the contrived case where
n=m; m>0, then the batch is reset, son=0, thenmnew range dels are added.
Yeah, that's right. Added a comment stating as such.
Batch.Reset is only legal when finished with a batch, to reset for reuse, so there must not be open iterators over the batch.
iterator.go line 1967 at r1 (raw file):
Previously, nicktrav (Nick Travers) wrote…
nit: similar comment to the one for range dels here too, for completeness? "New range keys have been added ... can update the iterator in place"
done
Since e32e94d Pebble has unconditionally included batch rangedel iterators
and batch rangekey iterators in the iterator stack. This was done to allow
SetOptions to update these batch iterators if rangedels or rangekeys were added
to the batch between iterator construction and the call to SetOptions.
In this commit, we update iterator construction to again exclude these empty
iterators. To compensate, SetOptions must now test whether any range deletions
or range keys existed at iterator construction, and recreate the point/range
key iterator stacks if necessary. Cases that require iterator reconstruction
are expected to be rare, whereas empty batch rangedel and rangekey iterators
are expected to be very common.