-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 rust-lld
on nightly x86_64-unknown-linux-gnu
#124129
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Enable `rust-lld` on nightly `x86_64-unknown-linux-gnu` r? ghost Draft until final preparations are done (making a bootstrap change entry requires a PR number)
Some changes occurred in run-make tests. cc @jieyouxu This PR modifies If appropriate, please update This PR modifies If appropriate, please update These commits modify compiler targets. |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (5a8c051): comparison URL. Overall result: ✅ improvements - no 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. @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: 676.731s -> 669.514s (-1.07%) |
For the bootstrap change: |
The bootstrap change is "temporary", in the sense that once this change lands on beta, it will be enabled by default and bootstrap won't need to configure it, if I understand it correctly. So the bootstrap change doesn't really modify the meaning of The annoying thing here is that we have many axes on what to configure - should LLD be built? (this should arguably be in the |
I think this makes sense to me as the right direction to go in, rather than trying to add another option in this PR. Overall this PR looks good to me at a high level, though I can't review the correctness of the compiler change (@petrochenkov should probably sign off on that), it seems OK to me. I think we should proceed with it. One thing that surprises me though is that this had a positive effect on the binary sizes for what we ship. Were we not already using LLD during that build? Perhaps we were picking up an old LLD from somewhere? I agree that we should have a blog post notifying users of this change (similar to the parallel frontend one). Once that's ready and a few nits here are fixed, I'm happy to r+ |
1978bc2
to
ded5bfb
Compare
The artifact size reduction is also somewhat surprising to me. I believe that we are indeed already using LLD for that build, via |
I wonder if these size reductions may be some kind of noise, they were not present in the previous runs of #113382 (other than the regular small fluctuations; the only difference with this PR is more builders are enabled there) including this one from yesterday. The artifacts in the dist build are not linked with the self-contained linker (rust-lld/lld 18.1.4), but via Let's maybe see if another run has different results. edit: done below, no significant change |
This comment was marked as resolved.
This comment was marked as resolved.
@bors retry The runner has received a shutdown signal |
☀️ Test successful - checks-actions |
Finished benchmarking commit (8af67ba): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression 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)Results (primary 16.6%, secondary 3.8%)This 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.
CyclesResults (primary -30.6%, secondary -24.2%)This 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 sizeResults (primary 2.0%, secondary 1.8%)This 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: 678.599s -> 669.726s (-1.31%) |
Link is currently failing with rust-lld (rust-lang/rust#124129).
With rust-lang/rust#124129, x64 linux switches to use LLD by default, but this causes dependency issues on NixOS. Disable LLD for now. This is tracked by rust-lang/rust#125321.
We believe we have done virtually all the internal work and tests we could to prepare for using
lld
as the default linker (at least on Linux). We're IMHO at a point where we'd need to expand testing and coverage in order to make progress on this effort.Therefore, for further testing and gathering real-world feedback, unexpected issues and use-cases, this PR enables
rust-lld
as the default linker:x86_64-unknown-linux-gnu
onlydownload-ci-llvm
), so that distros are not impactedas described in more detail in this zulip thread.
In case any issues happen to users, as e.g. lld is not bug-for-bug compatible with GNU ld, it's easy to disable with
-Zlinker-features=-lld
to revert to using the system's default linker.I don't know who should review this kind of things, as it's somewhat of a crosscutting effort. Compiler contributor, compiler performance WG and infra member sounds perfect, so r? @Mark-Simulacrum.
The last crater run encountered a low number (44) of mainly avoidable issues, like small incompatibilities, user errors, and a difference between the two linkers about which default to use with
--gc-sections
. Here's the triage report, categorizing the issues, with some analyses and workarounds. I'd appreciate another set of eyes looking at these results.The changes in this PR have been test-driven for CI changes, try builds with tests enabled, rustc-perf with bootstrapping, in PR #113382.
For infra, about the CI change: this PR forces
rust.lld
to false on vanilla LLVM builders, just to make sure we have coverage withoutrust-lld
. Though to be clear, just using an external LLVM is already enough to keeprust.lld
to false, in turn reverting everything to using the system's default linker.cc @rust-lang/bootstrap for the bootstrap and config change
cc @petrochenkov for the small compiler change
cc @rust-lang/wg-compiler-performance
The blog post announcing the change, that we expect to merge around the same time as we merge this PR, is open on the blog repo.
Bootstrap change history: this PR changes the default of a config option on
x86_64-unknown-linux-gnu
. It's, however, not expected to cause issues, or require any changes to existing configurations. It's a big enough change that people should at least know about it, in case it causes unexpected problems. If that happens, setrust.lld = false
in yourconfig.toml
(and open an issue).