storage: export OptionalValue

[jbowens]

This commit exports the OptionalValue type used for representing a MVCCValue that may not exist. Parts of the storage package interface are updated to prefer the OptionalValue over returning a pointer.

Callers that only check presence via Exists() or IsPresent(), or read the value inline, avoid the heap allocation entirely. Callers that need a pointer (e.g., for proto response fields) can call ToPointer() explicitly, deferring the allocation to the specific call sites that require it.

goos: darwin
goarch: arm64
cpu: Apple M1 Pro
                                                          │   old.txt   │              new.txt               │
                                                          │   sec/op    │   sec/op     vs base               │
MVCCGet/batch=false/versions=1/valueSize=8/numRangeKeys=0   2.783µ ± 6%   2.534µ ± 4%  -8.95% (p=0.000 n=10)

                                                          │   old.txt    │               new.txt               │
                                                          │     B/s      │     B/s       vs base               │
MVCCGet/batch=false/versions=1/valueSize=8/numRangeKeys=0   2.742Mi ± 6%   3.009Mi ± 4%  +9.74% (p=0.000 n=10)

                                                          │   old.txt   │              new.txt               │
                                                          │    B/op     │    B/op     vs base                │
MVCCGet/batch=false/versions=1/valueSize=8/numRangeKeys=0   114.00 ± 1%   66.00 ± 2%  -42.11% (p=0.000 n=10)

                                                          │  old.txt   │              new.txt               │
                                                          │ allocs/op  │ allocs/op   vs base                │
MVCCGet/batch=false/versions=1/valueSize=8/numRangeKeys=0   2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.000 n=10)

Epic: None
Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

@RaduBerinde made 2 comments.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola).

---

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

// MVCCValueHeader. It is preferred over *roachpb.Value to avoid forced heap
// allocation.
type OptionalValue struct {

Is a zero MVCCValue possible? Even for tombstones we would have a non-zero Timestamp, no?

[sumeerbhola]

nice find!

@sumeerbhola reviewed 19 files and all commit messages, and made 3 comments.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @jbowens and @RaduBerinde).

---

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

Previously, RaduBerinde wrote…

Is a zero MVCCValue possible? Even for tombstones we would have a non-zero Timestamp, no?

It's never been clear to me which paths populate the MVCCValue.Value.Timestamp. AFAIK, we don't encode the timestamp in the value.

Which is why methods like DecodeMVCCValue don't populate it. For tombstones, that decode method can return a zero MVCCValue.

We seem to selectively populate the Timestamp, by extracting it from the key, when plumbing information through layers.

And there are of course values that are not MVCC with zero timestamps. The comment for MVCCValue says it is not for them:

// MVCCValue is a versioned value, stored at an associated MVCCKey with a
// non-zero version timestamp.

That should imply that optionalValue is not for them either, but we use mvccGet and mvccPut for values with no timestamps, so the timestamp will be zero for them even when "populated". I think the code comment is not quite accurate.

---

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

// OptionalValue represents an optional roachpb.Value with its associated
// MVCCValueHeader. It is preferred over *roachpb.Value to avoid forced heap
// allocation.

Is it fair to say that OptionalValue is certain that the memory backing its internals cannot disappear from under it, e.g. for MVCCValue.Value.RawBytes? Which is why delaying the conversion from OptionalValue to a *roachpb.Value by delaying the call to ToPointer is safe?

For the first part above, I am wondering if we can say something in a code comment. Something like: the memory backing OptionalValue is expected to stay valid and immutable, and it is safe to copy around OptionalValue under that assumption.