-
Notifications
You must be signed in to change notification settings - Fork 5.5k
YJIT: Make ratio_in_yjit always available #8064
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
Conversation
72b69da
to
9771422
Compare
So the good news is it looks like you managed to make the overhead negligible, but that comes at the expense of some accuracy. Are we sure that the loss in accuracy will always be small? Looking at the PR. I think maybe things could be simplified a bit? It looks like there are a few functions exported on both sides. Ideally, maybe we should try to do as much of the vm_insn_count accounting on the C side? I'm also wondering if you tried to make it so that the C interpreter loop just does an atomic increment on a |
I assumed it wouldn't be faster than incrementing a local variable, so I didn't try it. I tried it now, and here's the result. Using |
Sounds like it could be a pretty good tradeoff 👍 🙂 |
9771422
to
6148884
Compare
I simplified the implementation 6cf33c8 and updated the benchmark results on the PR description. The current version seems much nicer 🙂 |
8d1220e
to
6cf33c8
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.
Nice! It's awesome that we can now have all the stats including percentage in YJIT on all builds! 😎
This PR changes
ratio_in_yjit
to be available on any YJIT-enabled build. To minimize overhead when--yjit
or--yjit-stats
is not used, we increment a global variable without branching or locking. The counter could be inaccurate on Ractors.Performance
interpreter
--yjit
--yjit-stats
This PR doesn't seem to have a significant performance impact on railsbench.