Skip to content

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

Merged
bors merged 1 commit intorust-lang:masterfrom
SparrowLii:deadlock
Jul 3, 2022
Merged

get rid of tcx in deadlock handler when parallel compilation#98570
bors merged 1 commit intorust-lang:masterfrom
SparrowLii:deadlock

Conversation

@SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented Jun 27, 2022 •
edited
Loading

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
Contributor

rust-highfive commented Jun 27, 2022

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
This was referenced Jun 27, 2022
Closed
Closed
@cjgillot
Copy link
Contributor

cjgillot commented Jun 27, 2022

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 •
edited
Loading

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.

Sign in to view
@SparrowLii
Copy link
Member Author

SparrowLii commented Jun 28, 2022 •
edited
Loading

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

cjgillot
cjgillot reviewed Jun 28, 2022
View reviewed changes
compiler/rustc_query_impl/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Jun 28, 2022

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

@SparrowLii SparrowLii Jun 29, 2022

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

cjgillot commented Jun 29, 2022

@bors r+ rollup=never

@bors
Copy link
Collaborator

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
Collaborator

bors commented Jul 3, 2022

⌛ Testing commit fbca21e with merge 8c52a83...

@bors
Copy link
Collaborator

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

rust-timer commented Jul 3, 2022

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

@SparrowLii SparrowLii mentioned this pull request Jul 5, 2023
Open
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

@michaelwoerister michaelwoerister

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

Milestone

1.64.0

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

@SparrowLii @rust-highfive @cjgillot @rust-log-analyzer @bors @Zoxc @rust-timer @michaelwoerister @rustbot