[interpreter/ruby]: Optimize ep check to simplify work for verifier, increase limit#22
Closed
dalehamel wants to merge 3 commits into
Closed
[interpreter/ruby]: Optimize ep check to simplify work for verifier, increase limit#22dalehamel wants to merge 3 commits into
dalehamel wants to merge 3 commits into
Conversation
6f98000 to
67a5788
Compare
Member
Author
|
Upstream PR opened open-telemetry#1100 |
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
67a5788 to
2462f24
Compare
Member
Author
|
merged upstream |
This file contains hidden or 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
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.
What
Optimizes the inner loop used when walking from the cfp->ep until we hit a local env, when checking for a method entry.
I also realized we had a regression on MAX_EP_CHECKS, on our branch I had raised it to 10 but in the open-telemetry#907 it was only set to 6. I addressed that issue, as we have some cases (ruby graphql libraries in rails) that seem to potentially have up to 10 levels of indirection to get to the IMEMO_MENT entry. It will now walk up to 10 environment pointers before giving up (and erroring), instead of just 6.
Why
Even though it passed the verifier in CI, when I rebased after merging and tried in production, I started having verifier errors. It works on all CI amd64 checks, works locally on aarch64 with linux 6.8, but fails with amd64 on linux 6.6.
It seems that when I switched from goto's to a simpler
UNROLLwhile addressing the feedback in open-telemetry#907 , I didn't optimally structure the loop.Without these changes, it will error1 out if there is a lot of indirection to find a local method entry, such as in ruby graphql libraries and rails with a lot of meta-programming.
How
I tried to push all code that didn't need to be in the unroll to either before or after it. The unroll will now only check:
All other probe reads and cfunc checks are moved out of the loop explicitly.
The MAX_EP_CHECKS macro is bumped to 10
Footnotes
perhaps bailing on unwinding the whole stack is too severe, we can probably unwind the other frames and if we hit max ep checks, return a dummy frame for userpace to symbolize as something like
<unknown ruby_local_ep_missing>↩