Don't recompute SymbolExportLevel for upstream crates. #48611
Merged
Conversation
This file contains 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
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
michaelwoerister
removed
the
S-waiting-on-review
pietroalbini
added
the
S-waiting-on-author
michaelwoerister
force-pushed
the
share-generics2
branch
2 times, most recently
from
aac4a2f
to
03f384f
Compare
|
@bors try |
|
⌛ Trying commit 03f384f with merge 1d719d20eb4669ff7545f5d4445975bb1d082f35... |
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum, could you start a perf run for this? It might already bring improvements in the current state (which is still just a refactoring without additional functionality). |
|
Perf run queued. |
|
Thanks, @Mark-Simulacrum! Seems to be pretty good for small programs. |
|
I'm tempted to suggest we land this as-is unless that's a hardship for some reason because it feels like it could give us some fairly amazing wins in the run-pass suite of tests. Does that seem likely/reasonable? |
|
@Mark-Simulacrum, that's a great idea! It might really help with run-pass. r? @alexcrichton , if that's OK with you. |
michaelwoerister
changed the title
|
@bors: r+ Nice wins! |
|
📌 Commit 03f384f has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=5 -- this can't hurt but if it does improve testing times then we could speed up cycle time |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Mar 3, 2018
…=alexcrichton Fixes rust-lang#47311. r? @nrc
michaelwoerister
force-pushed
the
share-generics2
branch
from
03f384f
to
7a008f5
Compare
|
Rebased. @bors r=alexcrichton |
|
📌 Commit 7a008f5 has been approved by |
|
⌛ Testing commit 7a008f5db05e9471fcd17c1717a85247ca65820f with merge 3ff22504c52436459ec0c1716982e429feae394a... |
bors
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Mar 6, 2018
Don't recompute SymbolExportLevel for upstream crates. The data collected in #48373 suggests that we can avoid generating up to 30% of the LLVM definitions by only instantiating function monomorphizations once with a given crate graph. Some more data, collected with a [proof-of-concept implementation](https://github.com/michaelwoerister/rust/commits/share-generics) of re-using monomorphizations, which is less efficient than the MIR-only RLIB approach, suggests that it's still around 25% LLVM definitions that we can save. So far, this PR only cleans up handling of symbol export status. Too early to review still.
4 tasks
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
AppVeyor timeout 😭 |
This was referenced Mar 6, 2018
bors
added a commit
that referenced
this pull request
Mar 20, 2018
Allow for re-using monomorphizations in upstream crates. Followup to #48611. This implementation is pretty much finished modulo failing tests if there are any. Not quite ready for review yet though. ### DESCRIPTION This PR introduces a `share-generics` mode for RLIBs and Rust dylibs. When a crate is compiled in this mode, two things will happen: - before instantiating a monomorphization in the current crate, the compiler will look for that monomorphization in all upstream crates and link to it, if possible. - monomorphizations are not internalized during partitioning. Instead they are added to the list of symbols exported from the crate. This results in less code being translated and LLVMed. However, there are also downsides: - it will impede optimization somewhat, since fewer functions can be internalized, and - Rust dylibs will have bigger symbol tables since they'll also export monomorphizations. Consequently, this PR only enables the `shared-generics` mode for opt-levels `No`, `Less`, `Size`, and `MinSize`, and for when incremental compilation is activated. `-O2` and `-O3` will still generate generic functions per-crate. Another thing to note is that this has a somewhat similar effect as MIR-only RLIBs, in that monomorphizations are shared, but it is less effective because it cannot share monomorphizations between sibling crates: ``` A <--- defines `fn foo<T>() { .. }` / \ / \ B C <--- both call `foo<u32>()` \ / \ / D <--- calls `foo<u32>()` too ``` With `share-generics`, both `B` and `C` have to instantiate `foo<u32>` and only `D` can re-use it (from either `B` or `C`). With MIR-only RLIBs, `B` and `C` would not instantiate anything, and in `D` we would then only instantiate `foo<u32>` once. On the other hand, when there are many leaf crates in the graph (e.g. when compiling many individual test binaries) then the `share-generics` approach will often be more effective. ### TODO - [x] Add codegen test that makes sure monomorphizations can be internalized in non-Rust binaries. - [x] Add codegen-units test that makes sure we share generics. - [x] Add run-make test that makes sure we don't export any monomorphizations from non-Rust binaries. - [x] Review for reproducible-builds implications.
bors
added a commit
that referenced
this pull request
Mar 22, 2018
Allow for re-using monomorphizations in upstream crates. Followup to #48611. This implementation is pretty much finished modulo failing tests if there are any. Not quite ready for review yet though. ### DESCRIPTION This PR introduces a `share-generics` mode for RLIBs and Rust dylibs. When a crate is compiled in this mode, two things will happen: - before instantiating a monomorphization in the current crate, the compiler will look for that monomorphization in all upstream crates and link to it, if possible. - monomorphizations are not internalized during partitioning. Instead they are added to the list of symbols exported from the crate. This results in less code being translated and LLVMed. However, there are also downsides: - it will impede optimization somewhat, since fewer functions can be internalized, and - Rust dylibs will have bigger symbol tables since they'll also export monomorphizations. Consequently, this PR only enables the `shared-generics` mode for opt-levels `No`, `Less`, `Size`, and `MinSize`, and for when incremental compilation is activated. `-O2` and `-O3` will still generate generic functions per-crate. Another thing to note is that this has a somewhat similar effect as MIR-only RLIBs, in that monomorphizations are shared, but it is less effective because it cannot share monomorphizations between sibling crates: ``` A <--- defines `fn foo<T>() { .. }` / \ / \ B C <--- both call `foo<u32>()` \ / \ / D <--- calls `foo<u32>()` too ``` With `share-generics`, both `B` and `C` have to instantiate `foo<u32>` and only `D` can re-use it (from either `B` or `C`). With MIR-only RLIBs, `B` and `C` would not instantiate anything, and in `D` we would then only instantiate `foo<u32>` once. On the other hand, when there are many leaf crates in the graph (e.g. when compiling many individual test binaries) then the `share-generics` approach will often be more effective. ### TODO - [x] Add codegen test that makes sure monomorphizations can be internalized in non-Rust binaries. - [x] Add codegen-units test that makes sure we share generics. - [x] Add run-make test that makes sure we don't export any monomorphizations from non-Rust binaries. - [x] Review for reproducible-builds implications.
bors
added a commit
that referenced
this pull request
Apr 6, 2018
Allow for re-using monomorphizations in upstream crates. Followup to #48611. This implementation is pretty much finished modulo failing tests if there are any. Not quite ready for review yet though. ### DESCRIPTION This PR introduces a `share-generics` mode for RLIBs and Rust dylibs. When a crate is compiled in this mode, two things will happen: - before instantiating a monomorphization in the current crate, the compiler will look for that monomorphization in all upstream crates and link to it, if possible. - monomorphizations are not internalized during partitioning. Instead they are added to the list of symbols exported from the crate. This results in less code being translated and LLVMed. However, there are also downsides: - it will impede optimization somewhat, since fewer functions can be internalized, and - Rust dylibs will have bigger symbol tables since they'll also export monomorphizations. Consequently, this PR only enables the `shared-generics` mode for opt-levels `No`, `Less`, `Size`, and `MinSize`, and for when incremental compilation is activated. `-O2` and `-O3` will still generate generic functions per-crate. Another thing to note is that this has a somewhat similar effect as MIR-only RLIBs, in that monomorphizations are shared, but it is less effective because it cannot share monomorphizations between sibling crates: ``` A <--- defines `fn foo<T>() { .. }` / \ / \ B C <--- both call `foo<u32>()` \ / \ / D <--- calls `foo<u32>()` too ``` With `share-generics`, both `B` and `C` have to instantiate `foo<u32>` and only `D` can re-use it (from either `B` or `C`). With MIR-only RLIBs, `B` and `C` would not instantiate anything, and in `D` we would then only instantiate `foo<u32>` once. On the other hand, when there are many leaf crates in the graph (e.g. when compiling many individual test binaries) then the `share-generics` approach will often be more effective. ### TODO - [x] Add codegen test that makes sure monomorphizations can be internalized in non-Rust binaries. - [x] Add codegen-units test that makes sure we share generics. - [x] Add run-make test that makes sure we don't export any monomorphizations from non-Rust binaries. - [x] Review for reproducible-builds implications.
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The data collected in #48373 suggests that we can avoid generating up to 30% of the LLVM definitions by only instantiating function monomorphizations once with a given crate graph. Some more data, collected with a proof-of-concept implementation of re-using monomorphizations, which is less efficient than the MIR-only RLIB approach, suggests that it's still around 25% LLVM definitions that we can save.
So far, this PR only cleans up handling of symbol export status. Too early to review still.