Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
86259: storage: implement `RangeKeyChanged()` for `MVCCIncrementalIterator` r=tbg a=erikgrinaker **storage: use `RangeKeyChanged()` in `MVCCIncrementalIterator`** This patch uses `RangeKeyChanged()` to detect changes to range keys in `MVCCIncrementalIterator`. There are no functional changes. Some quick benchmarks, using catchup scans: ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=0-24 538ms ± 1% 535ms ± 1% ~ (p=0.211 n=10+9) CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=1-24 690ms ± 0% 590ms ± 1% -14.56% (p=0.000 n=9+10) CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=100-24 743ms ± 1% 646ms ± 1% -13.13% (p=0.000 n=9+9) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 794ms ± 1% 794ms ± 0% ~ (p=0.579 n=10+10) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 966ms ± 0% 911ms ± 1% -5.72% (p=0.000 n=10+10) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 974ms ± 0% 920ms ± 0% -5.51% (p=0.000 n=10+10) ``` Release justification: bug fixes and low-risk updates to new functionality. Release note: None **storage: implement `RangeKeyChanged()` for `MVCCIncrementalIterator`** This patch implements `RangeKeyChanged()` for `MVCCIncrementalIterator`. It only fires if the time bound range keys change, not if a `Next(Key)IgnoringTime()` operation reveals additional range keys. Resolves #86105. Release justification: bug fixes and low-risk updates to new functionality Release note: None **storage: add `MVCCIncrementalIterator.RangeKeysIgnoringTime()`** This patch changes `MVCCIncrementalIterator` range key behavior following a `Next(Key)IgnoringTime()` call. Previously, range key methods would then expose unfiltered range keys. Now, the standard range key methods only apply to filtered range keys, and an additional `RangeKeysIgnoringTime()` method provides access to unfiltered range keys. This implies that if such a call steps onto a range key that's entirely outside of the time bounds then: * `HasPointAndRange()` will return `false`,`false` if on a bare range key. * `RangeKeyChanged()` will not fire, unless stepping off of a range key within the time bounds. * `RangeBounds()` and `RangeKeys()` will return empty results. This is done to avoid conditional range key handling following these calls, except for the exact sites where the caller is interested in the unfiltered range keys. Release justification: bug fixes and low-risk updates to new functionality Release note: None 86440: storage: use concrete `pebbleIterator` in `intentInterleavingIter` r=tbg a=erikgrinaker **storage: tweak `unsafeMVCCIterator` construction** Release justification: non-production code changes Release note: None **storage: inline some `intentInterleavingIter` methods** This patch splits up `maybeSkipIntentRangeKeys()` and `maybeSuppressRangeKeyChanged()` to allow for mid-stack inlining. I doubt that the gains are as large as these microbenchmarks claim, and there's a fair bit of variance between runs, but it can't hurt. ``` name old time/op new time/op delta MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.37µs ± 2% 5.43µs ± 2% +1.13% (p=0.041 n=10+10) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 38.2µs ± 2% 38.2µs ± 2% ~ (p=0.971 n=10+10) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 2.79ms ± 2% 2.71ms ± 2% -2.59% (p=0.000 n=10+10) MVCCReverseScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.99µs ± 1% 5.99µs ± 2% ~ (p=0.541 n=10+10) MVCCReverseScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 51.7µs ± 3% 52.1µs ± 1% ~ (p=0.631 n=10+10) MVCCReverseScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 3.88ms ± 2% 3.87ms ± 1% ~ (p=0.897 n=10+8) MVCCComputeStats_Pebble/valueSize=32/numRangeKeys=0-24 158ms ± 1% 155ms ± 1% -2.34% (p=0.000 n=10+9) ``` Touches #82559. Release justification: bug fixes and low-risk updates to new functionality Release note: None **storage: use concrete `pebbleIterator` in `intentInterleavingIter`** Since `intentInterleavingIter` always constructs the underlying iterators from the given reader, and these readers always construct `*pebbleIterator`, it can use the concrete type rather than interfaces for both iterators. This avoids dynamic dispatch, yielding a decent performance improvement. Unfortunately, this requires disabling `unsafeMVCCIterator` inside `intentInterleavingIter`. This wasn't always enabled anyway, since it was omitted both for the engine iterator and when using an engine directly (which doesn't have consistent iterators). ``` name old time/op new time/op delta MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.43µs ± 2% 5.45µs ± 2% ~ (p=0.566 n=10+10) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 38.2µs ± 2% 37.0µs ± 1% -3.02% (p=0.000 n=10+10) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 2.71ms ± 2% 2.66ms ± 1% -1.83% (p=0.000 n=10+9) MVCCReverseScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.99µs ± 2% 5.86µs ± 2% -2.15% (p=0.000 n=10+10) MVCCReverseScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 52.1µs ± 1% 50.2µs ± 2% -3.77% (p=0.000 n=10+10) MVCCReverseScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 3.87ms ± 1% 3.83ms ± 1% -1.26% (p=0.000 n=8+10) MVCCComputeStats_Pebble/valueSize=32/numRangeKeys=0-24 155ms ± 1% 155ms ± 1% ~ (p=0.842 n=9+10) ``` Touches #82559. Release justification: low risk, high benefit changes to existing functionality Release note: None 86478: storage: use concrete `pebbleIterator` in `verifyingIterator` r=tbg a=erikgrinaker **storage: add Pebble SST iterator benchmarks** Release justification: non-production code changes Release note: None **storage: use concrete `pebbleIterator` in `verifyingIterator`** Gives a slight performance boost, since it avoid dynamic dispatch: ``` name old time/op new time/op delta SSTIterator/keys=1/variant=pebble/verify=true-24 42.8µs ± 1% 42.4µs ± 1% -0.79% (p=0.043 n=10+10) SSTIterator/keys=100/variant=pebble/verify=true-24 61.8µs ± 1% 60.7µs ± 1% -1.64% (p=0.000 n=10+10) SSTIterator/keys=10000/variant=pebble/verify=true-24 1.91ms ± 0% 1.88ms ± 0% -1.79% (p=0.000 n=10+10) ``` An attempt was also made at using `RangeKeyChanged()` instead of `HasPointAndRange()`, but this had no effect. Touches #83051. Release justification: bug fixes and low-risk updates to new functionality Release note: None 86513: storage: use `RangeKeyChanged` in `ReadAsOfIterator` r=tbg a=erikgrinaker This patch uses `RangeKeyChanged()` to detect range keys in `ReadAsOfIterator`, and caches them to improve performance. It also fixes a bug where the iterator would fail to detect tombstones with a non-empty `MVCCValueHeader`. Resolves #84714. Release justification: bug fixes and low-risk updates to new functionality Release note: None 86514: storage: use `RangeKeyChanged()` in `MVCCDeleteRangeUsingTombstone()` r=tbg a=erikgrinaker Release justification: bug fixes and low-risk updates to new functionality Release note: None 86529: storage: omit unnecessary range key calls during intent resolution r=tbg a=erikgrinaker Release justification: bug fixes and low-risk updates to new functionality Release note: None Co-authored-by: Erik Grinaker <[email protected]>
- Loading branch information