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

get rid of tcx in deadlock handler when parallel compilation #98570

Merged
merged 1 commit into from
Jul 3, 2022

Conversation

SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented Jun 27, 2022

This is a very obscure and hard-to-trace problem that affects thread scheduling. If we copy tcx to the deadlock handler thread, it will perform unpredictable behavior and cause very weird problems when executing try_collect_active_jobs(For example, the deadlock handler thread suddenly preempts the content of the blocked worker thread and executes the unknown judgment branch, like #94654).
Fortunately we can avoid this behavior by precomputing query_map. This change fixes the following ui tests failure on my environment when set parallel-compiler = true:

    [ui] src/test\ui\async-await\no-const-async.rs
    [ui] src/test\ui\infinite\infinite-struct.rs
    [ui] src/test\ui\infinite\infinite-tag-type-recursion.rs
    [ui] src/test\ui\issues\issue-3008-1.rs
    [ui] src/test\ui\issues\issue-3008-2.rs
    [ui] src/test\ui\issues\issue-32326.rs
    [ui] src/test\ui\issues\issue-57271.rs
    [ui] src/test\ui\issues\issue-72554.rs
    [ui] src/test\ui\parser\fn-header-semantic-fail.rs
    [ui] src/test\ui\union\union-nonrepresentable.rs

Updates #75760
Fixes #94654

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

r? @michaelwoerister

(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 Jun 27, 2022
@cjgillot
Copy link
Contributor

Thanks @SparrowLii. That's a great solution.
Could you remove try_collect_active_jobs from the QueryContext trait and make it an inherent method on QueryCtxt?

@cjgillot cjgillot self-assigned this Jun 27, 2022
@SparrowLii
Copy link
Member Author

SparrowLii commented Jun 28, 2022

Could you remove try_collect_active_jobs from the QueryContext trait and make it an inherent method on QueryCtxt?

Sure, made the corresponding changes.

#[Edit] It's a bit difficult to remove it from the QueryContext as try_start function call it when encounter cycle error
in serial compilation. Do we still need to do this?

@rust-log-analyzer

This comment has been minimized.

@SparrowLii
Copy link
Member Author

SparrowLii commented Jun 28, 2022

Umm.. seems that rustc_query_system still needs try_collect_active_jobs to exist in QueryContext: https://github.com/rust-lang/rust/runs/7084788737?check_suite_focus=true.

Just reverted the last change

@@ -26,7 +26,7 @@ use rustc_span::Span;
#[macro_use]
mod plumbing;
pub use plumbing::QueryCtxt;
use rustc_query_system::query::*;
pub use rustc_query_system::query::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this pub required?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to use try_collect_active_jobs and deadlock in rustc_interface::util::handle_deadlock. We can add pub use rustc_query_system::query::{deadlock, QueryContext} instead of pub the all thing here.

@cjgillot
Copy link
Contributor

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 29, 2022

📌 Commit fbca21e has been approved by cjgillot

@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 Jun 29, 2022
@Zoxc
Copy link
Contributor

Zoxc commented Jun 29, 2022

If you're going to do significant work in handle_deadlock (which is called while Rayon's sleep lock is held), you might as well run the entire deadlock handler there and avoid the thread spawning.

An alternative way to fix #94654 would be to extend WorkerLocal to also work on the deadlock handler thread, as it currently expects to only be used on Rayon's threads.

@bors
Copy link
Contributor

bors commented Jul 3, 2022

⌛ Testing commit fbca21e with merge 8c52a83...

@bors
Copy link
Contributor

bors commented Jul 3, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 8c52a83 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 3, 2022
@bors bors merged commit 8c52a83 into rust-lang:master Jul 3, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8c52a83): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.4% -2.4% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-2.0% -2.0% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.0% -2.0% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

ICE with --alt rustc: WorkerLocal can only be used on the thread pool it was created on
9 participants