-
Notifications
You must be signed in to change notification settings - Fork 13k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate typeck/trans into distinct crates. #19362
Conversation
You're a machine! |
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray file?
@nikomatsakis out of curiosity can you check the distribution tarball size before/after this? Before we commit too much to this route I just wanna be aware if we're increasing our distribution size by many MB for each split. I suspect it won't have too too much impact, but may as well be safe! |
@alexcrichton sure, good question. How do I check that? (some makefile target, I guess?) I should also do some timings. I am curious to see how much (if any) this helps with |
Heads up: I was compiling this on top of master to collect distribution size information, but it doesn't compile as it is: src/librustc_typeck/lib.rs:68:1: 68:39 error: unused attribute, #[deny(unused_attributes)] on by default
src/librustc_typeck/lib.rs:68 #![comment = "The Rust type checker."]
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/librustc_typeck/lib.rs:69:1: 69:25 error: unused attribute, #[deny(unused_attributes)] on by default
src/librustc_typeck/lib.rs:69 #![license = "MIT/ASL2"] (I didn't know that raised a warning now!) |
Also it seems |
@nikomatsakis I think that |
@japaric yeah, I guess I got trigger-happy and didn't let the make process run all the way through for the last commit. Interestingly, after fixing various minor things, I am now encountering a stack overflow (!). Not sure yet what's going on, though it looks legit -- i.e., it's just overflowing the stack. But I could be wrong. |
(What's odd is that this occurs in the driver crate) |
Status: I am narrowing down precisely what change causes the overflow |
ed1f631
to
629b567
Compare
So this is rebased and fully functional. @alexcrichton in 629b567 I upped the stack size to 32MB. This is probably bad for some reason and I suspect we could get away with less. Thoughts? |
@nikomatsakis Compilation error after checking out this branch: librustc_trans/back/link.rs:157:12: 157:21 error: unresolved enum variant, struct or const `FileInput`
src/librustc_trans/back/link.rs:157 if let FileInput(ref path) = *input { |
- if let FileInput(ref path) = *input {
+ if let Input::File(ref path) = *input { |
@alexcrichton the distribution size is basically unaffected by this patch. I see the normal distribution as 108MB and the version on this branch as 108MB. |
@japaric stupid rebase, thanks :) |
@alexcrichton Can confirm that the distribution size is pretty much unaffected:
Will try to collect some timing information |
Awesome, thanks for the investigation @nikomatsakis and @japaric! |
@nikomatsakis also upping the stack size seems ok to me. |
629b567
to
39eed46
Compare
Timing information:
More detailed (per crate-ish) timings A nice 230 seconds (12.6%) speedup for the fully optimized (no parallel codegen) case. |
down on its exports. Remove some dead code that is revealed.
doesn't work in a multi-crate context. We'll need to come up with something better.
structs out from driver and into other places.
…B of stack unconditionally. This is prompted by some sort of bug in trans that causes a stack overflow when the modules in trans are made private. (In particular, the overflow can also be avoided by making `controlflow` and `callee` public, but that seems strictly worse than just using more stack.)
39eed46
to
5d19432
Compare
Rebased! |
I am going to start giving this p=1 precedence because it can improve cycle time, memory usage, and (perhaps most importantly) incremental recompilation, and thus has a "waterfall" effect on later productivity. |
eadfa94
to
602fc78
Compare
This has the goal of further reducing peak memory usage and enabling more parallelism. This patch should allow trans/typeck to build in parallel. The plan is to proceed by moving as many additional passes as possible into distinct crates that lay alongside typeck/trans. Basically, the idea is that there is the `rustc` crate which defines the common data structures shared between passes. Individual passes then go into their own crates. Finally, the `rustc_driver` crate knits it all together. cc @jakub-: One wrinkle is the diagnostics plugin. Currently, it assumes all diagnostics are defined and used within one crate in order to track what is used and what is duplicated. I had to disable this. We'll have to find an alternate strategy, but I wasn't sure what was best so decided to just disable the duplicate checking for now.
This has the goal of further reducing peak memory usage and enabling more parallelism. This patch should allow trans/typeck to build in parallel. The plan is to proceed by moving as many additional passes as possible into distinct crates that lay alongside typeck/trans. Basically, the idea is that there is the
rustc
crate which defines the common data structures shared between passes. Individual passes then go into their own crates. Finally, therustc_driver
crate knits it all together.cc @jakub-: One wrinkle is the diagnostics plugin. Currently, it assumes all diagnostics are defined and used within one crate in order to track what is used and what is duplicated. I had to disable this. We'll have to find an alternate strategy, but I wasn't sure what was best so decided to just disable the duplicate checking for now.