storage: use concrete pebbleIterator in intentInterleavingIter

[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

[cockroach-teamcity]

This change is 

[erikgrinaker]

CC @nvanbenschoten, this should make up for some of the regression since 22.1.

[tbg]

Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)

[erikgrinaker]

TFTR!

bors r=tbg

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[sumeerbhola]

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

-- commits line 41 at r3:
I think we should try to address this and not take this testing loss. One possibility is to move the simple functionality provided by unsafeMVCCIterator into pebbleIterator and do it when RaceEnabled. This will be a net improvement since engine iterators will also have it.
btw, I did not understand the "omitted ... when using an engine directly" part -- Pebble.NewMVCCIterator does have the wrapping code.

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

-- commits line 41 at r3:
Wrote up #86657, will have a look during stability.

btw, I did not understand the "omitted ... when using an engine directly" part -- Pebble.NewMVCCIterator does have the wrapping code.

It does, but for readers without ConsistentIterators() we instead construct the pebbleIterator by cloning the engine iterator:

cockroach/pkg/storage/intent_interleaving_iter.go

Lines 274 to 275 in e209dc1

	iter = newPebbleIteratorByCloning(
	intentIter.GetRawIter(), opts, StandardDurability, reader.SupportsRangeKeys())