Crate store cleanup #52927
Crate store cleanup #52927
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
cratestore-cleanup
branch
from
e1fc421 to
9a061d2
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
cratestore-cleanup
branch
from
9a061d2 to
dd753cd
Compare
|
@bors r+ |
|
📌 Commit dd753cd has been approved by |
bors
added
S-waiting-on-bors
… r=varkor Crate store cleanup Each commit mostly stands on its own. Most of the diff is lifetime-related and uninteresting.
… r=varkor Crate store cleanup Each commit mostly stands on its own. Most of the diff is lifetime-related and uninteresting.
src/Cargo.lock
Outdated
| @@ -2115,6 +2115,7 @@ dependencies = [ | |||
| "rustc 0.0.0", | |||
| "rustc_data_structures 0.0.0", | |||
| "rustc_incremental 0.0.0", | |||
| "rustc_metadata 0.0.0", | |||
There was a problem hiding this comment.
Could this and the dependency for rustc_resolve be avoided? Keeping these crates decoupled keeps incremental compile times down if the crates are being modified and helps parallel builds in general
There was a problem hiding this comment.
I can, yes -- I'd rather not though. metadata compiles in ~10 seconds for me at least on a non-incremental build IIRC so it's not too heavy weight; generally speaking it feels like optimizing dependency edges between crates like that isn't the right thing to be doing; it's not tenable long-term and generally feels somewhat wrong to me.
But if you think this is a serious hit I can spend some time pulling things back into librustc, though some part of me thinks that we should be actively trying to reduce the size of librustc, syntax, and librustc_mir so that they compile faster as today each takes >100 seconds on clean builds even on good hardware.
There was a problem hiding this comment.
Hm are you sure it compiles that fast? This travis log claims 152s for rustc_metadata (and 373s for rustc), and locally it just took 76s for rustc_metadata and and 159s for rustc. I think 10s is vastly underestimating how long it takes to build this crate (unless you've got hyper-speed hardware!)
I agree though yeah this shouldn't go in librustc, this could perhaps go in a new utils crate or something like that (not putting things in librustc is good).
There was a problem hiding this comment.
Sure we can create "utils" crates but ultimately then what it feels like we're going to end up with is librustc_metadata_utils, librustc_target_utils, librustc_typeck_utils etc for those functions which are currently somewhat out of place in librustc/middle (often used by driver and metadata, or typeck and driver, or some other pair of libraries with one always ultimately depending on the other); or at least that's been my impression while trying to do some mild cleanup to middle.
I'll see if it makes sense to have a util crate that essentially just depends on librustc and then put this and some similar functions in there (for now probably adding a comment that if you find yourself wanting to add a dependency other than rustc/syntax to it you probably shouldn't; the timing results below do seem to indicate that metadata is a bit more hefty than I had expected.
Looking at a build I just did it's actually quite painful that metadata takes so long to build on Travis: as a loose statistic, metadata is ~10k lines of code and librustc is ~100k, but on Travis they build in around a 2.6x difference (400s for librustc and 150s for metadata in stage1).
Locally in stage 1 I see librustc build in 270 seconds and metadata in 60 seconds -- it does look like it's a little heavier than I thought; I might've been recalling a check build.
It's also rather surprising that metadata builds in 114 seconds in stage0 but 150 seconds in stage 1. That's a rather significant disparity!
There was a problem hiding this comment.
Ah, I didn't even consider this. I'll pay more careful attention to the dependencies next time.
src/librustc_codegen_utils/link.rs
Outdated
| @@ -61,7 +62,7 @@ pub fn find_crate_name(sess: Option<&Session>, | |||
| attrs: &[ast::Attribute], | |||
| input: &Input) -> String { | |||
| let validate = |s: String, span: Option<Span>| { | |||
| cstore::validate_crate_name(sess, &s, span); | |||
| creader::validate_crate_name(sess, &s, span); | |||
There was a problem hiding this comment.
For example, could this move to rustc or to a different crate? The rustc_metadata crate is quite heavyweight and pulling it in for one function may not quite be worth it
|
@bors r- |
bors
added
S-waiting-on-author
Mark-Simulacrum
force-pushed
the
cratestore-cleanup
branch
from
dd753cd to
6fdd6f6
Compare
|
I've removed the dependency on metadata from codegen utils but the resolve dependency is more deeply rooted; fundamentally that's because resolve is actually guiding metadata loading/crate registration. The previous way of avoiding the dep was to have a trait in librustc and then resolve would store a trait object which I can return to but doesn't feel quite right. |
|
@bors: r+ |
|
📌 Commit 6fdd6f6 has been approved by |
bors
added
S-waiting-on-bors
… r=alexcrichton Crate store cleanup Each commit mostly stands on its own. Most of the diff is lifetime-related and uninteresting.
…hton Crate store cleanup Each commit mostly stands on its own. Most of the diff is lifetime-related and uninteresting.
|
☀️ Test successful - status-appveyor, status-travis |
Tested on commit rust-lang/rust@c11f2d2. Direct link to PR: <rust-lang/rust#52927> 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
|
Each commit mostly stands on its own.
Most of the diff is lifetime-related and uninteresting.