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

Add a CurrentGcx type to let the deadlock handler access TyCtxt #115220

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 25, 2023

This brings back GCX_PTR (previously removed in #74969) allowing the deadlock handler access to GlobalCtxt. This fixes #111522.

r? @cjgillot

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 25, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@bors
Copy link
Contributor

bors commented Aug 30, 2023

☔ The latest upstream changes (presumably #111713) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc Zoxc force-pushed the revive-gcx-ptr branch 2 times, most recently from 2f2133c to db47bee Compare August 31, 2023 03:36
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

cc @nnethercote as you authored #74969

compiler/rustc_middle/src/ty/context/tls.rs Outdated Show resolved Hide resolved
pub unsafe fn access<R>(&self, f: impl for<'tcx> FnOnce(&'tcx GlobalCtxt<'tcx>) -> R) -> R {
let gcx_ptr: *const GlobalCtxt<'_> = self.value.lock().unwrap() as *const _;
f(unsafe { &*gcx_ptr })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the TyCtxt inside ImplicitCtxt, and always use this to access it?
Can the unsafety be removing by tweaking enter_global_context API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Unsafety could probably be removed with some blocking scheme, but our existing scheme is simple and fast.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2023
@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor

We currently have two ways to access a TyCtxt: by directly passing it as a function argument, or via ImplicitCtxt. That's already one more way than I would like, and this PR is adding a third. And look at the new code added to the deadlock handler: an unsafe block containing five levels of indentation. I've spent lots of time streamlining startup to make it as comprehensible as possible, so I don't like all this extra complexity.

@Zoxc Zoxc changed the title Revive GCX_PTR Add a CurrentGcx type to let the deadlock handler access TyCtxt Sep 25, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented Sep 25, 2023

I've changed this to use a CurrentGcx type which is passed around during setup and it not available outside context.rs after. The deadlock handler gets a copy of it. I've also changed the code to use RwLock to ensure accessing the GlobalCtxt is safe.

});
}

let current_gcx = CurrentGcx::new();
let current_gcx_ = FromDyn::from(current_gcx.clone());
let current_gcx = FromDyn::from(current_gcx);
Copy link
Contributor

Choose a reason for hiding this comment

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

This naming scheme is really hard to read. There should be more than a single trailing underscore to distinguish two similar-but-different things. A comment would also be helpful to explain the distinction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're exactly the same value. I've changed it to clone after the FromDyn construction to make that more clear.

/// the deadlock handler is not called inside such a job.
#[derive(Clone)]
pub struct CurrentGcx {
value: Lrc<Lock<Option<GcxPtr>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

GcxPtr contains a Lrc<RwLock<Option<*const ()>>>. Which means that CurrentGcx is basically a Lrc<Lock<Option<Lrc<RwLock<Option<*const ()>>>>>>.

I think this is the most complex Rust type I've ever seen. Something is very wrong here. Can it be made simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make enter behave well when called concurrently, but I think CurrentGcx would have to hold a set of GcxPtr for that to work properly. I've removed GcxPtr and instead let enter panic for concurrent usage.

@rust-log-analyzer

This comment has been minimized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 18, 2023

I don't have ideas for further improvements here, and we do want to land this before rust-lang/compiler-team#681.

@nnethercote
Copy link
Contributor

I still don't like how much the CurrentGcx has to be passed around. I suggest starting a thread on Zulip asking for ideas. Explain the problem, the constraints, the attempted solutions, and see if someone has ideas for doing this in a more simple fashion.

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 19, 2023

It doesn't matter if you like it or not. It's required for correctness.

@@ -194,27 +197,38 @@ pub(crate) fn run_in_thread_pool_with_globals<F: FnOnce() -> R + Send, R: Send>(
let registry = sync::Registry::new(threads);

if !sync::is_dyn_thread_safe() {
return run_in_thread_with_globals(edition, || {
return run_in_thread_with_globals(edition, |current_gcx| {
// Register the thread for use with the `WorkerLocal` type.
Copy link
Member

@SparrowLii SparrowLii Oct 20, 2023

Choose a reason for hiding this comment

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

Could we make CurrentGcx global so we don't pass it everywhere? I mean more global than tls.
So that we can even use it to fill ImplicitCtxt when #111522 occurs

@oli-obk
Copy link
Contributor

oli-obk commented Oct 25, 2023

It doesn't matter if you like it or not. It's required for correctness.

It would be nice to have an explanation as to why this is the only solution.

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☔ The latest upstream changes (presumably #117482) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@Zoxc any updates on this? thanks

@oli-obk
Copy link
Contributor

oli-obk commented Feb 27, 2024

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned cjgillot Feb 27, 2024
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 29, 2024

It would be nice to have an explanation as to why this is the only solution.

I didn't mean to imply it was the only solution. My point was that you shouldn't introduce bugs because you don't like the current code or you think the code with the bug is cleaner.

I did think of a potential alternative. We could maintain a list of threads which waits on queries and when a deadlock is detected, the deadlock handler would pick a thread to wake up and designate it to process cycles. That would get rid of the unsafe and the thread in the deadlock handler.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2024

@bors delegate+

r=me after a rebase.

@bors
Copy link
Contributor

bors commented Mar 25, 2024

✌️ @Zoxc, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 28, 2024

@bors r=@oli-obk

@bors
Copy link
Contributor

bors commented Mar 28, 2024

📌 Commit 9936a39 has been approved by oli-obk

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2024
@bors
Copy link
Contributor

bors commented Mar 28, 2024

⌛ Testing commit 9936a39 with merge c5e7f45...

@bors
Copy link
Contributor

bors commented Mar 28, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c5e7f45 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2024
@bors bors merged commit c5e7f45 into rust-lang:master Mar 28, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 28, 2024
@Zoxc Zoxc deleted the revive-gcx-ptr branch March 28, 2024 16:39
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c5e7f45): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.5%, 0.6%] 4
Regressions ❌
(secondary)
1.7% [1.7%, 1.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [0.5%, 0.6%] 4

Binary size

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

Bootstrap: 669.029s -> 669.68s (0.10%)
Artifact size: 315.66 MiB -> 315.70 MiB (0.01%)

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: parallel compiler: no ImplicitCtxt stored in tls
10 participants