-
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
Enable constant folding across crates (weak linkage + comdat) #114816
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 0aac118e1d1a266e0df946c9d45ecec205090fff with merge 1683cf3d081d95409f167be3f83bf1751c161e55... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
Some(ConstAllocationDebugHint::CallerLocation) => "callerloc_", | ||
Some(ConstAllocationDebugHint::TypeName) => "typename_", | ||
Some(ConstAllocationDebugHint::VTable) => "vtable_", | ||
None => "alloc_", |
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.
Please use a prefix which is clearly unique to rustc (and preferably to a single version of rustc in case staticlibs of different rustc versions are mixed)
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.
Done: feel free to provide suggestions on the new naming scheme
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c9fccbb340b8b5dd74189b8980b0d3b586ca47a6): 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.
Binary sizeResultsThis 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.
Bootstrap: 633.189s -> 639.354s (0.97%) |
This could use another perf run, which should be much better since it doesn't have the effect of disabling DCE anymore. (I don't have permission) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 30ff300 with merge f923a643d2dec533a4239551e9f4011e0b53bcab... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f923a643d2dec533a4239551e9f4011e0b53bcab): 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.
Binary sizeResultsThis 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.
Bootstrap: 633.508s -> 635.971s (0.39%) |
that's still pretty horrifying, tbh. |
Something must be wrong though: We are not even seeing the reported wins on binary size? |
My testing was on Windows, where I was seeing minor improvements, whereas the Rust performance report is on Linux. I can replicate the binary size regressions locally using WSL, so I'm going to dig into what's happening... |
Looks like it's extra data in the symbol table - if I run I'll see if I can figure out how to instruct LLVM not to include these names in the symbol table if debug info isn't enabled. I've also found that part of the regression comes from building the standard library with debug info enabled, which results in that debug info being in the final binary even if it is built with |
I'm closing this PR for now: given the build-time regression and binary size regression (for Linux) |
Draft PR for rust-lang/compiler-team#662