New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove a lot of bounds checks in BinDecoder by tracking position with a second slice #1399
Conversation
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 |
a1f94c2
to
1ebccb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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
BinDecoder
incurred 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_u32
andread_i32
to reduce the number of bounds checks in those.This change to the representation of
BinDecoder
produces 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 manyBinDecoder
methods enough that LLVM decides to inline them where it didn't before.