Fix type and linkage errors with parallel codegen #26018
Conversation
|
Those speedups looks awesome! But I have to ask, has the single-threaded performance regressed compared to a compiler that doesn't have this patch? |
|
@bstrie I haven't measured, but it really shouldn't have - nearly everything here is either specific to codegen_units or fixes actual bugs. If anything, single threaded performance should have slightly increased because we no longer generate some duplicate code. |
|
Awesome. I'd love to see someone try the same timing with Servo. |
|
Me too! It has the most affect where crates are large and monolithic, i.e., like rustc used to be. We used to get 50% speedup on a stage2 build with || codegen. I believe Servo is many small crates, so the effect will probably be similar to rust or smaller. |
|
The script crate definitely fits that description, and that's our biggest offender. |
|
If anyone measures please let me know the times (and if it even works). |
| if let Some(elexpr) = els { | ||
| let mut trans = TransItemVisitor { ccx: bcx.fcx.ccx }; | ||
| trans.visit_expr(&*elexpr); | ||
| } |
There was a problem hiding this comment.
I vaguely remember that a long time ago this was required for something like:
// foo.rs
pub fn foo<T>() {
if true {
} else {
println!("test");
}
}
// bar.rs
extern crate foo;
fn main() {
foo::foo<int>();
}The monomorphization of foo in bar.rs caused something to reference the statics produced by println!, assuming they were available in foo.o (but we didn't translate them due to the constant branch).
I may also be remembering something totally bogus though! I suspect if this passes all tests it's fine anyway.
There was a problem hiding this comment.
I believe we should still translate the dead branch thanks to the visit from base::trans_item, so the extra visit here is unnecessary
|
@nrc r+ modulo possible error message changes. Nice work! |
Note that they will be visited anyway by base::trans_item, this just duplicates the items.
|
📌 Commit 83c73e3 has been approved by |
|
⌛ Testing commit 83c73e3 with merge 882a762... |
|
💔 Test failed - auto-mac-32-opt |
|
📌 Commit 8d9a581 has been approved by |
Closes #19826 r? @nikomatsakis There is still some work to do until parallel codegen is perfect - we don't pass make check due to missing the symbol for registering a macro. But we do bootstrap with codegen_units=4 and pass make check without codegen_units, so I think it should land. For the curious, make rustc-stage2 -j8: ``` codegen_units=1: real 9m18.074s user 11m59.858s sys 0m13.281s codegen_units=4: real 6m3.672s user 13m5.474s sys 0m12.146s ``` Which is a 33% speedup :-)
|
💔 Test failed - auto-linux-32-opt |
|
@bors: retry |
|
That looks like a real error, but I cannot reproduce locally |
Closes #19826 r? @nikomatsakis There is still some work to do until parallel codegen is perfect - we don't pass make check due to missing the symbol for registering a macro. But we do bootstrap with codegen_units=4 and pass make check without codegen_units, so I think it should land. For the curious, make rustc-stage2 -j8: ``` codegen_units=1: real 9m18.074s user 11m59.858s sys 0m13.281s codegen_units=4: real 6m3.672s user 13m5.474s sys 0m12.146s ``` Which is a 33% speedup :-)
|
Ah I've seen that brand of failure before, so I think it's safe to say it's benign (ish). |
Closes #19826
r? @nikomatsakis
There is still some work to do until parallel codegen is perfect - we don't pass make check due to missing the symbol for registering a macro. But we do bootstrap with codegen_units=4 and pass make check without codegen_units, so I think it should land.
For the curious, make rustc-stage2 -j8:
Which is a 33% speedup :-)