storage: avoid EncodeMVCCValue struct comparison#88989
Conversation
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) ``` Release note: None
|
This change is |
|
Any bright ideas on how we can retain the mid-stack inlining here? |
jbowens
left a comment
There was a problem hiding this comment.
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)
nvb
left a comment
There was a problem hiding this comment.
This 
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:
|
Build failed (retrying...): |
|
Build succeeded: |
In Go 1.19, struct comparisons saw a significant performance regression, apparently due to the use of
memeqbody. This patch changesEncodeMVCCValueto 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.
Resolves #88818.
Release note: None