Use protected visibility when building rustc with LLD by davidlattimore · Pull Request #131634 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation45 Commits1 Checks12 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.
I contemplated trying to enable protected visibility for more cases when LLD will be used other than just -Zlinker-features=+lld, but that would be more a complex change that probably still wouldn't cover all cases when LLD is used, so went with the simplest option of just checking if the linker-feature is enabled.
r? lqd
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.
Multiple atomic commits in a single PR are totally fine, that's easier to review and better for history anyway (the way you have this PR is ideal imo). We definitely don't want a bunch of back and forth WIP-style commits that make changes just to undo them or fix things up, but there is no 1PR=1commit rule like LLVM.
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Use protected visibility when LLD feature is enabled and enable it when building rustc
I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.
I contemplated trying to enable protected visibility for more cases when LLD will be used other than just -Zlinker-features=+lld, but that would be more a complex change that probably still wouldn't cover all cases when LLD is used, so went with the simplest option of just checking if the linker-feature is enabled.
r? lqd
☀️ Try build successful - checks-actions
Build commit: 6ad0952 (6ad095273a92c251fb0c1697fd745ab6caf21d2c)
This comment has been minimized.
Finished benchmarking commit (6ad0952): comparison URL.
Overall result: ✅ improvements - no action needed
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 may lead to changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.6% | [-34.2%, -0.2%] | 212 |
| Improvements ✅ (secondary) | -4.6% | [-29.9%, -0.2%] | 225 |
| All ❌✅ (primary) | -1.6% | [-34.2%, -0.2%] | 212 |
Max RSS (memory usage)
Results (primary -2.2%, secondary -3.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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -2.2% | [-6.5%, -0.5%] | 146 |
| Improvements ✅ (secondary) | -3.2% | [-7.0%, -0.6%] | 151 |
| All ❌✅ (primary) | -2.2% | [-6.5%, -0.5%] | 146 |
Cycles
Results (primary -3.3%, secondary -7.4%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -3.3% | [-24.2%, -0.6%] | 53 |
| Improvements ✅ (secondary) | -7.4% | [-22.6%, -1.9%] | 94 |
| All ❌✅ (primary) | -3.3% | [-24.2%, -0.6%] | 53 |
Binary size
Results (primary -0.3%, secondary -0.7%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -0.3% | [-0.8%, -0.1%] | 11 |
| Improvements ✅ (secondary) | -0.7% | [-0.8%, -0.5%] | 37 |
| All ❌✅ (primary) | -0.3% | [-0.8%, -0.1%] | 11 |
Bootstrap: 783.66s -> 778.727s (-0.63%)
Artifact size: 332.16 MiB -> 331.51 MiB (-0.20%)
I was expecting we'd only do this when using lld by default on stable. Just using a different default linker on nightly has bitten us at least once in the recent past, and I didn't think we'd want the 2 configs to drift even more. But if others are fine with it, so am I.
Cool, as expected these results match bjorn's old prototype I mentioned in the MCP.
However, could you explain a bit about the intent? Did you want to change visibility when using lld in general, or when using rust-lld? Because just checking -Zlinker-features=+lld flag is not sufficient to know whether either of these will be true. The linker flavor can be inferred from other arguments or the target, rust-lld needs self-contained linking enabled by the target/CLI (and to use cg_llvm and not other backends).
I wasn't sure about having two commits in a PR
It's very much welcomed in general, however in this case I think we may need to extract the second commit in another PR: the exact handling of use-lld in bootstrap is being reworked by t-bootstrap. Until this clean-up and tests are done, depending on which stage we pass this flag, I'm not personally sure that we couldn't be unknowingly switching from system lld to beta/stage 1 rust-lld, so I'll let @Kobzol comment on this change. If you did this specifically to get protected visibility on our dist artifacts, setting the default visibility explicitly in the CI config seems better to me, but again I'm not on t-infra/t-bootstrap and they will know better.
Perf. results look great! 🎉 The bootstrap change is a bit drive-by, though. We should finally refactor the way we handle LLD in bootstrap, I'll try to priotize it soon-ish. That's partially orthogonal to this PR, though.
I wonder if we should first enable this only for the compiler itself? Both to separate the perf. effect on the compiler from the perf. effect on the binaries, and also to test it for a while on nightly on the compiler before we enable it by default for all binaries compiled by a nightly compiler.
My intent was primarily, at least initially to use protected visibility for rustc-driver. Not tying it to LLD and flagging it on for just rustc initially sounds sensible to me. One thing I'm not sure about though is whether that means I'll need to wait until -Zdefault-visibility hits stable, which, I think would be November 28th. Unless there's a way to only add the flag for later stages - although I can imagine that might not be desirable.
I would only do it for stage 2, we already do a bunch of optimizations on stage 2 only anyway, e.g. PGO and BOLT.
So when building stage 2 rustc, I'd just forcefully set the -Zdefault-visibility flag.
Member
lqd commented
• Loading
One thing I'm not sure about though is whether that means I'll need to wait until
-Zdefault-visibilityhits stable, which, I think would be November 28th
bootstrap uses beta and RUSTC_BOOTSTRAP so the stable cycle is less important. If you wanted to enable this at stage 1, then it'd only need #130005 to land on beta, which should be in the next couple days. If you want to only enable it at stage 2, you can do it now.
IIRC for stage-dependent flags, it should be done in bootstrap, e.g. around here (similarly to -Zdylib-lto, which is not set at stage 1). Then temporarily comment setting this CI environment variable and we can do a try build with smoke tests enabled.
@rustbot author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
| @@ -428,7 +428,7 @@ pub fn linker_flags( |
|---|
| ) -> Vec { |
| let mut args = vec![]; |
| if !builder.is_lld_direct_linker(target) && builder.config.lld_mode.is_used() { |
| args.push(String::from("-Clink-arg=-fuse-ld=lld")); |
| args.push(String::from("-Zlinker-features=+lld")); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep building the standard library with default visibility as CI will build with lld, but the end user may then use this version of the standard library with ld.bfd. For librustc_driver.so it is completely fine though. Same for tools and codegen backends.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point. I suspect that means that parts of the standard library that make it into the dylib, will unfortunately still have default visibility, but there might not be much we can do about that in the short term - unless we were to build a copy of the standard library specifically for use in rustc_driver that used protected visibility, then distribute a copy of the standard library that used default visibility. I guess that'd be like building the compiler with -Zbuild-std and -Zdefault-visibility=protected.
This PR modifies src/bootstrap/src/core/config.
If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.
This comment has been minimized.
As suggested, I've updated this PR so that LLD is no longer involved. I now just set -Zdefault-visibility=protected when building rustc. This means that libstd isn't built with protected symbols, which means we do have a few more relocations than before, so the performance gain might not be as good as in the original check. However, it still should give some benefit. My stage2 rustc now has 1438 GLOB_DAT relocations, whereas the stage0 without this change has 7078. I think the previous way of enabling protected visibility got down to around 700 GLOB_DAT relocations - but that relied on compiling libstd with the flag, which likely would have broken anyone trying to use that libstd with GNU ld < 2.40.
I added a flag --protected-symbol-definitions that defaults to true. This was done in case someone wants to turn off protected symbols when building rustc for some reason. Is this OK having this flag default to true, or perhaps I should instead have a flag that has the opposite meaning and defaults to false? e.g. --default-visibility-definitions.
Use protected visibility when LLD feature is enabled and enable it when building rustc Use protected visibility when building rustc
Currently it's tied to the usage of LLD, so it will be used exactly on the builders that currently use LLD to link rustc.
Yeah, I’m aware of that. I was just pointing out if we are ever going to override this with =false in the config. Also, using =true may not enable it either since it depends on LLD. I think it makes more sense to remove this option and handle it automatically in the bootstrap.
Ah, yeah, I don't think that we would ever disable it, the config option would mostly be useful to people that build rustc and 1) use new LD and want to get the optimization or 2) use LLD and want to disable the optimization (for some reason). Both sounds kind of niche, so until someone tells us that they need it, I'd also just remove the option.
Use protected visibility when building rustc Use protected visibility when building rustc with LLD
Simpler change is good. I've removed the config option and now just use protected symbols when building rustc with LLD.
Since this is something important we don't want to change accidentally, we could/should set the interposable visibility we expect for std explicitly.
Since this is something important we don't want to change accidentally, we could/should set the interposable visibility we expect for std explicitly.
I added a commit for this.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (3639412): comparison URL.
Overall result: ✅ improvements - no action needed
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 may lead to changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.7% | [-31.2%, -0.1%] | 183 |
| Improvements ✅ (secondary) | -4.2% | [-27.5%, -0.1%] | 223 |
| All ❌✅ (primary) | -1.7% | [-31.2%, -0.1%] | 183 |
Max RSS (memory usage)
Results (primary -1.9%, secondary -2.7%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.9% | [-2.9%, -0.6%] | 57 |
| Improvements ✅ (secondary) | -2.7% | [-5.1%, -0.5%] | 67 |
| All ❌✅ (primary) | -1.9% | [-2.9%, -0.6%] | 57 |
Cycles
Results (primary -6.5%, secondary -7.5%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -6.5% | [-22.6%, -0.9%] | 17 |
| Improvements ✅ (secondary) | -7.5% | [-20.9%, -1.8%] | 77 |
| All ❌✅ (primary) | -6.5% | [-22.6%, -0.9%] | 17 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 783.744s -> 781.748s (-0.25%)
Artifact size: 333.54 MiB -> 334.99 MiB (0.44%)
📌 Commit 00da974 has been approved by Kobzol
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
There was a test failure on mingw after I added the commit to set interposable visibility when building std.
error: found crates (`core` and `core`) with colliding StableCrateId values
Not sure if it was a flake or an actual failure. Either way, I removed that commit from this PR and put it in a separate PR - #132432
Looked like a genuine failure to me. Anyway, I agree that it's better to separate these two.
Finished benchmarking commit (9fa9ef3): comparison URL.
Overall result: ✅ improvements - no action needed
@rustbot label: -perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.8% | [-31.3%, -0.1%] | 174 |
| Improvements ✅ (secondary) | -4.2% | [-27.5%, -0.2%] | 224 |
| All ❌✅ (primary) | -1.8% | [-31.3%, -0.1%] | 174 |
Max RSS (memory usage)
Results (primary -2.1%, secondary -3.3%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -2.1% | [-6.4%, -0.5%] | 112 |
| Improvements ✅ (secondary) | -3.3% | [-7.5%, -0.6%] | 129 |
| All ❌✅ (primary) | -2.1% | [-6.4%, -0.5%] | 112 |
Cycles
Results (primary -8.2%, secondary -7.4%)
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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -8.2% | [-22.6%, -0.7%] | 16 |
| Improvements ✅ (secondary) | -7.4% | [-20.7%, -1.6%] | 77 |
| All ❌✅ (primary) | -8.2% | [-22.6%, -0.7%] | 16 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 782.66s -> 779.567s (-0.40%)
Artifact size: 333.48 MiB -> 334.97 MiB (0.45%)
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.