Add a `CurrentGcx` type to let the deadlock handler access `TyCtxt` by Zoxc · Pull Request #115220 · rust-lang/rust

Zoxc · 2023-08-25T16:28:00Z

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

r? @cjgillot

rustbot · 2023-08-25T16:28:07Z

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 · 2023-08-30T10:46:28Z

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

cjgillot

cc @nnethercote as you authored #74969

cjgillot · 2023-09-09T09:26:09Z

+    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 })
+    }


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?

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

nnethercote · 2023-09-14T04:44:42Z

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 · 2023-09-25T22:56:27Z

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.

nnethercote · 2023-09-29T07:44:32Z


+    let current_gcx = CurrentGcx::new();
+    let current_gcx_ = FromDyn::from(current_gcx.clone());
+    let current_gcx = FromDyn::from(current_gcx);


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.

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

nnethercote · 2023-09-29T07:45:09Z

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


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?

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.

Zoxc · 2023-10-18T14:31:56Z

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

nnethercote · 2023-10-19T20:12:00Z

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 · 2023-10-19T20:41:15Z

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

SparrowLii · 2023-10-20T03:40:51Z

    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.


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 · 2023-10-25T12:07:13Z

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 · 2023-11-01T14:49:33Z

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

Dylan-DPC · 2024-02-13T13:54:33Z

@Zoxc any updates on this? thanks

oli-obk · 2024-02-27T08:55:16Z

r? @oli-obk

Zoxc · 2024-02-29T23:05:21Z

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 · 2024-03-25T09:23:08Z

@bors delegate+

r=me after a rebase.

bors · 2024-03-25T09:23:10Z

✌️ @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 · 2024-03-28T12:37:26Z

@bors r=@oli-obk

bors · 2024-03-28T12:37:29Z

📌 Commit 9936a39 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-03-28T14:00:11Z

⌛ Testing commit 9936a39 with merge c5e7f45...

bors · 2024-03-28T16:21:58Z

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

rust-timer · 2024-03-28T18:46:25Z

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%)

rustbot assigned cjgillot Aug 25, 2023

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

Zoxc mentioned this pull request Aug 25, 2023

ICE: parallel compiler: deadlock detected #111521

Closed

SparrowLii mentioned this pull request Aug 30, 2023

Tracking Issue for Parallel Rustc Front-end #113349

Open

28 tasks

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

cjgillot reviewed Sep 9, 2023

View reviewed changes

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

Zoxc force-pushed the revive-gcx-ptr branch from db47bee to bfd42e9 Compare September 9, 2023 13:20

This comment has been minimized.

Sign in to view

Zoxc force-pushed the revive-gcx-ptr branch from bfd42e9 to b304272 Compare September 9, 2023 13:30

nnethercote reviewed Sep 14, 2023

View reviewed changes

Comment thread compiler/rustc_middle/src/ty/context/tls.rs Outdated

Zoxc force-pushed the revive-gcx-ptr branch from b304272 to 3bb7aa3 Compare September 25, 2023 22:50

Zoxc changed the title ~~Revive GCX_PTR~~ Add a CurrentGcx type to let the deadlock handler access TyCtxt Sep 25, 2023

nnethercote reviewed Sep 29, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

Zoxc force-pushed the revive-gcx-ptr branch from 9c39555 to ccb5018 Compare October 1, 2023 06:20

SparrowLii reviewed Oct 20, 2023

View reviewed changes

lqd mentioned this pull request Nov 1, 2023

enable parallel rustc front end in nightly builds #117435

Merged

rustbot assigned oli-obk and unassigned cjgillot Feb 27, 2024

oli-obk mentioned this pull request Mar 4, 2024

Give deadlock handler access to GlobalCtxt #121971

Closed

Add a CurrentGcx type to let the deadlock handler access TyCtxt

9936a39

Zoxc force-pushed the revive-gcx-ptr branch from ccb5018 to 9936a39 Compare March 27, 2024 10:44

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 added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2024

bors merged commit c5e7f45 into rust-lang:master Mar 28, 2024

rustbot added this to the 1.79.0 milestone Mar 28, 2024

Zoxc deleted the revive-gcx-ptr branch March 28, 2024 16:39

bors mentioned this pull request Mar 28, 2024

Move SourceMap initialization #122811

Merged

Uh oh!

Conversation

Zoxc commented Aug 25, 2023

Uh oh!

rustbot commented Aug 25, 2023

Uh oh!

bors commented Aug 30, 2023

Uh oh!

cjgillot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

nnethercote commented Sep 14, 2023

Uh oh!

Zoxc commented Sep 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Zoxc commented Oct 18, 2023

Uh oh!

nnethercote commented Oct 19, 2023

Uh oh!

Zoxc commented Oct 19, 2023

Uh oh!

SparrowLii Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 25, 2023

Uh oh!

bors commented Nov 1, 2023

Uh oh!

Dylan-DPC commented Feb 13, 2024

Uh oh!

oli-obk commented Feb 27, 2024

Uh oh!

Zoxc commented Feb 29, 2024

Uh oh!

oli-obk commented Mar 25, 2024

Uh oh!

bors commented Mar 25, 2024

Uh oh!

Zoxc commented Mar 28, 2024

Uh oh!

bors commented Mar 28, 2024

Uh oh!

bors commented Mar 28, 2024

Uh oh!

bors commented Mar 28, 2024

Uh oh!

rust-timer commented Mar 28, 2024

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

SparrowLii Oct 20, 2023 •

edited

Loading