-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: increase inline budget #114191
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
JIT: increase inline budget #114191
Conversation
The current JIT inline strategy is prone to running out of budget at inopportune times, deeply inlining at some top-level sites and not inlining at all at others. This doesn't happen all that often, but when it does it has very adverse impact on performance. While we await a better strategy, we can at least reduce how often this happens by increasing the budget. Partially addresses regressions seen in dotnet#113913
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.
Pull Request Overview
This PR increases the JIT inline budget to alleviate issues where the inlining strategy runs out of budget during critical compilation moments, partially addressing performance regressions noted in #113913.
- Increased DEFAULT_INLINE_BUDGET from 10 to 20 in src/coreclr/jit/compiler.h
Comments suppressed due to low confidence (1)
src/coreclr/jit/compiler.h:11425
- Ensure that existing performance tests address the impact of increasing the inline budget, verifying that compile times remain within acceptable ranges while benefiting inlining.
#define DEFAULT_INLINE_BUDGET 20 // Maximum estimated compile time increase via inlining
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Do we have a good way to see the impact including the VM overhead today? Will we be able to look at the crossgen or JIT startup benchmarks to help get an idea of the wins/losses? A couple notes from looking through the diffs on things that popped out. Nothing that really impacts this PR, but rather SPMI diffs more generally that I've seen in a few PRs now and that might be nice to see if there is a way to improve anything here.
|
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. We'll want to keep a close eye out on next weeks perf triage results and adjust accordingly!
Those historically haven't been that useful. But we can certainly check crossgen for impact. The TP increases in the diffs are modest, but there are enough missed contexts to make interpretation difficult. These misses cut two ways: they cause cycle losses on the diff side because methods fail part way, and those methods fail because they're likely trying to do inlines that SPMI can't support, so if they had succeeded, they would likely have taken longer... so not easy to extrapolate from the info there. Also the jit can spend time trying inlines that eventually fail so don't show up as code diffs but will show up as TP diffs. I can create a bespoke SPMI collection for ASP.NET with these changes. Likely that gives something that will replay cleaning with both old and new jits, and from that we can at least get a better measure of TP impact within the JIT.
I don't think we know how to produce output that is both limited and maximally informative. I think you just get the first N lines and sometimes that's just symbol diffs. |
With bespoke asp.net SPMI collection
So 1% TP increase (JIT only), 0.4% code size increase. |
Ok, let's merge and keep an eye out for anything unexpected. Will kick of an SPMI collection once this gets mirrored. |
The current JIT inline strategy is prone to running out of budget at inopportune times, deeply inlining at some top-level sites and not inlining at all at others. This doesn't happen all that often, but when it does it has very adverse impact on performance.
While we await a better strategy, we can at least reduce how often this happens by increasing the budget.
Partially addresses regressions seen in #113913