Remove a lot of bounds checks in BinDecoder by tracking position with a second slice #1399
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
Codecov Report
@@ Coverage Diff @@
## main #1399 +/- ##
=======================================
Coverage 85.16% 85.16%
=======================================
Files 153 153
Lines 15038 15035 -3
=======================================
- Hits 12806 12804 -2
+ Misses 2232 2231 -1 |
saethlin
force-pushed
the
shrink-errors
branch
3 times, most recently
from
a1f94c2
to
1ebccb9
Compare
djc
reviewed
Mar 10, 2021
There was a problem hiding this comment.
Okay, here's some preliminary questions/remarks, but I'd like to see this split into multiple commits (or PRs) that each make one logical change (separate Error type, change BinDecoder representation). Let's defer the addition of inline/cold attributes to the end and focus more on the functional changes first?
One other question I have is how much the quality of the error messages regresses once we no longer have the actual data in decode errors.
djc
reviewed
Mar 10, 2021
|
@djc Really appreciate the feedback. I'm going to reduce this to just the first commit and send the other changes in at least 2 other PRs. |
saethlin
changed the title
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.
This branch is now awkwardly-named. It used to be a lot of changes, but I'm trickling them in one PR at a time. I don't mind the name, but I'm happy to replace the PR if y'all care.
The representation of
BinDecoderincurred an extra bounds check on every operation, which was guaranteed to succeed. Using two slices (one is the original so that it can backtrack) to represent the state removes one bounds check on every access, and this also adds an assert toread_u32andread_i32to reduce the number of bounds checks in those.This change to the representation of
BinDecoderproduces a ~19% improvement in the message-parsing code. This is not so much because the bounds checks were actually that much overhead; removing the checks shrinks the code size of manyBinDecodermethods enough that LLVM decides to inline them where it didn't before.