-
Notifications
You must be signed in to change notification settings - Fork 375
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
Support stats "ratio_in_yjit" #3991
Conversation
When ruby's extended YJIT stats are available (`--yjit-stat`), then we have access to numerous additional stats. This adds `:ratio_in_yjit`, which is only present when additional stats are enabled. Example: ``` RUBYOPT='--yjit --yjit-stats=quiet' ruby -e ' def foo = "a" * 10 300.times{foo} puts "ratio_in_yjit: #{::RubyVM::YJIT.runtime_stats[:ratio_in_yjit]}" ' ratio_in_yjit: 0.9550806290358625 ``` Motivation: This should allow for better understanding of how to tune `--yjit-exec-mem-size`. From https://github.com/ruby/ruby/blob/ef084cc8f4958c1b6e4ead99136631bef6d8ddba/doc/yjit/yjit.md?plain=1#L213-L223: > If you start Ruby with `--yjit-stats`, e.g. using an environment variable `RUBYOPT=--yjit-stats`, > `RubyVM::YJIT.runtime_stats[:ratio_in_yjit]` shows the ratio of YJIT-executed instructions in %. > Ideally, `ratio_in_yjit` should be as large as 99%, and increasing `--yjit-exec-mem-size` often > helps improving `ratio_in_yjit`. How to test the change?: ``` RUBYOPT="--yjit --yjit-stats=quiet" bundle exec rake spec:yjit bundle exec rake spec:yjit # skipped ```
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3991 +/- ##
==========================================
- Coverage 97.87% 97.86% -0.01%
==========================================
Files 1314 1314
Lines 78652 78665 +13
Branches 3909 3911 +2
==========================================
+ Hits 76978 76984 +6
- Misses 1674 1681 +7 ☔ View full report in Codecov by Sentry. |
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.
Hey @jethrodaniel thanks for opening up the discussion on this! I think is a nice thing to add, would happily approve the PR once CI is happy and whatnot :)
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.
I've triggered the CI run and left a few notes. TBH I think this could go as-is, but I've suggested a few improvements ;)
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. I realized my suggestion for the matrix did not address the part where 3.2 doesn't support --yjit-stats=quiet
so I pushed a tiny change to address that.
I'm just waiting for CI to settle and I'll merge this in. Thanks for the nice contribution! 🙏
What does this PR do?
When ruby's extended YJIT stats are available (
--yjit-stat
), then we have access to numerous additional stats.This adds
:ratio_in_yjit
, which is only present when additional stats are enabled.Example:
Motivation:
This should allow for a better understanding of how to tune
--yjit-exec-mem-size
.From https://github.com/ruby/ruby/blob/ef084cc8f4958c1b6e4ead99136631bef6d8ddba/doc/yjit/yjit.md?plain=1#L213-L223:
Additional Notes:
Fwiw, I directly based this on the previous YJIT PRs, which were very helpful here:
ratio_in_yjit
has been available sine 3.3.0: https://github.com/ruby/ruby/blob/caf0d2058aa223515897401ff94e11e1c0671ce0/doc/NEWS/NEWS-3.3.0.md?plain=1#L436How to test the change?