From d757c720c7e36b69af1f974d0ac052fd061d8242 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 14 Aug 2022 22:11:27 +0000 Subject: [PATCH 1/5] storage: improve `pebbleMVCCScanner` range key assertions Release note: None --- pkg/storage/pebble_mvcc_scanner.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/storage/pebble_mvcc_scanner.go b/pkg/storage/pebble_mvcc_scanner.go index c71ff6a9c41f..be6a42d9d6cf 100644 --- a/pkg/storage/pebble_mvcc_scanner.go +++ b/pkg/storage/pebble_mvcc_scanner.go @@ -1199,7 +1199,7 @@ func (p *pebbleMVCCScanner) enablePointSynthesis() { if ok, _ := p.parent.Valid(); !ok { panic(errors.AssertionFailedf("enablePointSynthesis called with invalid iter")) } - if _, ok := p.parent.(*pointSynthesizingIter); ok { + if p.pointIter != nil { panic(errors.AssertionFailedf("enablePointSynthesis called when already enabled")) } if _, hasRange := p.parent.HasPointAndRange(); !hasRange { @@ -1256,6 +1256,12 @@ func (p *pebbleMVCCScanner) iterValid() bool { if p.parent.RangeKeyChanged() { p.enablePointSynthesis() } + if util.RaceEnabled && p.pointIter == nil { + if _, hasRange := p.parent.HasPointAndRange(); hasRange { + panic(errors.AssertionFailedf("range key did not trigger point synthesis at %s", + p.parent.UnsafeKey())) + } + } return true } From a62f9675113b023b8c4ad222cf487379e1dfa596 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sat, 13 Aug 2022 19:01:54 +0000 Subject: [PATCH 2/5] storage: use `RangeKeyChanged()` in `pointSynthesizingIter` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch uses `RangeKeyChanged()` to detect changes to range keys in `pointSynthesizingIter`, avoiding repeated key decoding and comparisons in the hot path. ``` name old time/op new time/op delta MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.89µs ± 1% 5.94µs ± 0% +0.74% (p=0.008 n=5+5) MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=1-24 12.9µs ± 0% 13.0µs ± 1% ~ (p=0.151 n=5+5) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 41.9µs ± 1% 43.0µs ± 1% +2.62% (p=0.008 n=5+5) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=1-24 75.7µs ± 1% 71.9µs ± 1% -4.90% (p=0.008 n=5+5) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 2.97ms ± 1% 2.97ms ± 1% ~ (p=0.841 n=5+5) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=1-24 5.62ms ± 1% 5.13ms ± 0% -8.88% (p=0.008 n=5+5) ``` Release note: None --- pkg/storage/point_synthesizing_iter.go | 36 ++++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/pkg/storage/point_synthesizing_iter.go b/pkg/storage/point_synthesizing_iter.go index 8a861f1b8a1f..2e744a9e233e 100644 --- a/pkg/storage/point_synthesizing_iter.go +++ b/pkg/storage/point_synthesizing_iter.go @@ -86,7 +86,8 @@ type pointSynthesizingIter struct { rangeKeysEnd int // rangeKeysStart contains the start key of the current rangeKeys stack. It is - // only used to memoize rangeKeys for adjacent keys. + // stored separately, rather than holding the entire MVCCRangeKeyStack, to + // avoid cloning the EndKey. rangeKeysStart roachpb.Key // atPoint is true if the synthesizing iterator is positioned on a real point @@ -107,6 +108,11 @@ type pointSynthesizingIter struct { // iterErr contains the error from the underlying iterator, if any. iterErr error + + // rangeKeyChanged keeps track of any changes to the underlying iter's range + // keys. It is reset to false on every call to updateRangeKeys(), and + // accumulates changes during intermediate positioning operations. + rangeKeyChanged bool } var _ MVCCIterator = new(pointSynthesizingIter) @@ -131,6 +137,7 @@ func newPointSynthesizingIter(parent MVCCIterator) *pointSynthesizingIter { func newPointSynthesizingIterAtParent(parent MVCCIterator) *pointSynthesizingIter { iter := newPointSynthesizingIter(parent) iter.iterValid = true + iter.rangeKeyChanged = true iter.updateSeekGEPosition(parent.UnsafeKey()) return iter } @@ -170,9 +177,13 @@ func (i *pointSynthesizingIter) iterPrev() (bool, error) { } // updateValid updates i.iterValid and i.iterErr based on the underlying -// iterator position, and returns them. +// iterator position, and returns them. It also keeps track of changes +// to range keys. func (i *pointSynthesizingIter) updateValid() (bool, error) { i.iterValid, i.iterErr = i.iter.Valid() + // TODO(erikgrinaker): This doesn't really belong in updateValid(), move it + // somewhere more suitable when we clean up the positioning logic. + i.rangeKeyChanged = i.rangeKeyChanged || i.iter.RangeKeyChanged() return i.iterValid, i.iterErr } @@ -186,16 +197,18 @@ func (i *pointSynthesizingIter) updateRangeKeys() { } else { i.rangeKeysPos = append(i.rangeKeysPos[:0], i.iter.UnsafeKey().Key...) if i.prefix { - // A prefix iterator will always be at the start bound of the range key, - // and never move onto a different range key, so we can omit the cloning - // and other processing. - i.rangeKeys = i.iter.RangeKeys().Versions - i.rangeKeysEnd = len(i.rangeKeys) + if i.rangeKeyChanged { + // A prefix iterator will always be at the start bound of the range key, + // and never move onto a different range key, so we can omit the cloning + // and other processing. + i.rangeKeys = i.iter.RangeKeys().Versions + i.rangeKeysEnd = len(i.rangeKeys) + } } else { - // TODO(erikgrinaker): Use RangeKeyChanged() to detect this. - if rangeStart := i.iter.RangeBounds().Key; !rangeStart.Equal(i.rangeKeysStart) { - i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeStart...) - i.iter.RangeKeys().Versions.CloneInto(&i.rangeKeysBuf) + if i.rangeKeyChanged { + rangeKeys := i.iter.RangeKeys() + i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...) + rangeKeys.Versions.CloneInto(&i.rangeKeysBuf) i.rangeKeys = i.rangeKeysBuf } if i.rangeKeysPos.Equal(i.rangeKeysStart) { @@ -205,6 +218,7 @@ func (i *pointSynthesizingIter) updateRangeKeys() { i.extendRangeKeysEnd() } } + i.rangeKeyChanged = false if !i.reverse { i.rangeKeysIdx = 0 } else { From 23eaed1644a70cd04aa0c5f63065cf23f45755fb Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 14 Aug 2022 14:15:45 +0000 Subject: [PATCH 3/5] storage: memoize parent state in `pointSynthesizingIter` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch memoizes the parent iterator state in `pointSynthesizingIter` every time its position changes, which omits a number of redundant function calls and key comparisons. ``` name old time/op new time/op delta MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.66µs ± 1% 5.57µs ± 1% -1.61% (p=0.000 n=10+10) MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=1-24 12.3µs ± 1% 11.9µs ± 1% -3.33% (p=0.000 n=10+10) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 38.7µs ± 2% 38.8µs ± 1% ~ (p=0.448 n=9+9) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=1-24 67.0µs ± 1% 60.0µs ± 1% -10.45% (p=0.000 n=10+10) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 2.81ms ± 1% 2.83ms ± 1% +0.73% (p=0.002 n=10+10) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=1-24 4.99ms ± 1% 4.29ms ± 1% -14.03% (p=0.000 n=10+9) MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24 3.33µs ± 2% 3.33µs ± 3% ~ (p=0.382 n=10+10) MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24 6.71µs ± 2% 6.59µs ± 1% -1.90% (p=0.000 n=10+10) ``` Release note: None --- pkg/storage/point_synthesizing_iter.go | 193 ++++++++++++++----------- 1 file changed, 112 insertions(+), 81 deletions(-) diff --git a/pkg/storage/point_synthesizing_iter.go b/pkg/storage/point_synthesizing_iter.go index 2e744a9e233e..43e915dcb1e4 100644 --- a/pkg/storage/point_synthesizing_iter.go +++ b/pkg/storage/point_synthesizing_iter.go @@ -103,15 +103,18 @@ type pointSynthesizingIter struct { // following a SeekLT or Prev call. reverse bool - // iterValid is true if the underlying iterator is valid. - iterValid bool - - // iterErr contains the error from the underlying iterator, if any. - iterErr error - - // rangeKeyChanged keeps track of any changes to the underlying iter's range - // keys. It is reset to false on every call to updateRangeKeys(), and - // accumulates changes during intermediate positioning operations. + // The following fields memoize the state of the underlying iterator, and are + // updated every time its position changes. + iterValid bool + iterErr error + iterKey MVCCKey + iterHasPoint bool + iterHasRange bool + iterAtRangeKeysPos bool + + // rangeKeyChanged keeps track of changes to the underlying iter's range keys. + // It is reset to false on every call to updateRangeKeys(), and accumulates + // changes during intermediate positioning operations. rangeKeyChanged bool } @@ -133,12 +136,13 @@ func newPointSynthesizingIter(parent MVCCIterator) *pointSynthesizingIter { } // newPointSynthesizingIterAtParent creates a new pointSynthesizingIter and -// loads the position from the parent iterator (which must be valid). +// loads the position from the parent iterator. func newPointSynthesizingIterAtParent(parent MVCCIterator) *pointSynthesizingIter { iter := newPointSynthesizingIter(parent) - iter.iterValid = true - iter.rangeKeyChanged = true - iter.updateSeekGEPosition(parent.UnsafeKey()) + iter.rangeKeyChanged = true // force range key detection + if ok, err := iter.updateIter(); ok && err == nil { + iter.updateSeekGEPosition(parent.UnsafeKey()) + } return iter } @@ -163,26 +167,33 @@ func (i *pointSynthesizingIter) release() { } // iterNext is a convenience function that calls iter.Next() -// and returns the value of updateValid(). +// and returns the value of updateIter(). func (i *pointSynthesizingIter) iterNext() (bool, error) { i.iter.Next() - return i.updateValid() + return i.updateIter() } // iterPrev is a convenience function that calls iter.Prev() -// and returns the value of updateValid(). +// and returns the value of updateIter(). func (i *pointSynthesizingIter) iterPrev() (bool, error) { i.iter.Prev() - return i.updateValid() -} - -// updateValid updates i.iterValid and i.iterErr based on the underlying -// iterator position, and returns them. It also keeps track of changes -// to range keys. -func (i *pointSynthesizingIter) updateValid() (bool, error) { - i.iterValid, i.iterErr = i.iter.Valid() - // TODO(erikgrinaker): This doesn't really belong in updateValid(), move it - // somewhere more suitable when we clean up the positioning logic. + return i.updateIter() +} + +// updateIter memoizes the iterator fields from the underlying iterator, and +// also keeps track of rangeKeyChanged. It returns the iterator validity and +// error. +func (i *pointSynthesizingIter) updateIter() (bool, error) { + if i.iterValid, i.iterErr = i.iter.Valid(); i.iterValid { + i.iterHasPoint, i.iterHasRange = i.iter.HasPointAndRange() + i.iterKey = i.iter.UnsafeKey() + i.iterAtRangeKeysPos = (i.prefix && i.iterHasRange) || + (!i.prefix && i.iterKey.Key.Equal(i.rangeKeysPos)) + } else { + i.iterHasPoint, i.iterHasRange = false, false + i.iterKey = MVCCKey{} + i.iterAtRangeKeysPos = false + } i.rangeKeyChanged = i.rangeKeyChanged || i.iter.RangeKeyChanged() return i.iterValid, i.iterErr } @@ -190,12 +201,11 @@ func (i *pointSynthesizingIter) updateValid() (bool, error) { // updateRangeKeys updates i.rangeKeys and related fields with range keys from // the underlying iterator. rangeKeysIdx is reset to the first/last range key. func (i *pointSynthesizingIter) updateRangeKeys() { - if !i.iterValid { - i.clearRangeKeys() - } else if _, hasRange := i.iter.HasPointAndRange(); !hasRange { + if !i.iterHasRange { i.clearRangeKeys() } else { - i.rangeKeysPos = append(i.rangeKeysPos[:0], i.iter.UnsafeKey().Key...) + i.rangeKeysPos = append(i.rangeKeysPos[:0], i.iterKey.Key...) + i.iterAtRangeKeysPos = true // by definition if i.prefix { if i.rangeKeyChanged { // A prefix iterator will always be at the start bound of the range key, @@ -230,16 +240,11 @@ func (i *pointSynthesizingIter) updateRangeKeys() { // extendRangeKeysEnd extends i.rangeKeysEnd to the current point key's // timestamp in the underlying iterator. It never reduces i.rangeKeysEnd. func (i *pointSynthesizingIter) extendRangeKeysEnd() { - if i.iterValid { - if hasPoint, _ := i.iter.HasPointAndRange(); hasPoint { - p := i.iter.UnsafeKey() - if (i.prefix || p.Key.Equal(i.rangeKeysPos)) && !p.Timestamp.IsEmpty() { - if end := sort.Search(len(i.rangeKeys), func(idx int) bool { - return i.rangeKeys[idx].Timestamp.Less(p.Timestamp) - }); end > i.rangeKeysEnd { - i.rangeKeysEnd = end - } - } + if i.iterHasPoint && i.iterAtRangeKeysPos && !i.iterKey.Timestamp.IsEmpty() { + if end := sort.Search(len(i.rangeKeys), func(idx int) bool { + return i.rangeKeys[idx].Timestamp.Less(i.iterKey.Timestamp) + }); end > i.rangeKeysEnd { + i.rangeKeysEnd = end } } } @@ -248,20 +253,18 @@ func (i *pointSynthesizingIter) extendRangeKeysEnd() { // iterator is positioned on the real point key in the underlying iterator. // Requires i.rangeKeys to have been positioned first. func (i *pointSynthesizingIter) updateAtPoint() { - if !i.iterValid { - i.atPoint = false - } else if hasPoint, _ := i.iter.HasPointAndRange(); !hasPoint { + if !i.iterHasPoint { i.atPoint = false } else if len(i.rangeKeys) == 0 { i.atPoint = true - } else if point := i.iter.UnsafeKey(); !i.prefix && !point.Key.Equal(i.rangeKeysPos) { + } else if !i.iterAtRangeKeysPos { i.atPoint = false } else if !i.reverse { - i.atPoint = i.rangeKeysIdx >= i.rangeKeysEnd || !point.Timestamp.IsSet() || - i.rangeKeys[i.rangeKeysIdx].Timestamp.LessEq(point.Timestamp) + i.atPoint = i.rangeKeysIdx >= i.rangeKeysEnd || !i.iterKey.Timestamp.IsSet() || + i.rangeKeys[i.rangeKeysIdx].Timestamp.LessEq(i.iterKey.Timestamp) } else { - i.atPoint = i.rangeKeysIdx < 0 || (point.Timestamp.IsSet() && - point.Timestamp.LessEq(i.rangeKeys[i.rangeKeysIdx].Timestamp)) + i.atPoint = i.rangeKeysIdx < 0 || (i.iterKey.Timestamp.IsSet() && + i.iterKey.Timestamp.LessEq(i.rangeKeys[i.rangeKeysIdx].Timestamp)) } } @@ -269,13 +272,9 @@ func (i *pointSynthesizingIter) updateAtPoint() { // underlying iterator. This may step the underlying iterator to position it // correctly relative to bare range keys. func (i *pointSynthesizingIter) updatePosition() { - if !i.iterValid { - i.atPoint = false - i.clearRangeKeys() - - } else if hasPoint, hasRange := i.iter.HasPointAndRange(); !hasRange { + if !i.iterHasRange { // Fast path: no range keys, so just clear range keys and bail out. - i.atPoint = hasPoint + i.atPoint = i.iterHasPoint i.clearRangeKeys() } else if !i.reverse { @@ -284,7 +283,7 @@ func (i *pointSynthesizingIter) updatePosition() { // The next position may be a point key with the same key as the current // range key, which must be interleaved with the synthetic points. i.updateRangeKeys() - if hasRange && !hasPoint { + if i.iterHasRange && !i.iterHasPoint { if _, err := i.iterNext(); err != nil { return } @@ -294,9 +293,9 @@ func (i *pointSynthesizingIter) updatePosition() { } else { // If we're on a bare range key in the reverse direction, and we've already - // emitted synthetic points for this key (as evidenced by rangeKeysPos), + // emitted synthetic points for this key (as given by iterAtRangeKeysPos), // then we skip over the bare range key to avoid duplicates. - if hasRange && !hasPoint && i.iter.UnsafeKey().Key.Equal(i.rangeKeysPos) { + if i.iterHasRange && !i.iterHasPoint && i.iterAtRangeKeysPos { if _, err := i.iterPrev(); err != nil { return } @@ -326,7 +325,7 @@ func (i *pointSynthesizingIter) clearRangeKeys() { func (i *pointSynthesizingIter) SeekGE(seekKey MVCCKey) { i.reverse = false i.iter.SeekGE(seekKey) - if ok, _ := i.updateValid(); !ok { + if ok, _ := i.updateIter(); !ok { i.updatePosition() return } @@ -337,7 +336,7 @@ func (i *pointSynthesizingIter) SeekGE(seekKey MVCCKey) { func (i *pointSynthesizingIter) SeekIntentGE(seekKey roachpb.Key, txnUUID uuid.UUID) { i.reverse = false i.iter.SeekIntentGE(seekKey, txnUUID) - if ok, _ := i.updateValid(); !ok { + if ok, _ := i.updateIter(); !ok { i.updatePosition() return } @@ -349,9 +348,8 @@ func (i *pointSynthesizingIter) SeekIntentGE(seekKey roachpb.Key, txnUUID uuid.U func (i *pointSynthesizingIter) updateSeekGEPosition(seekKey MVCCKey) { // Fast path: no range key, so just reset the iterator and bail out. - hasPoint, hasRange := i.iter.HasPointAndRange() - if !hasRange { - i.atPoint = hasPoint + if !i.iterHasRange { + i.atPoint = i.iterHasPoint i.clearRangeKeys() return } @@ -359,20 +357,19 @@ func (i *pointSynthesizingIter) updateSeekGEPosition(seekKey MVCCKey) { // If we land in the middle of a bare range key then skip over it to the next // point/range key. If prefix is enabled, we must be at its start key, so we // can omit the comparison. - if hasRange && !hasPoint && !i.prefix && - !i.iter.RangeBounds().Key.Equal(i.iter.UnsafeKey().Key) { + if i.iterHasRange && !i.iterHasPoint && !i.prefix && + !i.iter.RangeBounds().Key.Equal(i.iterKey.Key) { if ok, _ := i.iterNext(); !ok { i.updatePosition() return } - hasPoint, hasRange = i.iter.HasPointAndRange() } i.updateRangeKeys() // If we're still at a bare range key, we must be at its start key. Move the // iterator ahead to look for a point key at the same key. - if hasRange && !hasPoint { + if i.iterHasRange && !i.iterHasPoint { if _, err := i.iterNext(); err != nil { return } @@ -448,9 +445,9 @@ func (i *pointSynthesizingIter) NextKey() { } } // Don't call NextKey() if the underlying iterator is already on the next key. - if i.atPoint || i.prefix || i.rangeKeysPos.Equal(i.iter.UnsafeKey().Key) { + if i.atPoint || i.iterAtRangeKeysPos { i.iter.NextKey() - if _, err := i.updateValid(); err != nil { + if _, err := i.updateIter(); err != nil { return } } @@ -461,15 +458,14 @@ func (i *pointSynthesizingIter) NextKey() { func (i *pointSynthesizingIter) SeekLT(seekKey MVCCKey) { i.reverse = true i.iter.SeekLT(seekKey) - if ok, _ := i.updateValid(); !ok { + if ok, _ := i.updateIter(); !ok { i.updatePosition() return } // Fast path: no range key, so just reset the iterator and bail out. - hasPoint, hasRange := i.iter.HasPointAndRange() - if !hasRange { - i.atPoint = hasPoint + if !i.iterHasRange { + i.atPoint = i.iterHasPoint i.clearRangeKeys() return } @@ -482,14 +478,13 @@ func (i *pointSynthesizingIter) SeekLT(seekKey MVCCKey) { // TODO(erikgrinaker): It might be faster to do an unversioned seek from the // next key first to look for points. var positioned bool - if seekKey.Timestamp.IsSet() && hasRange && seekKey.Key.Compare(i.iter.RangeBounds().EndKey) < 0 { + if seekKey.Timestamp.IsSet() && i.iterHasRange && + seekKey.Key.Compare(i.iter.RangeBounds().EndKey) < 0 { if ok, err := i.iterNext(); err != nil { return - } else if ok { - if hasP, _ := i.iter.HasPointAndRange(); hasP && i.iter.UnsafeKey().Key.Equal(seekKey.Key) { - i.updateRangeKeys() - positioned = true - } + } else if ok && i.iterHasPoint && i.iterKey.Key.Equal(seekKey.Key) { + i.updateRangeKeys() + positioned = true } if ok, _ := i.iterPrev(); !ok { i.updatePosition() @@ -548,7 +543,7 @@ func (i *pointSynthesizingIter) Prev() { // If we've exhausted the current range keys, and we're not positioned on a // point key at the current range key position, then update with the // underlying iter position (which must be before the current key). - if i.rangeKeysIdx < 0 && (!i.atPoint || !i.rangeKeysPos.Equal(i.iter.UnsafeKey().Key)) { + if i.rangeKeysIdx < 0 && (!i.atPoint || !i.iterAtRangeKeysPos) { i.updatePosition() } } @@ -568,13 +563,13 @@ func (i *pointSynthesizingIter) Valid() (bool, error) { // Key implements MVCCIterator. func (i *pointSynthesizingIter) Key() MVCCKey { - return i.UnsafeKey().Clone() + return i.iterKey.Clone() } // UnsafeKey implements MVCCIterator. func (i *pointSynthesizingIter) UnsafeKey() MVCCKey { if i.atPoint { - return i.iter.UnsafeKey() + return i.iterKey } if i.rangeKeysIdx >= i.rangeKeysEnd || i.rangeKeysIdx < 0 { return MVCCKey{} @@ -679,6 +674,42 @@ func (i *pointSynthesizingIter) assertInvariants() error { return nil } + // Make sure the state of the underlying iterator matches the memoized state. + if ok, err := i.iter.Valid(); ok { + if !i.iterValid { + return errors.AssertionFailedf("iterValid %t != iter.Valid %t", i.iterValid, ok) + } + if i.iterErr != nil { + return errors.NewAssertionErrorWithWrappedErrf(i.iterErr, "valid iter with error") + } + if key := i.iter.UnsafeKey(); !i.iterKey.Equal(key) { + return errors.AssertionFailedf("iterKey %s != iter.UnsafeKey %s", i.iterKey, key) + } + if hasP, hasR := i.iter.HasPointAndRange(); hasP != i.iterHasPoint || hasR != i.iterHasRange { + return errors.AssertionFailedf("iterHasPoint %t, iterHasRange %t != iter.HasPointAndRange %t %t", + i.iterHasPoint, i.iterHasRange, hasP, hasR) + } + if i.iterAtRangeKeysPos && !i.iterKey.Key.Equal(i.rangeKeysPos) { + return errors.AssertionFailedf("iterAtRangeKeysPos true but iterKey %s != rangeKeysPos %s", + i.iterKey, i.rangeKeysPos) + } + } else { + if hasErr, iterHasErr := err != nil, i.iterErr != nil; hasErr != iterHasErr { + return errors.AssertionFailedf("i.iterErr %t != iter.Valid err %t", iterHasErr, hasErr) + } + if i.iterValid { + return errors.AssertionFailedf("invalid iterator with iterValid %t", i.iterValid) + } + if i.iterHasPoint || i.iterHasRange { + return errors.AssertionFailedf("invalid iterator with iterHasPoint %t, iterHasRange %t", + i.iterHasPoint, i.iterHasRange) + } + if i.iterAtRangeKeysPos { + return errors.AssertionFailedf("invalid iterator with iterAtRangeKeysPos %t", + i.iterAtRangeKeysPos) + } + } + // In prefix mode, the iterator must never be used in reverse. if i.prefix && i.reverse { return errors.AssertionFailedf("prefix iterator used in reverse") From 41f7c3c51095811fb7c0bc0086d6dcfed009a55d Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 14 Aug 2022 16:06:29 +0000 Subject: [PATCH 4/5] storage: omit `pointSynthesizingIter` start key comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch adds a field `rangeKeyStartPassed` which is set to `true` when the iterator moves past the start key of a range key in the forward direction. This allows omitting a key comparison for subsequent point keys until encountering a new range key. ``` name old time/op new time/op delta MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=0-24 5.57µs ± 1% 5.58µs ± 2% ~ (p=0.841 n=5+5) MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=1-24 11.8µs ± 1% 11.9µs ± 0% +0.71% (p=0.016 n=5+5) MVCCScan_Pebble/rows=1/versions=1/valueSize=64/numRangeKeys=100-24 164µs ± 1% 163µs ± 1% ~ (p=0.548 n=5+5) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=0-24 38.9µs ± 1% 38.8µs ± 1% ~ (p=0.841 n=5+5) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=1-24 61.7µs ± 1% 59.5µs ± 0% -3.65% (p=0.008 n=5+5) MVCCScan_Pebble/rows=100/versions=1/valueSize=64/numRangeKeys=100-24 170µs ± 1% 165µs ± 1% -3.07% (p=0.008 n=5+5) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=0-24 2.89ms ± 1% 2.82ms ± 0% -2.29% (p=0.008 n=5+5) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=1-24 4.37ms ± 0% 4.28ms ± 0% -1.90% (p=0.008 n=5+5) MVCCScan_Pebble/rows=10000/versions=1/valueSize=64/numRangeKeys=100-24 5.38ms ± 1% 5.40ms ± 1% ~ (p=0.690 n=5+5) ``` Release note: None --- pkg/storage/point_synthesizing_iter.go | 36 +++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/pkg/storage/point_synthesizing_iter.go b/pkg/storage/point_synthesizing_iter.go index 43e915dcb1e4..f6b6a9f2a8a9 100644 --- a/pkg/storage/point_synthesizing_iter.go +++ b/pkg/storage/point_synthesizing_iter.go @@ -90,6 +90,11 @@ type pointSynthesizingIter struct { // avoid cloning the EndKey. rangeKeysStart roachpb.Key + // rangeKeysStartPassed is true if the iterator has moved past the start bound + // of the current range key. This allows omitting a key comparison every time + // the iterator moves to a new key, but only in the forward direction. + rangeKeysStartPassed bool + // atPoint is true if the synthesizing iterator is positioned on a real point // key in the underlying iterator. See struct comment for details. atPoint bool @@ -220,12 +225,23 @@ func (i *pointSynthesizingIter) updateRangeKeys() { i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...) rangeKeys.Versions.CloneInto(&i.rangeKeysBuf) i.rangeKeys = i.rangeKeysBuf + i.rangeKeysStartPassed = false // we'll compare below } - if i.rangeKeysPos.Equal(i.rangeKeysStart) { + + // In the forward direction, once we step off rangeKeysStart we won't see + // it again until we hit the next range key, so we can omit a key + // comparison for all point keys in between. This isn't true in reverse, + // unfortunately, where we encounter the start key at the end. + if i.rangeKeysStartPassed && !i.reverse { + i.rangeKeysEnd = 0 + i.extendRangeKeysEnd() + } else if i.rangeKeysPos.Equal(i.rangeKeysStart) { i.rangeKeysEnd = len(i.rangeKeys) + i.rangeKeysStartPassed = false } else { i.rangeKeysEnd = 0 i.extendRangeKeysEnd() + i.rangeKeysStartPassed = !i.reverse } } i.rangeKeyChanged = false @@ -314,6 +330,7 @@ func (i *pointSynthesizingIter) clearRangeKeys() { i.rangeKeysStart = i.rangeKeysStart[:0] } i.rangeKeysEnd = 0 + i.rangeKeysStartPassed = false if !i.reverse { i.rangeKeysIdx = 0 } else { @@ -753,6 +770,9 @@ func (i *pointSynthesizingIter) assertInvariants() error { if len(i.rangeKeysStart) > 0 { return errors.AssertionFailedf("no rangeKeys but rangeKeysStart %s", i.rangeKeysStart) } + if i.rangeKeysStartPassed { + return errors.AssertionFailedf("no rangeKeys but rangeKeysStartPassed") + } return nil } @@ -777,6 +797,20 @@ func (i *pointSynthesizingIter) assertInvariants() error { } } + // rangeKeysStartPassed must match rangeKeysPos and rangeKeysStart. Note that + // it is not always correctly enabled when switching directions on a point key + // covered by a range key after rangeKeysStart, since we only update it when + // stepping onto a different key. + if i.rangeKeysStartPassed { + if i.prefix { + return errors.AssertionFailedf("rangeKeysStartPassed seen for prefix iterator") + } + if i.rangeKeysStartPassed && i.rangeKeysPos.Equal(i.rangeKeysStart) { + return errors.AssertionFailedf("rangeKeysStartPassed %t, but pos %s and start %s ", + i.rangeKeysStartPassed, i.rangeKeysPos, i.rangeKeysStart) + } + } + // rangeKeysIdx must be valid if we're not on a point. if !i.atPoint && (i.rangeKeysIdx < 0 || i.rangeKeysIdx >= i.rangeKeysEnd) { return errors.AssertionFailedf("not atPoint with invalid rangeKeysIdx %d at %s", From 89fb53a6eed71f33c92363346ec28bc4037a5fd1 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 14 Aug 2022 17:23:11 +0000 Subject: [PATCH 5/5] storage: clean up `pointSynthesizingIter` This patch makes some minor cleanups of `pointSynthesizingIter`. There are no visible behavioral changes. Release note: None --- pkg/storage/point_synthesizing_iter.go | 191 +++++++++++++------------ 1 file changed, 100 insertions(+), 91 deletions(-) diff --git a/pkg/storage/point_synthesizing_iter.go b/pkg/storage/point_synthesizing_iter.go index f6b6a9f2a8a9..e1adf7bafc9c 100644 --- a/pkg/storage/point_synthesizing_iter.go +++ b/pkg/storage/point_synthesizing_iter.go @@ -39,9 +39,8 @@ var pointSynthesizingIterPool = sync.Pool{ // are kept in rangeKeys. When atPoint is true, the iterator is positioned on a // real point key in the underlying iterator. Otherwise, it is positioned on a // synthetic point key given by rangeKeysPos and rangeKeys[rangeKeysIdx]. -// // rangeKeysEnd specifies where to end point synthesis at the current position, -// e.g. the first range key below an existing point key. +// i.e. the first range key below an existing point key. // // The relative positioning of pointSynthesizingIter and the underlying iterator // is as follows in the forward direction: @@ -62,29 +61,34 @@ var pointSynthesizingIterPool = sync.Pool{ // // See also assertInvariants() which asserts positioning invariants. type pointSynthesizingIter struct { + // iter is the underlying MVCC iterator. iter MVCCIterator + // prefix indicates that the underlying iterator is a prefix iterator, which + // can only be on a single key. This allows omitting cloning and comparison. + prefix bool + + // reverse is true when the current iterator direction is in reverse, i.e. + // following a SeekLT or Prev call. + reverse bool + // rangeKeys contains any range key versions that overlap the current key // position, for which points will be synthesized. rangeKeys MVCCRangeKeyVersions // rangeKeysBuf is a reusable buffer for rangeKeys. Non-prefix iterators use - // it as a MVCCRangeKeyVersions.CloneInto() target and set rangeKeys to it. + // it as a CloneInto() target and place a reference in rangeKeys. rangeKeysBuf MVCCRangeKeyVersions - // rangeKeysPos is the current key (along the rangeKeys span) that points will - // be synthesized for. It is only set if rangeKeys is non-empty, and may - // differ from the underlying iterator position. + // rangeKeysPos is the current key position along the rangeKeys span, where + // points will be synthesized. It is only set if rangeKeys is non-empty, and + // may differ from the underlying iterator position. rangeKeysPos roachpb.Key // rangeKeysIdx is the rangeKeys index of the current/pending range key // to synthesize a point for. See struct comment for details. rangeKeysIdx int - // rangeKeysEnd contains the exclusive index at which to stop synthesizing - // point keys, since points are not synthesized below existing point keys. - rangeKeysEnd int - // rangeKeysStart contains the start key of the current rangeKeys stack. It is // stored separately, rather than holding the entire MVCCRangeKeyStack, to // avoid cloning the EndKey. @@ -95,27 +99,24 @@ type pointSynthesizingIter struct { // the iterator moves to a new key, but only in the forward direction. rangeKeysStartPassed bool + // rangeKeysEnd contains the exclusive index at which to stop synthesizing + // point keys, since points are not synthesized below existing point keys. + rangeKeysEnd int + // atPoint is true if the synthesizing iterator is positioned on a real point // key in the underlying iterator. See struct comment for details. atPoint bool - // If prefix is true, then the underlying iterator is a prefix iterator which - // can only be on a single key position. This allows omitting key cloning and - // comparisons in many cases. - prefix bool - - // reverse is true when the current iterator direction is in reverse, i.e. - // following a SeekLT or Prev call. - reverse bool + // atRangeKeysPos is true if the underlying iterator is at rangeKeysPos. + atRangeKeysPos bool // The following fields memoize the state of the underlying iterator, and are // updated every time its position changes. - iterValid bool - iterErr error - iterKey MVCCKey - iterHasPoint bool - iterHasRange bool - iterAtRangeKeysPos bool + iterValid bool + iterErr error + iterKey MVCCKey + iterHasPoint bool + iterHasRange bool // rangeKeyChanged keeps track of changes to the underlying iter's range keys. // It is reset to false on every call to updateRangeKeys(), and accumulates @@ -123,7 +124,7 @@ type pointSynthesizingIter struct { rangeKeyChanged bool } -var _ MVCCIterator = new(pointSynthesizingIter) +var _ MVCCIterator = (*pointSynthesizingIter)(nil) // newPointSynthesizingIter creates a new pointSynthesizingIter, or gets one // from the pool. @@ -186,81 +187,90 @@ func (i *pointSynthesizingIter) iterPrev() (bool, error) { } // updateIter memoizes the iterator fields from the underlying iterator, and -// also keeps track of rangeKeyChanged. It returns the iterator validity and -// error. +// also keeps track of rangeKeyChanged. It must be called after every iterator +// positioning operation, and returns the iterator validity/error. func (i *pointSynthesizingIter) updateIter() (bool, error) { if i.iterValid, i.iterErr = i.iter.Valid(); i.iterValid { i.iterHasPoint, i.iterHasRange = i.iter.HasPointAndRange() i.iterKey = i.iter.UnsafeKey() - i.iterAtRangeKeysPos = (i.prefix && i.iterHasRange) || + i.atRangeKeysPos = (i.prefix && i.iterHasRange) || (!i.prefix && i.iterKey.Key.Equal(i.rangeKeysPos)) } else { i.iterHasPoint, i.iterHasRange = false, false i.iterKey = MVCCKey{} - i.iterAtRangeKeysPos = false + i.atRangeKeysPos = false } i.rangeKeyChanged = i.rangeKeyChanged || i.iter.RangeKeyChanged() return i.iterValid, i.iterErr } -// updateRangeKeys updates i.rangeKeys and related fields with range keys from -// the underlying iterator. rangeKeysIdx is reset to the first/last range key. +// updateRangeKeys updates i.rangeKeys and related fields with the underlying +// iterator state. It must be called very time the pointSynthesizingIter moves +// to a new key position, i.e. after exhausting all point/range keys at the +// current position. rangeKeysIdx and rangeKeysEnd are reset. func (i *pointSynthesizingIter) updateRangeKeys() { if !i.iterHasRange { i.clearRangeKeys() - } else { - i.rangeKeysPos = append(i.rangeKeysPos[:0], i.iterKey.Key...) - i.iterAtRangeKeysPos = true // by definition - if i.prefix { - if i.rangeKeyChanged { - // A prefix iterator will always be at the start bound of the range key, - // and never move onto a different range key, so we can omit the cloning - // and other processing. - i.rangeKeys = i.iter.RangeKeys().Versions - i.rangeKeysEnd = len(i.rangeKeys) - } - } else { - if i.rangeKeyChanged { - rangeKeys := i.iter.RangeKeys() - i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...) - rangeKeys.Versions.CloneInto(&i.rangeKeysBuf) - i.rangeKeys = i.rangeKeysBuf - i.rangeKeysStartPassed = false // we'll compare below - } + return + } - // In the forward direction, once we step off rangeKeysStart we won't see - // it again until we hit the next range key, so we can omit a key - // comparison for all point keys in between. This isn't true in reverse, - // unfortunately, where we encounter the start key at the end. - if i.rangeKeysStartPassed && !i.reverse { - i.rangeKeysEnd = 0 - i.extendRangeKeysEnd() - } else if i.rangeKeysPos.Equal(i.rangeKeysStart) { - i.rangeKeysEnd = len(i.rangeKeys) - i.rangeKeysStartPassed = false - } else { - i.rangeKeysEnd = 0 - i.extendRangeKeysEnd() - i.rangeKeysStartPassed = !i.reverse - } - } + // Detect and fetch new range keys in the underlying iterator since the + // last call to updateRangeKeys(). + if i.rangeKeyChanged { i.rangeKeyChanged = false - if !i.reverse { - i.rangeKeysIdx = 0 + if i.prefix { + // A prefix iterator will always be at the start bound of the range key, + // and never move onto a different range key, so we can omit the cloning. + i.rangeKeys = i.iter.RangeKeys().Versions } else { - i.rangeKeysIdx = i.rangeKeysEnd - 1 // NB: -1 is correct with no range keys + rangeKeys := i.iter.RangeKeys() + i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...) + rangeKeys.Versions.CloneInto(&i.rangeKeysBuf) + i.rangeKeys = i.rangeKeysBuf + i.rangeKeysStartPassed = false // we'll compare below } } + + // Update the current iterator position. + i.rangeKeysPos = append(i.rangeKeysPos[:0], i.iterKey.Key...) + i.atRangeKeysPos = true // by definition + + // Update the rangeKeysEnd with the range keys to synthesize points for at + // this position. Notably, we synthesize for all range keys at their start + // bound, but otherwise only the ones above existing point keys. + // + // In the forward direction, once we step off rangeKeysStart we won't see a + // start bound again until we hit a new range key, so we can omit a key + // comparison for all point keys in between. This isn't true in reverse, + // unfortunately, where we only encounter the start key at the end. + if i.rangeKeysStartPassed && !i.reverse { + i.rangeKeysEnd = 0 + i.extendRangeKeysEnd() + } else if i.prefix || i.rangeKeysPos.Equal(i.rangeKeysStart) { + i.rangeKeysEnd = len(i.rangeKeys) + i.rangeKeysStartPassed = false + } else { + i.rangeKeysEnd = 0 + i.extendRangeKeysEnd() + i.rangeKeysStartPassed = !i.reverse + } + + // Reset rangeKeysIdx to the first range key. + if !i.reverse { + i.rangeKeysIdx = 0 + } else { + i.rangeKeysIdx = i.rangeKeysEnd - 1 // NB: -1 is correct with no range keys + } } -// extendRangeKeysEnd extends i.rangeKeysEnd to the current point key's +// extendRangeKeysEnd extends i.rangeKeysEnd below the current point key's // timestamp in the underlying iterator. It never reduces i.rangeKeysEnd. func (i *pointSynthesizingIter) extendRangeKeysEnd() { - if i.iterHasPoint && i.iterAtRangeKeysPos && !i.iterKey.Timestamp.IsEmpty() { - if end := sort.Search(len(i.rangeKeys), func(idx int) bool { - return i.rangeKeys[idx].Timestamp.Less(i.iterKey.Timestamp) - }); end > i.rangeKeysEnd { - i.rangeKeysEnd = end + if i.iterHasPoint && i.atRangeKeysPos && !i.iterKey.Timestamp.IsEmpty() { + if l := len(i.rangeKeys); i.rangeKeysEnd < l { + i.rangeKeysEnd = sort.Search(l-i.rangeKeysEnd, func(idx int) bool { + return i.rangeKeys[i.rangeKeysEnd+idx].Timestamp.Less(i.iterKey.Timestamp) + }) + i.rangeKeysEnd } } } @@ -273,7 +283,7 @@ func (i *pointSynthesizingIter) updateAtPoint() { i.atPoint = false } else if len(i.rangeKeys) == 0 { i.atPoint = true - } else if !i.iterAtRangeKeysPos { + } else if !i.atRangeKeysPos { i.atPoint = false } else if !i.reverse { i.atPoint = i.rangeKeysIdx >= i.rangeKeysEnd || !i.iterKey.Timestamp.IsSet() || @@ -284,7 +294,7 @@ func (i *pointSynthesizingIter) updateAtPoint() { } } -// updatePosition updates the synthesizing iterator with the position of the +// updatePosition updates the synthesizing iterator for the position of the // underlying iterator. This may step the underlying iterator to position it // correctly relative to bare range keys. func (i *pointSynthesizingIter) updatePosition() { @@ -309,9 +319,9 @@ func (i *pointSynthesizingIter) updatePosition() { } else { // If we're on a bare range key in the reverse direction, and we've already - // emitted synthetic points for this key (as given by iterAtRangeKeysPos), - // then we skip over the bare range key to avoid duplicates. - if i.iterHasRange && !i.iterHasPoint && i.iterAtRangeKeysPos { + // emitted synthetic points for this key (given by atRangeKeysPos), then we + // skip over the bare range key to avoid duplicates. + if i.iterHasRange && !i.iterHasPoint && i.atRangeKeysPos { if _, err := i.iterPrev(); err != nil { return } @@ -328,9 +338,9 @@ func (i *pointSynthesizingIter) clearRangeKeys() { i.rangeKeys = i.rangeKeys[:0] i.rangeKeysPos = i.rangeKeysPos[:0] i.rangeKeysStart = i.rangeKeysStart[:0] + i.rangeKeysEnd = 0 + i.rangeKeysStartPassed = false } - i.rangeKeysEnd = 0 - i.rangeKeysStartPassed = false if !i.reverse { i.rangeKeysIdx = 0 } else { @@ -390,7 +400,6 @@ func (i *pointSynthesizingIter) updateSeekGEPosition(seekKey MVCCKey) { if _, err := i.iterNext(); err != nil { return } - i.extendRangeKeysEnd() } // If we're seeking to a specific version, skip newer range keys. @@ -462,7 +471,7 @@ func (i *pointSynthesizingIter) NextKey() { } } // Don't call NextKey() if the underlying iterator is already on the next key. - if i.atPoint || i.iterAtRangeKeysPos { + if i.atPoint || i.atRangeKeysPos { i.iter.NextKey() if _, err := i.updateIter(); err != nil { return @@ -560,7 +569,7 @@ func (i *pointSynthesizingIter) Prev() { // If we've exhausted the current range keys, and we're not positioned on a // point key at the current range key position, then update with the // underlying iter position (which must be before the current key). - if i.rangeKeysIdx < 0 && (!i.atPoint || !i.iterAtRangeKeysPos) { + if i.rangeKeysIdx < 0 && (!i.atPoint || !i.atRangeKeysPos) { i.updatePosition() } } @@ -703,11 +712,12 @@ func (i *pointSynthesizingIter) assertInvariants() error { return errors.AssertionFailedf("iterKey %s != iter.UnsafeKey %s", i.iterKey, key) } if hasP, hasR := i.iter.HasPointAndRange(); hasP != i.iterHasPoint || hasR != i.iterHasRange { - return errors.AssertionFailedf("iterHasPoint %t, iterHasRange %t != iter.HasPointAndRange %t %t", + return errors.AssertionFailedf( + "iterHasPoint %t, iterHasRange %t != iter.HasPointAndRange %t %t", i.iterHasPoint, i.iterHasRange, hasP, hasR) } - if i.iterAtRangeKeysPos && !i.iterKey.Key.Equal(i.rangeKeysPos) { - return errors.AssertionFailedf("iterAtRangeKeysPos true but iterKey %s != rangeKeysPos %s", + if i.atRangeKeysPos && !i.iterKey.Key.Equal(i.rangeKeysPos) { + return errors.AssertionFailedf("atRangeKeysPos true but iterKey %s != rangeKeysPos %s", i.iterKey, i.rangeKeysPos) } } else { @@ -721,9 +731,8 @@ func (i *pointSynthesizingIter) assertInvariants() error { return errors.AssertionFailedf("invalid iterator with iterHasPoint %t, iterHasRange %t", i.iterHasPoint, i.iterHasRange) } - if i.iterAtRangeKeysPos { - return errors.AssertionFailedf("invalid iterator with iterAtRangeKeysPos %t", - i.iterAtRangeKeysPos) + if i.atRangeKeysPos { + return errors.AssertionFailedf("invalid iterator with atRangeKeysPos %t", i.atRangeKeysPos) } }