storage: clean up MVCC key encoding functions #75721
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
erikgrinaker
force-pushed
the
encode-decode-timestamp
branch
4 times, most recently
from
c5a0c2f
to
9035872
Compare
erikgrinaker
changed the title
jbowens
approved these changes
Jan 31, 2022
There was a problem hiding this comment.
Reviewed 4 of 6 files at r1, 18 of 19 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)
|
Nice to see this cleanup. |
erikgrinaker
force-pushed
the
encode-decode-timestamp
branch
from
9035872
to
887dddd
Compare
This splits MVCC keys and their encoding into a separate file, following the same convention as `engine_key.go`. There is additional decoding logic in `enginepb/decode.go` which has not been moved -- this was originally to avoid a RocksDB CGo dependency for these functions, but Pebble still has a (tiny) CGo dependency so we leave them for now. Release note: None
Release note: None
In particular, this separates the timestamp encoding from the overall key encoding. This is necessary for the MVCC range tombstone work, where Pebble key suffixes (timestamps) are processed in isolation. Unfortunately, this adds ~9% overhead for `EncodeMVCCKey()`. This was found to be due to additional function call overhead, and the Go compiler's unwillingness to inline these. This was considered acceptable for the encode path, while with the hotter `DecodeMVCCKey()` path the timestamp decoding logic was instead duplicated to avoid this overhead. ``` name old time/op new time/op delta EncodeMVCCKey/key=empty/ts=empty-24 15.8ns ± 0% 15.7ns ± 0% -0.46% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime-24 17.9ns ± 0% 19.5ns ± 0% +8.88% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 18.5ns ± 0% 20.1ns ± 0% +8.99% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=all-24 18.8ns ± 0% 20.4ns ± 0% +8.66% (p=0.000 n=10+10) EncodeMVCCKey/key=short/ts=walltime+logical-24 19.1ns ± 0% 20.7ns ± 0% +8.38% (p=0.000 n=10+9) EncodeMVCCKey/key=short/ts=all-24 19.5ns ± 0% 20.7ns ± 0% +6.18% (p=0.000 n=10+9) EncodeMVCCKey/key=short/ts=empty-24 16.3ns ± 0% 16.0ns ± 0% -1.86% (p=0.000 n=10+10) EncodeMVCCKey/key=short/ts=walltime-24 18.1ns ± 0% 20.6ns ± 0% +13.41% (p=0.000 n=8+8) EncodeMVCCKey/key=long/ts=empty-24 58.7ns ± 0% 58.8ns ± 0% +0.15% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=walltime-24 59.8ns ± 0% 60.8ns ± 0% +1.78% (p=0.000 n=10+9) EncodeMVCCKey/key=long/ts=walltime+logical-24 60.7ns ± 0% 61.7ns ± 0% +1.54% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=all-24 60.9ns ± 0% 61.9ns ± 0% +1.60% (p=0.000 n=10+9) DecodeMVCCKey/key=empty/ts=empty-24 12.4ns ± 0% 12.4ns ± 0% ~ (p=0.912 n=10+6) DecodeMVCCKey/key=empty/ts=walltime-24 13.3ns ± 0% 13.3ns ± 0% ~ (p=0.054 n=10+10) DecodeMVCCKey/key=empty/ts=walltime+logical-24 13.3ns ± 0% 13.3ns ± 0% -0.06% (p=0.034 n=10+10) DecodeMVCCKey/key=empty/ts=all-24 13.6ns ± 0% 13.6ns ± 0% ~ (p=0.509 n=10+10) DecodeMVCCKey/key=short/ts=walltime+logical-24 13.3ns ± 0% 13.3ns ± 0% ~ (all equal) DecodeMVCCKey/key=short/ts=all-24 13.6ns ± 0% 13.6ns ± 0% ~ (p=0.151 n=10+10) DecodeMVCCKey/key=short/ts=empty-24 12.5ns ± 0% 12.4ns ± 0% -0.21% (p=0.000 n=10+10) DecodeMVCCKey/key=short/ts=walltime-24 13.3ns ± 0% 13.3ns ± 0% ~ (p=0.577 n=8+10) DecodeMVCCKey/key=long/ts=walltime+logical-24 13.3ns ± 0% 13.3ns ± 0% ~ (all equal) DecodeMVCCKey/key=long/ts=all-24 13.6ns ± 0% 13.6ns ± 0% ~ (p=0.650 n=10+10) DecodeMVCCKey/key=long/ts=empty-24 12.4ns ± 0% 12.4ns ± 0% +0.15% (p=0.004 n=10+10) DecodeMVCCKey/key=long/ts=walltime-24 13.3ns ± 0% 13.3ns ± 0% +0.10% (p=0.012 n=10+9) ``` Release note: None
erikgrinaker
force-pushed
the
encode-decode-timestamp
branch
from
887dddd
to
a72dbea
Compare
|
TFTRs! bors r=jbowens,sumeerbhola |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
storage: move MVCC key/encoding logic into separate file
This splits MVCC keys and their encoding into a separate file, following
the same convention as
engine_key.go. There is additional decodinglogic in
enginepb/decode.gowhich has not been moved -- this wasoriginally to avoid a RocksDB CGo dependency for these functions, but
Pebble still has a (tiny) CGo dependency so we leave them for now.
Release note: None
storage: include MVCC in names of key encoding functions
Release note: None
storage: clean up MVCC key encoding functions
In particular, this separates the timestamp encoding from the overall
key encoding. This is necessary for the MVCC range tombstone work, where
Pebble key suffixes (timestamps) are processed in isolation.
Unfortunately, this adds ~9% overhead for
EncodeMVCCKey(). This wasfound to be due to additional function call overhead, and the Go
compiler's unwillingness to inline these. This was considered acceptable
for the encode path, while with the hotter
DecodeMVCCKey()path thetimestamp decoding logic was instead duplicated to avoid this overhead.
Release note: None