-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[experiment] remove diverge_from in box expr building #89332
Conversation
@rylev could you do a perf run please? |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 1aac85bb716c09304b313d69d30d74fe7e8e1a8e with merge 7b3d96ffe131afbf1b4626a6102c03f5352678c7... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
Queued 7b3d96ffe131afbf1b4626a6102c03f5352678c7 with parent 8f8092c, future comparison URL. |
Finished benchmarking commit (7b3d96ffe131afbf1b4626a6102c03f5352678c7): comparison url. Summary: This change led to small relevant improvements 🎉 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led to changes in compiler perf. @bors rollup=never |
Trying another approach. This should have similar result, but the change is made in library not in compiler. Please have another perf run. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit bd50bb2ab189a90a6413780f4d2536808b9a7e04 with merge d0848a1101a9caed97f1276d4326bfe2e6bdbc42... |
☀️ Try build successful - checks-actions |
Queued d0848a1101a9caed97f1276d4326bfe2e6bdbc42 with parent d7539a6, future comparison URL. |
Finished benchmarking commit (d0848a1101a9caed97f1276d4326bfe2e6bdbc42): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led 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 |
As I expected the performance is largely similar to removing the diverging path. I have no idea why adding rustc_allocator_nounwind makes expand_crate slower though. |
This needs to assign a reviewer. |
r? rust-lang/libs |
does it? it's still labeled as experiment |
I am unsure whether this should proceed. I can turn this into a PR that removes the unwinding path and gain a bit perf now, but it'll need to reverted anyway to support oom=panic. |
Also this is more T-compiler than T-libs. @rustbot label: -T-libs +T-compiler |
r? rust-lang/compiler |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Remove `NullOp::Box` Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460. ~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely. r? `@jonas-schievink` `@rustbot` label T-compiler
Remove `NullOp::Box` Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460. ~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely. r? `@jonas-schievink` `@rustbot` label T-compiler
Remove `NullOp::Box` Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460. ~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely. r? `@jonas-schievink` `@rustbot` label T-compiler
See if some instruction count regressions can be regained from #89030
r? @ghost