-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add a Rayon thread pool #50235
Add a Rayon thread pool #50235
Conversation
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #50016) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #50228) made this pull request unmergeable. Please resolve the merge conflicts. |
75d6eed
to
66e389d
Compare
Here are the changes to Rayon required, which I propose publishing in a |
☔ The latest upstream changes (presumably #50290) made this pull request unmergeable. Please resolve the merge conflicts. |
c61e880
to
d7c8e82
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 |
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.
Looks great, @Zoxc! It's very nice how well this is integrated with Rayon's abstractions.
I hope we can clean up the handling of thread-local state in the future. All this setting and forwarding is becoming a bit unwieldy.
I took a quick look at the changes to Rayon and they look sensible to me but @nikomatsakis still needs to review them before we can merge this.
src/librustc_driver/lib.rs
Outdated
@@ -198,7 +200,7 @@ pub fn run<F>(run_compiler: F) -> isize | |||
0 | |||
} | |||
|
|||
fn load_backend_from_dylib(path: &Path) -> fn() -> Box<TransCrate> { | |||
fn load_backend_from_dylib(path: &Path) -> fn() -> Box<TransCrate + sync::Send> { |
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.
Maybe Box<TransCrate + sync::Send>
could get a type alias like CrateStoreDyn
to clean things up.
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.
I've moved the creation of the thread-pool to an earlier stage, so TransCrate
does not require Send
.
src/libsyntax_pos/symbol.rs
Outdated
@@ -503,7 +503,7 @@ impl PartialOrd<InternedString> for InternedString { | |||
if self.symbol == other.symbol { | |||
return Some(Ordering::Equal); | |||
} | |||
self.with(|self_str| other.with(|other_str| self_str.partial_cmp(&other_str))) | |||
self.as_str().partial_cmp(&other.as_str()) |
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.
Did you ever get to the bottom of why this is necessary?
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.
Apparently it is this bug: #48406
Ideally we would merge the rayon changes into upstream if they seem general purpose enough. |
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 |
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 |
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 |
a8de59c
to
f43c046
Compare
@michaelwoerister approved it by IRC
This version of Rayon doesn't use coroutines. |
🔒 Merge conflict |
@bors r=michaelwoerister |
📌 Commit 4afdae6 has been approved by |
Add a Rayon thread pool r? @michaelwoerister
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister