YJIT: Add counter to measure how often we compile "cold" ISEQs #535
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
maximecb
merged commit 5cde049
into
v3.2.2-pshopify15-experimental2
124 of 168 checks passed
k0kubun
reviewed
Sep 14, 2023
| @@ -429,7 +429,7 @@ jit_compile(rb_execution_context_t *ec) | |||
| if (body->jit_entry == NULL) { | |||
| body->jit_entry_calls++; | |||
| if (yjit_enabled) { | |||
| if (body->jit_entry_calls == rb_yjit_call_threshold()) { | |||
| if (rb_yjit_threshold_hit(iseq, body->jit_entry_calls)) { | |||
There was a problem hiding this comment.
I think you want to make this change to jit_compile_exception as well.
There was a problem hiding this comment.
Eventually probably yes, but for now I would just like to see how the numbers come out for ISEQ entries.
There was a problem hiding this comment.
No this will mess with the numbers, the ratio being computed is in relation to the number of entries we compile...
There was a problem hiding this comment.
Note that compiled_iseq_entry counts both normal entries and exception entries, so with this patch, you can't calculate the ratio of compiled_entry_cold entries relative to what this rb_yjit_threshold_hit targets, though.
There was a problem hiding this comment.
Hmm, I thought it didn't. It won't be fully accurate if we call rb_yjit_threshold_hit in both places either though 🤔
I guess let's just live with this inaccuracy for now. I'm assuming that exception entries are a small percentage overall. If the number is high enough for this kind of optimization to be interesting, we'll figure out how to implement this properly for Ruby master.
casperisfine
pushed a commit
that referenced
this pull request
Sep 20, 2023
maximecb
added a commit
that referenced
this pull request
Sep 27, 2023
maximecb
added a commit
that referenced
this pull request
Sep 28, 2023
maximecb
added a commit
that referenced
this pull request
Sep 28, 2023
Fix counter name in DEFAULT_COUNTERS YJIT: add --yjit-cold-threshold, don't compile cold ISEQs YJIT: increase default cold threshold to 200_000 Remove rb_yjit_call_threshold() Remove conflict markers Fix compilation errors Threshold 1 should compile immediately
maximecb
added a commit
that referenced
this pull request
Oct 2, 2023
Fix counter name in DEFAULT_COUNTERS YJIT: add --yjit-cold-threshold, don't compile cold ISEQs YJIT: increase default cold threshold to 200_000 Remove rb_yjit_call_threshold() Remove conflict markers Fix compilation errors Threshold 1 should compile immediately Debug deadlock issue with test_ractor Fix call threshold issue with tests
k0kubun
pushed a commit
that referenced
this pull request
Oct 5, 2023
* YJIT: Add counter to measure how often we compile "cold" ISEQs (#535) Fix counter name in DEFAULT_COUNTERS YJIT: add --yjit-cold-threshold, don't compile cold ISEQs YJIT: increase default cold threshold to 200_000 Remove rb_yjit_call_threshold() Remove conflict markers Fix compilation errors Threshold 1 should compile immediately Debug deadlock issue with test_ractor Fix call threshold issue with tests * Revert exception threshold logic. Document option in yjid.md * (void) for 0 parameter functions in C99 * Rename iseq_entry_cold => cold_iseq_entry * Document --yjit-cold-threshold in ruby.c * Update doc/yjit/yjit.md Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> * Shorten help string to appease test * Address bug found by Kokubun. Reorder logic. --------- Co-authored-by: Alan Wu <XrXr@users.noreply.github.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
k0kubun
pushed a commit
that referenced
this pull request
Oct 5, 2023
* YJIT: Add counter to measure how often we compile "cold" ISEQs (#535) Fix counter name in DEFAULT_COUNTERS YJIT: add --yjit-cold-threshold, don't compile cold ISEQs YJIT: increase default cold threshold to 200_000 Remove rb_yjit_call_threshold() Remove conflict markers Fix compilation errors Threshold 1 should compile immediately Debug deadlock issue with test_ractor Fix call threshold issue with tests * Revert exception threshold logic. Document option in yjid.md * (void) for 0 parameter functions in C99 * Rename iseq_entry_cold => cold_iseq_entry * Document --yjit-cold-threshold in ruby.c * Update doc/yjit/yjit.md Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> * Shorten help string to appease test * Address bug found by Kokubun. Reorder logic. --------- Co-authored-by: Alan Wu <XrXr@users.noreply.github.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
casperisfine
pushed a commit
that referenced
this pull request
Oct 9, 2023
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.
I'd like to deploy this to Core so we can monitor the percentage of cold ISEQ entries that get compiled over the weekend.
The thresholds chosen are hardcoded, which is slightly hacky and just for this experiment, but they are set such that our largest benchmark (lobsters) has a small percentage ~2.9% of "cold" ISEQs only, with the idea that a much larger app should have a larger percentage of cold entries compiled, just because it has so much more code.
The idea being: maybe we can find heuristics that are set such that our benchmarks see no performance difference, the same amount of code gets compiled, but real production deployments benefit from less cold and rarely executed code being compiled.