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

use par_for_each_in in par_body_owners and collect_crate_mono_items #99457

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented Jul 19, 2022

Using par_iter in non-parallel mode will cause the entire process to abort when any iteration panics. So we can use par_for_each_in instead to make the error message consistent with parallel mode. This means that the compiler will output more error messages in some cases. This fixes the following ui tests when set parallel-compiler = true:

    [ui] src/test\ui\privacy\privacy2.rs
    [ui] src/test\ui\privacy\privacy3.rs
    [ui] src/test\ui\type_length_limit.rs

This refers to #68171

Updates #75760

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 19, 2022
@rust-highfive
Copy link
Collaborator

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022
@SparrowLii
Copy link
Member Author

cc @Zoxc @cjgillot

@rust-log-analyzer

This comment has been minimized.

@@ -146,7 +146,7 @@ cfg_if! {
t.into_iter()
}

pub fn par_for_each_in<T: IntoIterator>(t: T, for_each: impl Fn(T::Item) + Sync + Send) {
pub fn par_for_each_in<T: IntoIterator>(t: T, mut for_each: impl FnMut(T::Item) + Sync + Send) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this break the parallel compiler? The requirement is relaxed and then if we switch to parallel rustc it would fail to compile.

Copy link
Member Author

@SparrowLii SparrowLii Jul 19, 2022

Choose a reason for hiding this comment

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

It doesn't break the compiler from actual testing.

This function signature change is only required in non-parallel mode, and the one in parallel mode does not need to be changed. I think this is because the internal implementation of IntoParallelIterator does some kind of encapsulation of this(not sure).
#[Edit] See the new explanation below

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to maintain consistency in both modes, we should change Fn in the signature in parallel mode to FnMut. I'm not sure if this is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can do that, if we run it in parallel then the function would be called from multiple threads thus requiring it to be called without having exclusive access.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. So I think we can keep the Fn in the signature in parallel mode, as it doesn't cause compilation errors

Copy link
Member Author

@SparrowLii SparrowLii Jul 19, 2022

Choose a reason for hiding this comment

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

I revisited the implementation of both patterns. The place to call this interface is in collect.rs. Because the MTRef type is not a &mut reference in parallel mode, the Fn type can be used without causing a compilation error.

Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Makes sense now. Thanks for the PR!

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2022

📌 Commit e2ecb68 has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#98028 (Add E0790 as more specific variant of E0283)
 - rust-lang#99384 (use body's param-env when checking if type needs drop)
 - rust-lang#99401 (Avoid `Symbol` to `&str` conversions)
 - rust-lang#99419 (Stabilize `core::task::ready!`)
 - rust-lang#99435 (Revert "Stabilize $$ in Rust 1.63.0")
 - rust-lang#99438 (Improve suggestions for `NonZeroT` <- `T` coercion error)
 - rust-lang#99441 (Update mdbook)
 - rust-lang#99453 (:arrow_up: rust-analyzer)
 - rust-lang#99457 (use `par_for_each_in` in `par_body_owners` and `collect_crate_mono_items`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e6904fc into rust-lang:master Jul 19, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants