Make queries thread safe #49045

Zoxc · 2018-03-15T14:18:13Z

This makes queries thread safe by removing the query stack and making queries point to their parents. Queries write to the query map when starting and cycles are detected by checking if there's already an entry in the query map. This makes cycle detection O(1) instead of O(n), where n is the size of the query stack.

This is mostly corresponds to the method I described here.

cc @rust-lang/compiler

r? @michaelwoerister

michaelwoerister · 2018-03-15T17:05:25Z

src/librustc/ty/maps/plumbing.rs

+                    let job = if let Some(job) = job {
+                        job
+                    } else {
+                        break lock;


Is there a reason why this line is not just return Ok(lock);? I think that would be clearer.

Yeah, that would be better. The break is there because this used to be part of another function.

I've fixed this.

pietroalbini · 2018-03-20T08:27:37Z

Ping from triage @michaelwoerister! The PR author pushed new commits, could you review them?

michaelwoerister · 2018-03-20T09:32:27Z

I haven't forgotten about this, I'm just busy with bug fixing the last few days.

nikomatsakis · 2018-03-20T16:41:31Z

Same here. I have some time scheduled for review later today, though I expect i'm largely going to defer to @michaelwoerister and/or @eddyb here since I don't have a ton of time right now.

nikomatsakis

Epic PR @Zoxc =)

I did a first read. I'm basically happy for now with the overall setup, which is what I expected. One obvious problem is that the code could use a lot more comments.

This is to some extent a pre-existing problem: the whole query setup is woefully underdocumented as is, but this is as good a time as any to fix that. In general, we should aim to make this stuff maximally readable.

(Once this has landed, I would like to iterate on some of the details, such as whether to use lazy or eager cycle detection, and the best way to handle blocking jobs -- but that can wait.)

nikomatsakis · 2018-03-20T18:11:39Z

src/librustc/ty/context.rs

+    use rustc_data_structures::sync::Lrc;
+
+    #[derive(Clone)]
+    pub struct ImplicitCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {


Needs documentation. What is an ImplicitCtxt?

nikomatsakis · 2018-03-20T18:12:25Z

src/librustc/ty/context.rs

@@ -1484,11 +1486,25 @@ impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {

 impl<'gcx: 'tcx, 'tcx> GlobalCtxt<'gcx> {
    /// Call the closure with a local `TyCtxt` using the given arena.
-    pub fn enter_local<F, R>(&self, arena: &'tcx DroplessArena, f: F) -> R
+    pub fn enter_local<F, R>(&self,


If we're going to reformat, let's adopt the modern style:

pub fn enter_local<F, R>( &self, arena: &'tcx DroplessArena, f: F, ) -> R where F: ... {

nikomatsakis · 2018-03-20T18:13:38Z

src/librustc/ty/context.rs

+        })
+    }
+
+    pub fn with_thread_locals<F, R>(f: F) -> R


Comment please. (I know... the original code isn't that documented either...c'est la vie)

nikomatsakis · 2018-03-20T18:14:18Z

src/librustc/ty/context.rs

+        })
+    }
+
+    pub fn enter_context<'a, 'gcx: 'tcx, 'tcx, F, R>(context: &ImplicitCtxt<'a, 'gcx, 'tcx>,


Nit: as above, let's adopt the modern style

Also, comment =)

One thing I am wondering about: why does this take an &ImplicitCtxt versus creating one locally?

Typically you'd get the current ImplicitCtxt make a copy which changes a single field (queries modify the query field only, creating an new interner would modify tcx). Then you'd use enter_context to use this new ImplicitCtxt. We don't need to take ownership of the ImplicitCtxt so we pass it by reference.

nikomatsakis · 2018-03-20T18:15:13Z

src/librustc/ty/context.rs

+        }
+    }
+
+    pub fn with_fully_related_context<'a, 'gcx, 'tcx, F, R>(tcx: TyCtxt<'a, 'gcx, 'tcx>, f: F) -> R


Comment: what is a "fully related" context?

nikomatsakis · 2018-03-20T18:18:50Z

src/librustc/ty/maps/job.rs

+}
+
+impl<'tcx> QueryJob<'tcx> {
+    pub fn new(entry: StackEntry<'tcx>,


Can you please run rustfmt on this file? <3

nikomatsakis · 2018-03-20T18:23:23Z

src/librustc/ty/maps/mod.rs

@@ -389,7 +393,7 @@ define_maps! { <'tcx>
    [] fn normalize_projection_ty: NormalizeProjectionTy(
        CanonicalProjectionGoal<'tcx>
    ) -> Result<
-        Lrc<Canonical<'tcx, QueryResult<'tcx, NormalizationResult<'tcx>>>>,
+        Lrc<Canonical<'tcx, canonical::QueryResult<'tcx, NormalizationResult<'tcx>>>>,


Hmm. We should probably rename canonical::QueryResult, but not in this PR.

nikomatsakis · 2018-03-20T18:33:04Z

src/librustc/ty/maps/plumbing.rs

+            fn try_get_lock(tcx: TyCtxt<'a, $tcx, 'lcx>,
+                            mut span: Span,
+                            key: &$K)
+                            -> Result<LockGuard<'a, QueryMap<$tcx, Self>>,


Can we make a custom enum here? This type is sort of incomprehensible. Something with named variants seems better. Like:

enum TryGetLock { NotYetStarted(Guard), // Today: Ok(guard) JobCompleted(...), // Today: Err(Ok(...)) CycleDetected(...), // Today: Err(Err...)) }

I guess it means:

Ok(guard) -- no job yet started, lock is still held

Err(Ok(...)) -- cache hit, or else we blocked on a job and it completed

Err(Err(..)) -- cycle detected

nikomatsakis · 2018-03-20T18:36:59Z

src/librustc/ty/maps/plumbing.rs

-                    profq_msg!(tcx, ProfileQueriesMsg::CacheHit);
-                    tcx.dep_graph.read_index(value.index);
-                    return Ok((&value.value).clone());
+                macro_rules! get_lock {


why is this a macro and not a function?

Oh, because it returns I guess? Hmm. OK. Maybe we can rename it get_lock_or_return or something.

nikomatsakis · 2018-03-20T18:39:27Z

src/librustc/ty/maps/plumbing.rs

+                            // Also signal the completion of the job, so waiters
+                            // will continue execution
+                            job.signal_complete();
+                        });


Zoxc · 2018-03-21T11:51:33Z

@bors try

Zoxc · 2018-03-21T14:13:26Z

@bors try

bors · 2018-03-21T14:13:38Z

⌛ Trying commit 31c0581aa0a405f94d031f8570ba2f2938ccf2d1 with merge c83390e645c0ae1d55d6f56636824a76fe084663...

bors · 2018-03-21T16:12:20Z

☀️ Test successful - status-travis
State: approved= try=True

Zoxc · 2018-03-21T16:21:11Z

@Mark-Simulacrum Can I get a perf run?

Mark-Simulacrum · 2018-03-22T16:37:34Z

Perf queued.

Mark-Simulacrum · 2018-03-23T17:53:19Z

http://perf.rust-lang.org/compare.html?start=c19264fa835a1eca86de4fd2e86a87b3919e57cf&end=c83390e645c0ae1d55d6f56636824a76fe084663&stat=instructions%3Au

…nt to their parents instead.

Zoxc · 2018-03-24T14:54:20Z

I've renamed StackEntry to QueryInfo since there's no longer a stack. I removed the track_diagnostics field of QueryJob since it was no longer used. I also added some comments.

michaelwoerister · 2018-03-25T14:42:32Z

Sorry for not getting to this yet, @Zoxc. Either we'll take a look at this together during the work week or I'll do an in-depth review on Tuesday after Easter.

michaelwoerister · 2018-04-03T12:29:46Z

OK, I finally had time to review this. The comments you added since the initial version make things a lot clearer, thanks, @Zoxc! I think we can go ahead an merge this. The basic strategy seems sound to me and a good starting point.

@nikomatsakis, are you satisfied with the comments added or do you want more? Otherwise, r=me.

michaelwoerister · 2018-04-05T13:29:49Z

I'm going to go ahead and r+ this for now. We can always add more comments later.

@bors r+

bors · 2018-04-05T13:29:50Z

📌 Commit 4f7d0fd has been approved by michaelwoerister

@michaelwoerister

Make queries thread safe This makes queries thread safe by removing the query stack and making queries point to their parents. Queries write to the query map when starting and cycles are detected by checking if there's already an entry in the query map. This makes cycle detection O(1) instead of O(n), where `n` is the size of the query stack. This is mostly corresponds to the method I described [here](https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606). cc @rust-lang/compiler r? @michaelwoerister

bors · 2018-04-05T16:38:22Z

⌛ Testing commit 4f7d0fd with merge 7222241...

@michaelwoerister

Make queries thread safe This makes queries thread safe by removing the query stack and making queries point to their parents. Queries write to the query map when starting and cycles are detected by checking if there's already an entry in the query map. This makes cycle detection O(1) instead of O(n), where `n` is the size of the query stack. This is mostly corresponds to the method I described [here](https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606). cc @rust-lang/compiler r? @michaelwoerister

bors · 2018-04-05T19:25:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 7222241 to master...

Rollup of 8 pull requests Successful merges: - #49045 (Make queries thread safe) - #49350 (Expand macros in `extern {}` blocks) - #49497 (Chalkify - Tweak `Clause` definition and HRTBs) - #49597 (proc_macro: Reorganize public API) - #49686 (typos) - #49621 - #49697 - #49705 Failed merges:

michaelwoerister · 2018-04-06T08:07:43Z

🎉

rust-highfive assigned michaelwoerister Mar 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2018

Zoxc force-pushed the tls branch 2 times, most recently from 1836db4 to 61b5c59 Compare March 15, 2018 16:45

michaelwoerister reviewed Mar 15, 2018

View reviewed changes

Zoxc force-pushed the tls branch 2 times, most recently from 37bb29d to 31c0581 Compare March 15, 2018 23:48

nikomatsakis requested changes Mar 20, 2018

View reviewed changes

pietroalbini 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 Mar 20, 2018

Make queries thread safe. Remove the query stack and make queries poi…

29a4ec0

…nt to their parents instead.

Zoxc force-pushed the tls branch 2 times, most recently from bc428e1 to fa563bd Compare March 24, 2018 05:57

Some cleanups and added comments

4f7d0fd

Zoxc force-pushed the tls branch from fa563bd to 4f7d0fd Compare March 24, 2018 14:48

shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 30, 2018

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 Apr 5, 2018

alexcrichton mentioned this pull request Apr 5, 2018

Rollup of 8 pull requests #49696

Merged

bors merged commit 4f7d0fd into rust-lang:master Apr 5, 2018

This was referenced Apr 5, 2018

Use InternedString instead of Symbol for type parameter types (2) #49695

Merged

Print query stack on ICEs #49222

Merged

Use InternedString instead of Symbol for type parameter types #49266

Closed

Make Handler more thread-safe #49349

Merged

Zoxc deleted the tls branch April 6, 2018 08:20

ishitatsuyuki mentioned this pull request Apr 19, 2018

Memory usage regression in #49045 #50073

Closed

aturon mentioned this pull request Aug 16, 2019

Audit sources of shared state across rustc #63643

Open

37 tasks

Make queries thread safe #49045

Make queries thread safe #49045

Conversation

Zoxc commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Mar 20, 2018

michaelwoerister commented Mar 20, 2018

nikomatsakis commented Mar 20, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zoxc commented Mar 21, 2018

Zoxc commented Mar 21, 2018

bors commented Mar 21, 2018

bors commented Mar 21, 2018

Zoxc commented Mar 21, 2018

Mark-Simulacrum commented Mar 22, 2018

Mark-Simulacrum commented Mar 23, 2018

Zoxc commented Mar 24, 2018

michaelwoerister commented Mar 25, 2018

michaelwoerister commented Apr 3, 2018

michaelwoerister commented Apr 5, 2018

bors commented Apr 5, 2018

bors commented Apr 5, 2018

bors commented Apr 5, 2018

michaelwoerister commented Apr 6, 2018