rustc: Remove dylib crate type from most rustc crates
#56987
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors: try |
rustc: Remove `dylib` crate type from most rustc crates Now that procedural macros no longer link transitively to libsyntax, this shouldn't be needed any more! This commit is an experiment in removing all dynamic libraries from rustc except for librustc_driver itself. Let's see how far we can get with that!
|
I believe this won't work - clippy, codegen, perhaps miri aren't built in the same target directory as librustc_* so without dylib I think they won't be able to load those libraries from the prelude? Perhaps I'm forgetting what files we copy/need though and just the rlib is enough? Though in that case it's not super clear to me why proc macros needed the dylibs either... |
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum heh well I can empirically say that it does indeed work! All of those tools load the compiler primarily through the Any compiler crate can be loaded at any time through the sysroot, just because they aren't dylibs doesn't mean they can't be loaded (you can already today load the compiler's own crates.io dependencies). The sysroot still contains all the intermediate rlib files. Locally I've run diff --git a/src/lib.rs b/src/lib.rs
index 40694726..5cb1a737 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -17,6 +17,8 @@
// (currently there is no way to opt into sysroot crates w/o `extern crate`)
#[allow(unused_extern_crates)]
extern crate rustc_plugin;
+#[allow(unused_extern_crates)]
+extern crate rustc_driver;
use self::rustc_plugin::Registry;
#[plugin_registrar] |
|
@rust-timer build b7ced6c |
|
Success: Queued b7ced6c with parent 6f839fb, comparison URL. |
This is in anticipation for rust-lang/rust#56987 where the `rustc_driver` crate being linked in will be required to link correctly against the compiler. In the meantime it should be harmless otherwise!
|
Okay, yeah, that makes sense. I guess I just didn't quite connect the dots so to speak on rlib crates being loadable, but of course they have to be as otherwise the whole ecosystem wouldn't work. However, one additional concern with doing this is that it bumps artifact sizes by ~1.6x (from 487 MB to 765 MB). With a |
|
That's a good point! I hadn't written up a full summary for this yet because if it doesn't affect perf at all it's probably not worth it. That being said it's definitely expected that this will increase sizes. Rust dylibs are naturally much smaller than rlibs for two reasons:
That's just to say there's no hacks here AFAIK, it's just fundamental that the rlib sizes will be much larger. I don't think installation size is really all that important, though. So long as we're not an order of magnitude off at least! The more imporant metric (I think at least) is the download size. Downloading the rust-std+rustc components before this commit was 142MB, and this commit (from try) is 232 megabytes. Funnily enough that's also a 1.6x increase! I sort of highly doubt that anyone will notice this in practice. We add dependencies to the compiler willy-nilly and have already increased our download size 20% over the last year with binaries alone (that's not even taking into account the 86% increase over the last year in rust-docs download size). |
|
Finished benchmarking try commit b7ced6c |
|
Impressive amount of green 🎉 |
This is in anticipation for rust-lang/rust#56987 where the `rustc_driver` crate being linked in will be required to link correctly against the compiler. In the meantime it should be harmless otherwise!
alexcrichton
force-pushed
the
less-dylibs
branch
from
6419362 to
b503a94
Compare
|
Ok, great! I think that's green enough that this is worth considering at least. To detail this a bit more, we've historically used the Note that today tons of rlibs ship with the compiler. All of rustc's crates.io dependencies are compiled as rlibs and linked into one dylib or another. What this PR is doing is basically creating one dynamic library for the compiler, only Any tool which depends on the compiler (RLS, miri, clippy, rustdoc, etc) all use rustc through the There's a few major consequences as a result from this change:
I'm curious what others think of this change! This ideally shouldn't affect how the compiler is really developed that much, but I think I'd like to land this for the dynamic loader improvements on Linux. The dynamic library surely still has an absolutely massive symbol table, but it'd be great if we could try to minimize it in the future via one way or another. My favorite part about this, however, is how it simplifies the distribution of the compiler as there's only one dynamic library instead of a large proliferating set! Also note that the Ok for a randomly selected compiler team member... r? @eddyb and also cc @rust-lang/compiler and @rust-lang/release, but feel free to unsubscribe yourself if you're not interested in this change |
|
I'm definitely in favor of this change. This should also speed up tests on Linux and enable us to throw ThinLTO on |
Link to `rustc_driver` crate in plugin This is in anticipation for rust-lang/rust#56987 where the `rustc_driver` crate being linked in will be required to link correctly against the compiler. In the meantime it should be harmless otherwise!
|
Cool! Thanks, Alex! I'd also be interested whether we can do some kind of cross-crate LTO in the future. |
Might be interesting to add It hurts somewhat that we are increasing the download size by 1.6x here. Just 25% decrease was a major drive in adding |
|
@Zoxc @michaelwoerister ah yes good points! While we're not really ready to just flip a switch to do that, this would be a prerequisite for doing it. |
alexcrichton
force-pushed
the
less-dylibs
branch
from
6eee8e0 to
36a185e
Compare
|
@bors: r=michaelwoerister |
|
📌 Commit 36a185e has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 36a185e with merge faebe862969eb67db28aece199d19cde4c021d66... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
Memory access violation: |
kennytm
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #57416) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The segfault is legitimate and I could reproduce locally, but I don't have the time to investigate this so I'm going to close it. |
|
I tried rebasing this change on the current master to build it locally on mingw but I run into a linker issue ( |
|
@euclio that's different error, unrelated to previous one. |
|
@alexcrichton Has shipping |
ghost
mentioned this pull request
rustc: Remove `dylib` crate type from most rustc crates Revival of rust-lang#56987 cc @alexcrichton r? @michaelwoerister
rustc: Remove `dylib` crate type from most rustc crates Revival of #56987 cc @alexcrichton r? @michaelwoerister
rustc: Remove `dylib` crate type from most rustc crates Revival of rust-lang/rust#56987 cc @alexcrichton r? @michaelwoerister
Now that procedural macros no longer link transitively to libsyntax,
this shouldn't be needed any more! This commit is an experiment in
removing all dynamic libraries from rustc except for librustc_driver
itself. Let's see how far we can get with that!
More details below about this change