Uh oh!
There was an error while loading. Please reload this page.
Block inlining of IntroSort - #89310
Conversation
With PGO and (via dotnet#88749) one level of recursive inlining enabled, the jit sees the recursive call made by `IntroSort` as an attractive inline candidate, but it isn't. Fixesdotnet#89106.
ghost
commented
Jul 21, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
AndyAyersMS
commented
Jul 21, 2023
@EgorBo@stephentoub PTAL Benchmark is pretty noisy but this should resolve the regressions we've seen.
|
ghost
commented
Jul 21, 2023
Tagging subscribers to this area: @dotnet/area-system-runtime |
| } | ||
| } | ||
| [MethodImpl(MethodImplOptions.NoInlining)] |
There was a problem hiding this comment.
Can we include a comment as to why this is here? I imagine the calculus around whether this is valuable could change in the future.
There was a problem hiding this comment.
Added.
Note IntroSort is nominally doubly recursive but has already had the tail recursive call transformed into a loop manually. If we were ever tempted to undo that and rely on the JIT to do something similar, we might find the NoInline annotation would inhibit that transformation too.
@EgorBo we might consider special heuristics for recursion, generally if the method is large and doesn't have a frequent fast path that avoids recursion, then there's not much to be gained.
AndyAyersMS
commented
Jul 23, 2023
Perf has returned to "normal" Note there is a longer-term regression here from PGO, see #87194. |
With PGO and (via #88749) one level of recursive inlining enabled, the jit sees the recursive call made by
IntroSortas an attractive inline candidate, but it isn't.Fixes#89106.