Rework how the codegen coordinator code handles the allocator shim #145955
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_gcc |
This comment has been minimized.
This comment has been minimized.
|
Seems that mixing prefer-dynamic and no prefer-dynamic between crate types is currently broken for more reasons than the allocator shim, so the first commit doesn't have any benefits on it's own. It is a prerequisite for the second commit however, which IMHO is big enough of an improvement that I would like to see this PR merged. |
|
I would prefer if #145962 gets merged first. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for #145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
|
I have low familiarity with this part of the code, but the changes seem reasonable, with tests and some comments, and I know that you know this stuff well. r=me when you are ready to merge. @rustbot author |
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang#145955.
Making it participate in LTO would be incorrect if you compile a crate as both a dylib (which needs it) and rlib (which must not include it) in the same rustc invocation. With linker plugin LTO, the allocator shim will still participate in LTO as it is safe to do so in that case.
…lsewhere A lot of places had special handling just in case they would get an allocator module even though most of these places could never get one or would have a trivial implementation for the allocator module. Moving all handling of the allocator module to a single place simplifies things a fair bit.
abbb7d1 to
319fe23
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 45b9d13 (parent) -> 71289c3 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 71289c378d0a406a4f537fe4001282d19362931f --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (71289c3): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 4.5%, secondary 3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.7%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.9%, secondary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 466.452s -> 467.362s (0.20%) |
|
Could this be because the allocator shim no longer participates in LTO? I plan to make it participate again a couple PRs later in this series when I moved LTO to a later in the compilation. Having the allocator shim participate in LTO earlier makes the rest of the series harder to implement. It previously was in a weird limbo state between being ignored until linking and being treated like a regular module during LTO. At the same time, I don't expect to finish making it participate in LTO again for at least a month. And the rest of the PRs will likely make it even harder to revert this PR. |
|
It is possible to do some kind of quick and dirty change that makes it participate in LTO, not for merging, just to test whether that fixes the regression? If we had confidence that the regression is fixable then waiting a month would be much more bearable. |
Ensure we emit an allocator shim when only some crate types need one Found this while trying to write a test for rust-lang/rust#145955.
|
Opened MCP908 to describe the full plan I have. |
Misc LTO cleanups Follow up to rust-lang#145955. * Remove want_summary argument from `prepare_thin`. Since rust-lang#133250 ThinLTO summary writing is instead done by `llvm_optimize`. * Two minor cleanups
Continuing from #144503 this centralizes most handling of the allocator shim to a single 4 line block in the codegen coordinator. The allocator shim is small enough that making it go through the main codegen loop and spawning a worker thread for it is wasted effort.