storage: avoid EncodeMVCCValue struct comparison

[erikgrinaker]

In Go 1.19, struct comparisons saw a significant performance regression, apparently due to the use of memeqbody. This patch changes EncodeMVCCValue to use field comparisons instead of struct comparisons, which yields a significant performance gain.

Unfortunately, this prevents mid-stack inlining. However, the struct comparison regression is significantly larger than the inlining gains. We should reconsider this once Go 1.20 lands, where the regression has been fixed.

name                                                              old time/op  new time/op  delta
EncodeMVCCValue/header=empty/value=tombstone-24                   5.96ns ± 1%  4.66ns ± 0%  -21.85%  (p=0.000 n=9+9)
EncodeMVCCValue/header=empty/value=short-24                       5.93ns ± 0%  4.66ns ± 0%  -21.40%  (p=0.000 n=9+9)
EncodeMVCCValue/header=empty/value=long-24                        5.92ns ± 0%  4.66ns ± 0%  -21.31%  (p=0.000 n=10+10)
EncodeMVCCValue/header=local_walltime/value=tombstone-24          51.9ns ± 1%  49.5ns ± 1%   -4.81%  (p=0.000 n=9+10)
EncodeMVCCValue/header=local_walltime/value=short-24              54.2ns ± 1%  52.5ns ± 1%   -3.25%  (p=0.000 n=10+10)
EncodeMVCCValue/header=local_walltime/value=long-24               1.34µs ± 2%  1.36µs ± 1%   +1.69%  (p=0.001 n=10+9)
EncodeMVCCValue/header=local_walltime+logical/value=tombstone-24  56.3ns ± 0%  53.3ns ± 1%   -5.40%  (p=0.000 n=10+10)
EncodeMVCCValue/header=local_walltime+logical/value=short-24      58.8ns ± 0%  56.3ns ± 2%   -4.18%  (p=0.000 n=10+9)
EncodeMVCCValue/header=local_walltime+logical/value=long-24       1.36µs ± 3%  1.36µs ± 1%     ~     (p=0.269 n=10+9)

Resolves #88818.

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

Any bright ideas on how we can retain the mid-stack inlining here?

[jbowens]

I suppose we could manually inline it (the logic itself is tiny), and litter warnings to update the manually inlined code in EncodeMVCCValue in many places. I don't think it's worth it though.

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nicktrav and @nvanbenschoten)

[nvanbenschoten]

This for the time being. It is unfortunate that we need to make these kinds of changes (adding code complexity and losing valuable inlining budget). I'd feel better about merging this if we had an issue tracking Go 1.20 and suggesting that we revert this and #88911 once the regression in Go is resolved.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @nicktrav)

[erikgrinaker]

TFTRs!

bors r+

I'd feel better about merging this if we had an issue tracking Go 1.20 and suggesting that we revert this and #88911 once the regression in Go is resolved.

#89199

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)