storage: batch pebbleResults allocation

[jbowens]

Embed a pebbleResults struct on the pebbleMVCCScanner to avoid an allocation.

                                                                     │  23.1.txt   │           23.1-wip.txt            │
                                                                     │   sec/op    │   sec/op     vs base              │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   109.3µ ± 1%   105.2µ ± 3%  -3.73% (p=0.009 n=6)

                                                                     │   23.1.txt   │          23.1-wip.txt          │
                                                                     │     B/s      │      B/s       vs base         │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   68.36Ki ± 0%   78.12Ki ± 12%  ~ (p=0.061 n=6)

                                                                     │  23.1.txt   │            23.1-wip.txt            │
                                                                     │    B/op     │    B/op      vs base               │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   354.0 ± 22%   228.5 ± 28%  -35.45% (p=0.002 n=6)

                                                                     │  23.1.txt  │           23.1-wip.txt            │
                                                                     │ allocs/op  │ allocs/op   vs base               │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.002 n=6)

Informs #96960.
Epic: None
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @yuzefovich)

---

pkg/storage/mvcc.go line 3713 at r1 (raw file):

		failOnMoreRecent: opts.FailOnMoreRecent,
		keyBuf:           mvccScanner.keyBuf,
		alloc:            mvccScanner.alloc,

is this alloc: mvccScanner.alloc, necessary given that we are not reusing anything contained in pebbleResults?
Or is it for a future where we may add other things to alloc that we want to reuse?

---

pkg/storage/pebble_mvcc_scanner.go line 481 at r1 (raw file):

		// Embedding the pebbleResults within the pebbleMVCCScanner avoids an
		// extra allocation, at the cost of higher allocated bytes when we use a
		// different implementation of the results interface.

We are only trying to avoid the allocation of this wrapper pebbleResults object, yes?
I assume we can't reuse the buffers that pebbleResults points to, since we are returning references to those byte slices to the callers that call into mvcc.go.
I would prefer if we cleared pebbleResults in pebbleMVCCScanner.release to avoid delaying gc of those byte slices and to ensure that there is no accidental reuse (the latter is just defensive given you already have *results = pebbleResults{} on the allocation path).

[jbowens]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola and @yuzefovich)

---

pkg/storage/mvcc.go line 3713 at r1 (raw file):

Previously, sumeerbhola wrote…

is this alloc: mvccScanner.alloc, necessary given that we are not reusing anything contained in pebbleResults?
Or is it for a future where we may add other things to alloc that we want to reuse?

it's because the caller passes in a results. If results points to pebbleMVCCScanner.alloc.pebbleResults, this struct initialization will clear any initialization of pebbleMVCCScanner.alloc.pebbleResults already performed by the caller.

---

pkg/storage/pebble_mvcc_scanner.go line 481 at r1 (raw file):

We are only trying to avoid the allocation of this wrapper pebbleResults object, yes?

Yeah

I would prefer if we cleared pebbleResults in pebbleMVCCScanner.release to avoid delaying gc of those byte slices and to ensure that there is no accidental reuse (the latter is just defensive given you already have *results = pebbleResults{} on the allocation path).

We currently do clear all of the pebbleMVCCScanner struct, including pebbleResults on release. I added a comment there specifically calling out the importance of clearing alloc.pebbleResults.

[jbowens]

bors r=sumeerbhola

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola and @yuzefovich)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[sumeerbhola]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens and @yuzefovich)

---

pkg/storage/mvcc.go line 3713 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

it's because the caller passes in a results. If results points to pebbleMVCCScanner.alloc.pebbleResults, this struct initialization will clear any initialization of pebbleMVCCScanner.alloc.pebbleResults already performed by the caller.

This is subtle. Could use a code comment.

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[jbowens]

bors cancel

[craig]

Canceled.

[jbowens]

bors r=sumeerbhola

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola and @yuzefovich)

---

pkg/storage/mvcc.go line 3713 at r1 (raw file):

Previously, sumeerbhola wrote…

This is subtle. Could use a code comment.

Done.

[yuzefovich]

Thanks!

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)