-
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
[WIP] Test performance of running MIR inliner on inline(always) function calls when mir-opt-level=1 #110560
[WIP] Test performance of running MIR inliner on inline(always) function calls when mir-opt-level=1 #110560
Conversation
r? @oli-obk (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 796cafe with merge b47b7746514197f42ef70d9744fcbbea0256a508... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b47b7746514197f42ef70d9744fcbbea0256a508): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
⬆️ In that experiment I enabled debug Not so bad for such a stressful experiment! Let's now repeat the experiment, but early reject inline candidates without an |
db67429
to
796cafe
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit e65665f with merge f283ebe5c544c33161c94d8b60b2423af93b8148... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f283ebe5c544c33161c94d8b60b2423af93b8148): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
⬆️ In that experiment, I enabled the Inlining pass for debug builds, but considered for inlining only functions marked as debug Wow, much better! These numbers are very inspiring. Let's now restrict the inlining consideration to non-local functions only (i.e. to function from other crates). If I understand it correctly, in this case, we will not call |
⌛ Trying commit cd69787 with merge e5fd7c91e36b8c7f682f5c0fe631164c5d15e628... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (e5fd7c91e36b8c7f682f5c0fe631164c5d15e628): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
⬆️ In that experiment, I enabled the Inlining pass for debug builds, but considered for inlining only non-local functions marked as debug It looks like there are very few regressions now! The most promising is that now there's almost no regression in Let's do some real inlining now! In the next experiment, I'm enabling inlining for debug builds (mir-opt-level=1) with all the previous restricting rules. |
2db86ac
to
43695a4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4078064
to
40cf2cb
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
@bors try |
⌛ Trying commit 40cf2cb with merge 5142ecd1025428756f8bad85db21c70248982db7... |
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (5142ecd1025428756f8bad85db21c70248982db7): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
⬆️ In that experiment, I enabled the Inlining pass for debug builds (mir-opt-level=1), but considered for inlining only non-local functions (i.e. functions from other crates) marked as debug debug debug These results show that the case is not hopeless, and enabling |
It seems #105278 is stalled, so I'd like to perform several performance tests with different MIR inliner setups.
Let's start with just reading a callee MIR body without actually inlining it (in both incremental and non-incremental configurations).