storage: require valid iterator for HasPointAndRange()#84961
storage: require valid iterator for HasPointAndRange()#84961craig[bot] merged 1 commit intocockroachdb:masterfrom
HasPointAndRange()#84961Conversation
|
This change is |
2b55bce to
c10f0c2
Compare
jbowens
left a comment
There was a problem hiding this comment.
Reviewed 10 of 12 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @itsbilal, and @nicktrav)
pkg/storage/point_synthesizing_iter.go line 516 at r1 (raw file):
if _, err := i.updateValid(); err != nil { return }
nit: would it make sense to implement iterNext and iterPrev methods that step i.iter and update validity, returning (ok bool, err error)? might cut down on the boilerplate
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/point_synthesizing_iter.go line 516 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: would it make sense to implement
iterNextanditerPrevmethods that stepi.iterand update validity, returning(ok bool, err error)? might cut down on the boilerplate
I'm kind of hoping that we'll get around to changing the MVCCIterator interface to do this in #82589. Might be able to squeeze it in before stability.
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/point_synthesizing_iter.go line 516 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
I'm kind of hoping that we'll get around to changing the
MVCCIteratorinterface to do this in #82589. Might be able to squeeze it in before stability.
Then again, I suppose the helpers would be even more useful in that case. Will add them in a bit.
This patch requires the caller to check `Valid()` before calling `SimpleMVCCIterator.HasPointAndRange()`. This avoids making redundant `Valid()` calls internally in `HasPointAndRange()`, which has a non-negligible cost, especially considering callers may often make multiple calls to `HasPointAndRange()` but only a single call to `Valid()` per step. This improves scan performance by as much as 2.5% in the no-range-key case. This also allows combining `EngineIterator.HasEnginePointAndRange()` with `HasPointAndRange()`, since `pebbleIterator.Valid()` could not be called from `EngineIterator` methods (due to lock table checks). ``` MVCCScan_Pebble/rows=1/versions=1/valueSize=64-24 4.75µs ± 2% 4.78µs ± 2% ~ (p=0.210 n=10+10) MVCCScan_Pebble/rows=1/versions=10/valueSize=64-24 6.63µs ± 1% 6.62µs ± 1% ~ (p=0.590 n=9+10) MVCCScan_Pebble/rows=100/versions=1/valueSize=64-24 38.9µs ± 1% 38.4µs ± 1% -1.45% (p=0.001 n=10+10) MVCCScan_Pebble/rows=100/versions=10/valueSize=64-24 124µs ± 1% 121µs ± 1% -2.52% (p=0.000 n=10+10) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64-24 2.91ms ± 1% 2.84ms ± 1% -2.39% (p=0.000 n=10+10) MVCCScan_Pebble/rows=10000/versions=10/valueSize=64-24 10.8ms ± 1% 10.5ms ± 1% -2.17% (p=0.000 n=10+9) MVCCGet_Pebble/batch=false/versions=1/valueSize=8-24 4.38µs ± 1% 4.37µs ± 1% ~ (p=0.920 n=10+9) MVCCGet_Pebble/batch=false/versions=10/valueSize=8-24 5.41µs ± 1% 5.39µs ± 2% ~ (p=0.170 n=10+10) MVCCGet_Pebble/batch=false/versions=100/valueSize=8-24 13.4µs ± 2% 13.2µs ± 1% -1.01% (p=0.016 n=10+9) MVCCGet_Pebble/batch=true/versions=1/valueSize=8-24 2.78µs ± 1% 2.77µs ± 1% -0.50% (p=0.021 n=10+9) MVCCGet_Pebble/batch=true/versions=10/valueSize=8-24 3.98µs ± 1% 3.98µs ± 1% ~ (p=0.921 n=9+10) MVCCGet_Pebble/batch=true/versions=100/valueSize=8-24 10.7µs ± 3% 10.6µs ± 5% ~ (p=0.684 n=10+10) ``` Release note: None
c10f0c2 to
898471f
Compare
|
CI failures are unrelated. TFTR! bors r=jbowens |
|
Build failed: |
|
This should go through once #84982 merges, so I'm going to queue it up. bors r=jbowens |
|
Build succeeded: |
This patch requires the caller to check
Valid()before callingSimpleMVCCIterator.HasPointAndRange(). This avoids making redundantValid()calls internally inHasPointAndRange(), which has anon-negligible cost, especially considering callers may often make
multiple calls to
HasPointAndRange()but only a single call toValid()per step. This improves scan performance by as much as2.5% in the no-range-key case.
This also allows combining
EngineIterator.HasEnginePointAndRange()withHasPointAndRange(), sincepebbleIterator.Valid()could not be calledfrom
EngineIteratormethods (due to lock table checks).Resolves #83801.
Release note: None