-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
always try inlining functions which do not call other functions #77307
Conversation
rebased on top of #77306 meaning that all inlining ICE I know of are fixed let's try to get a perf run here @bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit d91b7490909d1aff17c37fbbe3d51a8823e59431 with merge b07bb178f07169aa2e38cad62befd47aecc28342... |
☀️ Try build successful - checks-actions, checks-azure |
Queued b07bb178f07169aa2e38cad62befd47aecc28342 with parent d62d3f7, future comparison URL. |
Finished benchmarking try commit (b07bb178f07169aa2e38cad62befd47aecc28342): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Really interesting results! Incremental shows lots of regressions (although some wins too) which I think actually makes sense. Inlining makes our functions bigger which means changes to them take longer and also makes it more prone that a change to one function will cause other calling functions to also need to be re-compiled. Putting aside the incremental tests, there's a lot of green down the results! Check builds are regressed nearly universally so we should probably disable inlining when in check only mode. Debug and opt builds look really good though! There's some nice wins on real world crates: 7% on clap-rs-opt, 5.1% on webrender-debug, 5% on hyper-2-opt, 4.5% on ripgrep-debug and 4.1% on ripgrep-opt. The rustc bootstrap results are also impressive: nearly 9% wallclock time improvement on rustc-middle! |
I would ignore rustc bootstrap timing for now, I'm still experimenting with variance reduction strategies. 9% might not be representative. |
pushed some changes which might reduce the negative perf impact here @bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 9c25e949a8bf81185042f39e57604e44df7ab6a9 with merge 61b181866f0ce12555c356bf46da832c9a45a22e... |
☀️ Try build successful - checks-actions, checks-azure |
Queued 61b181866f0ce12555c356bf46da832c9a45a22e with parent 6ac6c67, future comparison URL. |
Finished benchmarking try commit (61b181866f0ce12555c356bf46da832c9a45a22e): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
let's try again 😅 @bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit bd954f9012e18717651abd20aa754cf4fd57bdfa with merge 5c1c73fd351cfb72d9ec163bc1f61db9e9bf2b63... |
☀️ Try build successful - checks-actions, checks-azure |
Queued 5c1c73fd351cfb72d9ec163bc1f61db9e9bf2b63 with parent 12f667f, future comparison URL. |
Finished benchmarking try commit (5c1c73fd351cfb72d9ec163bc1f61db9e9bf2b63): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
@bors try @rust-timer queue exclude=deeply-nested updated metadata encoding to only use 1 byte |
Awaiting bors try build completion |
⌛ Trying commit 6b17a76 with merge 2f0d06e5b8f278ed4180d80b7b6be06be6da3b36... |
☀️ Try build successful - checks-actions, checks-azure |
Queued 2f0d06e5b8f278ed4180d80b7b6be06be6da3b36 with parent 9832374, future comparison URL. |
Finished benchmarking try commit (2f0d06e5b8f278ed4180d80b7b6be06be6da3b36): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
@rust-timer: 🔑 Insufficient privileges: not in try users |
^ cc @pietroalbini |
@Mark-Simulacrum could you take a look at this? I don't think I changed anything on the bors side regarding permissions. |
@bors rollup=never |
It looks like we end up with more calls to |
@oli-obk and @tmiasko figured out why in this zulip thread for This was a bit surprising, and there wasn't an obvious reason this was done (apart from the case of generators), and may have just been an oversight. This might also be interesting to #80347. Here's my interpretation of what oli and tomasz suggested trying (errors would be my own), and it did bring back the counts of the |
Consistently avoid constructing optimized MIR when not doing codegen The optimized MIR for closures is being encoded unconditionally, while being unnecessary for cargo check. This turns out to be especially costly with MIR inlining enabled, since it triggers computation of optimized MIR for all callees that are being examined for inlining purposes rust-lang#77307 (comment). Skip encoding of optimized MIR for closures, enum constructors, struct constructors, and trait fns when not doing codegen, like it is already done for other items since 49433.
closing in favor of #81079 |
a bit broken, I accidentally rebased #75495 while it was still closed
r? @wesleywiser like before, not yet ready for merge