Skip to content
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

Fix type and linkage errors with parallel codegen #26018

Merged
merged 4 commits into from
Jun 8, 2015
Merged

Conversation

nrc
Copy link
Member

@nrc nrc commented Jun 4, 2015

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 :-)

@bstrie
Copy link
Contributor

bstrie commented Jun 5, 2015

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?

@nrc
Copy link
Member Author

nrc commented Jun 5, 2015

@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.

@bstrie
Copy link
Contributor

bstrie commented Jun 5, 2015

Awesome. I'd love to see someone try the same timing with Servo.

@nrc
Copy link
Member Author

nrc commented Jun 5, 2015

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.

@jdm
Copy link
Contributor

jdm commented Jun 5, 2015

The script crate definitely fits that description, and that's our biggest offender.

@nrc
Copy link
Member Author

nrc commented Jun 5, 2015

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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should still translate the dead branch thanks to the visit from base::trans_item, so the extra visit here is unnecessary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, right!

@nikomatsakis
Copy link
Contributor

@nrc r+ modulo possible error message changes. Nice work!

@nrc
Copy link
Member Author

nrc commented Jun 7, 2015

@bors: r=@nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 7, 2015

📌 Commit 83c73e3 has been approved by @nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 7, 2015

⌛ Testing commit 83c73e3 with merge 882a762...

@bors
Copy link
Contributor

bors commented Jun 7, 2015

💔 Test failed - auto-mac-32-opt

@nrc
Copy link
Member Author

nrc commented Jun 8, 2015

@bors: r=@nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 8, 2015

📌 Commit 8d9a581 has been approved by @nikomatsakis

bors added a commit that referenced this pull request Jun 8, 2015
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 :-)
@bors
Copy link
Contributor

bors commented Jun 8, 2015

⌛ Testing commit 8d9a581 with merge 2b3ed4e...

@bors
Copy link
Contributor

bors commented Jun 8, 2015

💔 Test failed - auto-linux-32-opt

@nrc
Copy link
Member Author

nrc commented Jun 8, 2015

@bors: retry

@nrc
Copy link
Member Author

nrc commented Jun 8, 2015

That looks like a real error, but I cannot reproduce locally

bors added a commit that referenced this pull request Jun 8, 2015
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 :-)
@bors
Copy link
Contributor

bors commented Jun 8, 2015

⌛ Testing commit 8d9a581 with merge 717e883...

@alexcrichton
Copy link
Member

Ah I've seen that brand of failure before, so I think it's safe to say it's benign (ish).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants