From 5e35fadddb8b91f6f7d47fb19e635d1fb1b2a965 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 2 Nov 2020 22:17:29 +0100 Subject: [PATCH 01/13] Move dep_graph checking into try_load_from_disk_and_cache_in_memory. --- .../rustc_query_system/src/query/plumbing.rs | 35 +++++++------------ 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index a7511846cadb6..c3627910341dd 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -2,8 +2,7 @@ //! generate the actual methods on tcx which find and execute the provider, //! manage the caches, and so forth. -use crate::dep_graph::{DepContext, DepKind, DepNode, DepNodeParams}; -use crate::dep_graph::{DepNodeIndex, SerializedDepNodeIndex}; +use crate::dep_graph::{DepContext, DepKind, DepNode, DepNodeIndex, DepNodeParams}; use crate::query::caches::QueryCache; use crate::query::config::{QueryDescription, QueryVtable, QueryVtableExt}; use crate::query::job::{ @@ -496,21 +495,7 @@ where // promoted to the current session during // `try_mark_green()`, so we can ignore them here. let loaded = tcx.start_query(job.id, None, || { - let marked = dep_graph.try_mark_green_and_read(tcx, &dep_node); - marked.map(|(prev_dep_node_index, dep_node_index)| { - ( - load_from_disk_and_cache_in_memory( - tcx, - key.clone(), - prev_dep_node_index, - dep_node_index, - &dep_node, - query, - compute, - ), - dep_node_index, - ) - }) + try_load_from_disk_and_cache_in_memory(tcx, key.clone(), &dep_node, query, compute) }); if let Some((result, dep_node_index)) = loaded { return job.complete(result, dep_node_index); @@ -522,21 +507,23 @@ where result } -fn load_from_disk_and_cache_in_memory( +fn try_load_from_disk_and_cache_in_memory( tcx: CTX, key: K, - prev_dep_node_index: SerializedDepNodeIndex, - dep_node_index: DepNodeIndex, dep_node: &DepNode, query: &QueryVtable, compute: fn(CTX::DepContext, K) -> V, -) -> V +) -> Option<(V, DepNodeIndex)> where CTX: QueryContext, + V: Debug, { // Note this function can be called concurrently from the same query // We must ensure that this is handled correctly. + let (prev_dep_node_index, dep_node_index) = + tcx.dep_context().dep_graph().try_mark_green_and_read(tcx, &dep_node)?; + debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); // First we try to load the result from the on-disk cache. @@ -558,7 +545,7 @@ where None }; - if let Some(result) = result { + let result = if let Some(result) = result { // If `-Zincremental-verify-ich` is specified, re-hash results from // the cache and make sure that they have the expected fingerprint. if unlikely!(tcx.dep_context().sess().opts.debugging_opts.incremental_verify_ich) { @@ -588,7 +575,9 @@ where incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query); result - } + }; + + Some((result, dep_node_index)) } fn incremental_verify_ich( From 0edc775b90a6892a37ec4d41f72417fc379e67db Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 2 Nov 2020 22:36:47 +0100 Subject: [PATCH 02/13] Only clone key when needed. --- compiler/rustc_query_system/src/query/plumbing.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index c3627910341dd..a21b033693418 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -495,7 +495,7 @@ where // promoted to the current session during // `try_mark_green()`, so we can ignore them here. let loaded = tcx.start_query(job.id, None, || { - try_load_from_disk_and_cache_in_memory(tcx, key.clone(), &dep_node, query, compute) + try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) }); if let Some((result, dep_node_index)) = loaded { return job.complete(result, dep_node_index); @@ -509,12 +509,13 @@ where fn try_load_from_disk_and_cache_in_memory( tcx: CTX, - key: K, + key: &K, dep_node: &DepNode, query: &QueryVtable, compute: fn(CTX::DepContext, K) -> V, ) -> Option<(V, DepNodeIndex)> where + K: Clone, CTX: QueryContext, V: Debug, { @@ -527,7 +528,7 @@ where debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); // First we try to load the result from the on-disk cache. - let result = if query.cache_on_disk(tcx, &key, None) { + let result = if query.cache_on_disk(tcx, key, None) { let prof_timer = tcx.dep_context().profiler().incr_cache_loading(); let result = query.try_load_from_disk(tcx, prev_dep_node_index); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); @@ -559,7 +560,8 @@ where let prof_timer = tcx.dep_context().profiler().query_provider(); // The dep-graph for this computation is already in-place. - let result = tcx.dep_context().dep_graph().with_ignore(|| compute(*tcx.dep_context(), key)); + let result = + tcx.dep_context().dep_graph().with_ignore(|| compute(*tcx.dep_context(), key.clone())); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); From cd1cb3449e3c92f6fbcd907d58b3009b1067d7f0 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 12 May 2021 08:50:03 +0200 Subject: [PATCH 03/13] Simplify control flow. --- .../rustc_query_system/src/query/plumbing.rs | 61 +++++++++---------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index a21b033693418..55739cbf1d8ac 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -528,7 +528,8 @@ where debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); // First we try to load the result from the on-disk cache. - let result = if query.cache_on_disk(tcx, key, None) { + // Some things are never cached on disk. + if query.cache_on_disk(tcx, key, None) { let prof_timer = tcx.dep_context().profiler().incr_cache_loading(); let result = query.try_load_from_disk(tcx, prev_dep_node_index); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); @@ -540,44 +541,38 @@ where "missing on-disk cache entry for {:?}", dep_node ); - result - } else { - // Some things are never cached on disk. - None - }; - let result = if let Some(result) = result { - // If `-Zincremental-verify-ich` is specified, re-hash results from - // the cache and make sure that they have the expected fingerprint. - if unlikely!(tcx.dep_context().sess().opts.debugging_opts.incremental_verify_ich) { - incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query); - } + if let Some(result) = result { + // If `-Zincremental-verify-ich` is specified, re-hash results from + // the cache and make sure that they have the expected fingerprint. + if unlikely!(tcx.dep_context().sess().opts.debugging_opts.incremental_verify_ich) { + incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query); + } - result - } else { - // We could not load a result from the on-disk cache, so - // recompute. - let prof_timer = tcx.dep_context().profiler().query_provider(); + return Some((result, dep_node_index)); + } + } - // The dep-graph for this computation is already in-place. - let result = - tcx.dep_context().dep_graph().with_ignore(|| compute(*tcx.dep_context(), key.clone())); + // We could not load a result from the on-disk cache, so + // recompute. + let prof_timer = tcx.dep_context().profiler().query_provider(); - prof_timer.finish_with_query_invocation_id(dep_node_index.into()); + // The dep-graph for this computation is already in-place. + let result = + tcx.dep_context().dep_graph().with_ignore(|| compute(*tcx.dep_context(), key.clone())); - // Verify that re-running the query produced a result with the expected hash - // This catches bugs in query implementations, turning them into ICEs. - // For example, a query might sort its result by `DefId` - since `DefId`s are - // not stable across compilation sessions, the result could get up getting sorted - // in a different order when the query is re-run, even though all of the inputs - // (e.g. `DefPathHash` values) were green. - // - // See issue #82920 for an example of a miscompilation that would get turned into - // an ICE by this check - incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query); + prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - result - }; + // Verify that re-running the query produced a result with the expected hash + // This catches bugs in query implementations, turning them into ICEs. + // For example, a query might sort its result by `DefId` - since `DefId`s are + // not stable across compilation sessions, the result could get up getting sorted + // in a different order when the query is re-run, even though all of the inputs + // (e.g. `DefPathHash` values) were green. + // + // See issue #82920 for an example of a miscompilation that would get turned into + // an ICE by this check + incremental_verify_ich(*tcx.dep_context(), &result, dep_node, query); Some((result, dep_node_index)) } From c3bf3969d408f46881908336f9e3a8721601abc4 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 2 Nov 2020 23:09:03 +0100 Subject: [PATCH 04/13] Move assertion inwards. `with_taks_impl` is only called from `with_eval_always_task` and `with_task` . The former is only used in query invocation, while the latter is also used to start the `tcx` and to trigger codegen. This move should not change significantly the number of calls to this assertion. --- .../rustc_query_system/src/dep_graph/graph.rs | 21 ++++++++++++++++--- .../rustc_query_system/src/query/plumbing.rs | 14 ------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 9c3dad8bd6349..21f9d51e7af30 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -11,6 +11,7 @@ use rustc_serialize::opaque::{FileEncodeResult, FileEncoder}; use parking_lot::Mutex; use smallvec::{smallvec, SmallVec}; use std::collections::hash_map::Entry; +use std::fmt::Debug; use std::hash::Hash; use std::marker::PhantomData; use std::sync::atomic::Ordering::Relaxed; @@ -208,7 +209,7 @@ impl DepGraph { /// `arg` parameter. /// /// [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/incremental-compilation.html - pub fn with_task, A, R>( + pub fn with_task, A: Debug, R>( &self, key: DepNode, cx: Ctxt, @@ -234,7 +235,7 @@ impl DepGraph { ) } - fn with_task_impl, A, R>( + fn with_task_impl, A: Debug, R>( &self, key: DepNode, cx: Ctxt, @@ -244,6 +245,20 @@ impl DepGraph { hash_result: impl FnOnce(&mut Ctxt::StableHashingContext, &R) -> Option, ) -> (R, DepNodeIndex) { if let Some(ref data) = self.data { + // If the following assertion triggers, it can have two reasons: + // 1. Something is wrong with DepNode creation, either here or + // in `DepGraph::try_mark_green()`. + // 2. Two distinct query keys get mapped to the same `DepNode` + // (see for example #48923). + assert!( + !self.dep_node_exists(&key), + "forcing query with already existing `DepNode`\n\ + - query-key: {:?}\n\ + - dep-node: {:?}", + arg, + key + ); + let dcx = cx.dep_context(); let task_deps = create_task(key).map(Lock::new); let result = K::with_deps(task_deps.as_ref(), || task(cx, arg)); @@ -359,7 +374,7 @@ impl DepGraph { /// Executes something within an "eval-always" task which is a task /// that runs whenever anything changes. - pub fn with_eval_always_task, A, R>( + pub fn with_eval_always_task, A: Debug, R>( &self, key: DepNode, cx: Ctxt, diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 55739cbf1d8ac..86097042da016 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -646,20 +646,6 @@ where C: QueryCache, CTX: QueryContext, { - // If the following assertion triggers, it can have two reasons: - // 1. Something is wrong with DepNode creation, either here or - // in `DepGraph::try_mark_green()`. - // 2. Two distinct query keys get mapped to the same `DepNode` - // (see for example #48923). - assert!( - !tcx.dep_context().dep_graph().dep_node_exists(&dep_node), - "forcing query with already existing `DepNode`\n\ - - query-key: {:?}\n\ - - dep-node: {:?}", - key, - dep_node - ); - let prof_timer = tcx.dep_context().profiler().query_provider(); let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { From 45d6decc19b419448961fdb5ba3e062c9267a393 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 30 May 2021 10:48:48 +0200 Subject: [PATCH 05/13] Remove try_mark_green_and_read. --- .../rustc_query_system/src/dep_graph/graph.rs | 15 ++------------- compiler/rustc_query_system/src/query/plumbing.rs | 9 ++++++--- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 21f9d51e7af30..16fca7a3cd958 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -499,22 +499,11 @@ impl DepGraph { None } - /// Try to read a node index for the node dep_node. + /// Try to mark a node index for the node dep_node. + /// /// A node will have an index, when it's already been marked green, or when we can mark it /// green. This function will mark the current task as a reader of the specified node, when /// a node index can be found for that node. - pub fn try_mark_green_and_read>( - &self, - tcx: Ctxt, - dep_node: &DepNode, - ) -> Option<(SerializedDepNodeIndex, DepNodeIndex)> { - self.try_mark_green(tcx, dep_node).map(|(prev_index, dep_node_index)| { - debug_assert!(self.is_green(&dep_node)); - self.read_index(dep_node_index); - (prev_index, dep_node_index) - }) - } - pub fn try_mark_green>( &self, tcx: Ctxt, diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 86097042da016..9f89f6f530910 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -523,7 +523,8 @@ where // We must ensure that this is handled correctly. let (prev_dep_node_index, dep_node_index) = - tcx.dep_context().dep_graph().try_mark_green_and_read(tcx, &dep_node)?; + tcx.dep_context().dep_graph().try_mark_green(tcx, &dep_node)?; + tcx.dep_context().dep_graph().read_index(dep_node_index); debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); @@ -725,9 +726,10 @@ where let dep_node = query.to_dep_node(*tcx.dep_context(), key); - match tcx.dep_context().dep_graph().try_mark_green_and_read(tcx, &dep_node) { + let dep_graph = tcx.dep_context().dep_graph(); + match dep_graph.try_mark_green(tcx, &dep_node) { None => { - // A None return from `try_mark_green_and_read` means that this is either + // A None return from `try_mark_green` means that this is either // a new dep node or that the dep node has already been marked red. // Either way, we can't call `dep_graph.read()` as we don't have the // DepNodeIndex. We must invoke the query itself. The performance cost @@ -736,6 +738,7 @@ where true } Some((_, dep_node_index)) => { + dep_graph.read_index(dep_node_index); tcx.dep_context().profiler().query_cache_hit(dep_node_index.into()); false } From 283a8e14453c9f1532087caa1aa08743c8f4c58d Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 2 Nov 2020 21:27:36 +0100 Subject: [PATCH 06/13] Make all query forcing go through try_execute_query. try_execute_query is now able to centralize the path for query get/ensure/force. try_execute_query now takes the dep_node as a parameter, so it can accommodate `force`. This dep_node is an Option to avoid computing it in the `get` fast path. try_execute_query now returns both the result and the dep_node_index to allow the caller to handle the dep graph. The caller is responsible for marking the dependency. --- .../rustc_query_system/src/query/plumbing.rs | 71 ++++++++----------- 1 file changed, 31 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 9f89f6f530910..82392dee16544 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -427,12 +427,13 @@ fn try_execute_query( span: Span, key: C::Key, lookup: QueryLookup, + dep_node: Option>, query: &QueryVtable, compute: fn(CTX::DepContext, C::Key) -> C::Value, -) -> C::Stored +) -> (C::Stored, Option) where C: QueryCache, - C::Key: DepNodeParams, + C::Key: Clone + DepNodeParams, CTX: QueryContext, { let job = match JobOwner::<'_, CTX::DepKind, C>::try_start( @@ -445,11 +446,10 @@ where query, ) { TryGetJob::NotYetStarted(job) => job, - TryGetJob::Cycle(result) => return result, + TryGetJob::Cycle(result) => return (result, None), #[cfg(parallel_compiler)] TryGetJob::JobCompleted((v, index)) => { - tcx.dep_context().dep_graph().read_index(index); - return v; + return (v, Some(index)); } }; @@ -461,10 +461,11 @@ where let result = tcx.start_query(job.id, None, || compute(*tcx.dep_context(), key)); let dep_node_index = dep_graph.next_virtual_depnode_index(); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - return job.complete(result, dep_node_index); + let result = job.complete(result, dep_node_index); + return (result, None); } - if query.anon { + let (result, dep_node_index) = if query.anon { let prof_timer = tcx.dep_context().profiler().query_provider(); let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { @@ -477,20 +478,21 @@ where prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - dep_graph.read_index(dep_node_index); - let side_effects = QuerySideEffects { diagnostics }; if unlikely!(!side_effects.is_empty()) { tcx.store_side_effects_for_anon_node(dep_node_index, side_effects); } - return job.complete(result, dep_node_index); - } - - let dep_node = query.to_dep_node(*tcx.dep_context(), &key); - - if !query.eval_always { + let result = job.complete(result, dep_node_index); + (result, dep_node_index) + } else if query.eval_always { + // `to_dep_node` is expensive for some `DepKind`s. + let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); + force_query_with_job(tcx, key, job, dep_node, query, compute) + } else { + // `to_dep_node` is expensive for some `DepKind`s. + let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); // The diagnostics for this query will be // promoted to the current session during // `try_mark_green()`, so we can ignore them here. @@ -498,13 +500,13 @@ where try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) }); if let Some((result, dep_node_index)) = loaded { - return job.complete(result, dep_node_index); + let result = job.complete(result, dep_node_index); + (result, dep_node_index) + } else { + force_query_with_job(tcx, key, job, dep_node, query, compute) } - } - - let (result, dep_node_index) = force_query_with_job(tcx, key, job, dep_node, query, compute); - dep_graph.read_index(dep_node_index); - result + }; + (result, Some(dep_node_index)) } fn try_load_from_disk_and_cache_in_memory( @@ -524,7 +526,6 @@ where let (prev_dep_node_index, dep_node_index) = tcx.dep_context().dep_graph().try_mark_green(tcx, &dep_node)?; - tcx.dep_context().dep_graph().read_index(dep_node_index); debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); @@ -700,7 +701,12 @@ where C: QueryCache, C::Key: DepNodeParams, { - try_execute_query(tcx, state, cache, span, key, lookup, query, compute) + let (result, dep_node_index) = + try_execute_query(tcx, state, cache, span, key, lookup, None, query, compute); + if let Some(dep_node_index) = dep_node_index { + tcx.dep_context().dep_graph().read_index(dep_node_index) + } + result } /// Ensure that either this query has all green inputs or been executed. @@ -779,23 +785,8 @@ where Err(lookup) => lookup, }; - let job = match JobOwner::<'_, CTX::DepKind, C>::try_start( - tcx, - state, - cache, - DUMMY_SP, - key.clone(), - lookup, - query, - ) { - TryGetJob::NotYetStarted(job) => job, - TryGetJob::Cycle(_) => return true, - #[cfg(parallel_compiler)] - TryGetJob::JobCompleted(_) => return true, - }; - - force_query_with_job(tcx, key, job, dep_node, query, compute); - + let _ = + try_execute_query(tcx, state, cache, DUMMY_SP, key, lookup, Some(dep_node), query, compute); true } From 13d4eb92b8894c54dd3f35c4bc362d3d51008b76 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 30 Dec 2020 22:07:42 +0100 Subject: [PATCH 07/13] Do not compute the dep_node twice. --- .../rustc_query_system/src/query/plumbing.rs | 54 ++++++++----------- 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 82392dee16544..aabe9973111d4 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -685,30 +685,6 @@ where (result, dep_node_index) } -#[inline(never)] -fn get_query_impl( - tcx: CTX, - state: &QueryState, - cache: &QueryCacheStore, - span: Span, - key: C::Key, - lookup: QueryLookup, - query: &QueryVtable, - compute: fn(CTX::DepContext, C::Key) -> C::Value, -) -> C::Stored -where - CTX: QueryContext, - C: QueryCache, - C::Key: DepNodeParams, -{ - let (result, dep_node_index) = - try_execute_query(tcx, state, cache, span, key, lookup, None, query, compute); - if let Some(dep_node_index) = dep_node_index { - tcx.dep_context().dep_graph().read_index(dep_node_index) - } - result -} - /// Ensure that either this query has all green inputs or been executed. /// Executing `query::ensure(D)` is considered a read of the dep-node `D`. /// Returns true if the query should still run. @@ -718,13 +694,17 @@ where /// /// Note: The optimization is only available during incr. comp. #[inline(never)] -fn ensure_must_run(tcx: CTX, key: &K, query: &QueryVtable) -> bool +fn ensure_must_run( + tcx: CTX, + key: &K, + query: &QueryVtable, +) -> (bool, Option>) where K: crate::dep_graph::DepNodeParams, CTX: QueryContext, { if query.eval_always { - return true; + return (true, None); } // Ensuring an anonymous query makes no sense @@ -741,12 +721,12 @@ where // DepNodeIndex. We must invoke the query itself. The performance cost // this introduces should be negligible as we'll immediately hit the // in-memory cache, or another query down the line will. - true + (true, Some(dep_node)) } Some((_, dep_node_index)) => { dep_graph.read_index(dep_node_index); tcx.dep_context().profiler().query_cache_hit(dep_node_index.into()); - false + (false, None) } } } @@ -808,25 +788,33 @@ where CTX: QueryContext, { let query = &Q::VTABLE; - if let QueryMode::Ensure = mode { - if !ensure_must_run(tcx, &key, query) { + let dep_node = if let QueryMode::Ensure = mode { + let (must_run, dep_node) = ensure_must_run(tcx, &key, query); + if !must_run { return None; } - } + dep_node + } else { + None + }; debug!("ty::query::get_query<{}>(key={:?}, span={:?})", Q::NAME, key, span); let compute = Q::compute_fn(tcx, &key); - let value = get_query_impl( + let (result, dep_node_index) = try_execute_query( tcx, Q::query_state(tcx), Q::query_cache(tcx), span, key, lookup, + dep_node, query, compute, ); - Some(value) + if let Some(dep_node_index) = dep_node_index { + tcx.dep_context().dep_graph().read_index(dep_node_index) + } + Some(result) } pub fn force_query(tcx: CTX, dep_node: &DepNode) -> bool From d2304008c1ea8985f21e98ecdba3108611e8786d Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 30 Dec 2020 22:08:57 +0100 Subject: [PATCH 08/13] Complete job outside of force_query_with_job. --- .../rustc_query_system/src/query/plumbing.rs | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index aabe9973111d4..fbf4e8feac0b0 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -484,12 +484,11 @@ where tcx.store_side_effects_for_anon_node(dep_node_index, side_effects); } - let result = job.complete(result, dep_node_index); (result, dep_node_index) } else if query.eval_always { // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - force_query_with_job(tcx, key, job, dep_node, query, compute) + force_query_with_job(tcx, key, job.id, dep_node, query, compute) } else { // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); @@ -500,12 +499,12 @@ where try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) }); if let Some((result, dep_node_index)) = loaded { - let result = job.complete(result, dep_node_index); (result, dep_node_index) } else { - force_query_with_job(tcx, key, job, dep_node, query, compute) + force_query_with_job(tcx, key, job.id, dep_node, query, compute) } }; + let result = job.complete(result, dep_node_index); (result, Some(dep_node_index)) } @@ -636,22 +635,22 @@ fn incremental_verify_ich( } } -fn force_query_with_job( +fn force_query_with_job( tcx: CTX, - key: C::Key, - job: JobOwner<'_, CTX::DepKind, C>, + key: K, + job_id: QueryJobId, dep_node: DepNode, - query: &QueryVtable, - compute: fn(CTX::DepContext, C::Key) -> C::Value, -) -> (C::Stored, DepNodeIndex) + query: &QueryVtable, + compute: fn(CTX::DepContext, K) -> V, +) -> (V, DepNodeIndex) where - C: QueryCache, CTX: QueryContext, + K: Debug, { let prof_timer = tcx.dep_context().profiler().query_provider(); let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { - tcx.start_query(job.id, diagnostics, || { + tcx.start_query(job_id, diagnostics, || { if query.eval_always { tcx.dep_context().dep_graph().with_eval_always_task( dep_node, @@ -680,8 +679,6 @@ where tcx.store_side_effects(dep_node_index, side_effects); } - let result = job.complete(result, dep_node_index); - (result, dep_node_index) } From 307aacaf05419fbd45c9c5c428cb1004801f455b Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 11 May 2021 20:12:52 +0200 Subject: [PATCH 09/13] Decouple JobOwner from cache. --- compiler/rustc_query_system/src/query/job.rs | 2 + .../rustc_query_system/src/query/plumbing.rs | 137 ++++++++---------- 2 files changed, 66 insertions(+), 73 deletions(-) diff --git a/compiler/rustc_query_system/src/query/job.rs b/compiler/rustc_query_system/src/query/job.rs index a967670280ff2..8b338a0b1d1e2 100644 --- a/compiler/rustc_query_system/src/query/job.rs +++ b/compiler/rustc_query_system/src/query/job.rs @@ -143,6 +143,8 @@ impl QueryJobId where D: Copy + Clone + Eq + Hash, { + #[cold] + #[inline(never)] pub(super) fn find_cycle_in_stack( &self, query_map: QueryMap, diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index fbf4e8feac0b0..204c10e8e990c 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -12,12 +12,12 @@ use crate::query::{QueryContext, QueryMap, QuerySideEffects, QueryStackFrame}; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::{FxHashMap, FxHasher}; +#[cfg(parallel_compiler)] +use rustc_data_structures::profiling::TimingGuard; use rustc_data_structures::sharded::{get_shard_index_by_hash, Sharded}; use rustc_data_structures::sync::{Lock, LockGuard}; use rustc_data_structures::thin_vec::ThinVec; -#[cfg(not(parallel_compiler))] -use rustc_errors::DiagnosticBuilder; -use rustc_errors::{Diagnostic, FatalError}; +use rustc_errors::{Diagnostic, DiagnosticBuilder, FatalError}; use rustc_span::{Span, DUMMY_SP}; use std::cell::Cell; use std::collections::hash_map::Entry; @@ -147,24 +147,21 @@ impl Default for QueryState { /// A type representing the responsibility to execute the job in the `job` field. /// This will poison the relevant query if dropped. -struct JobOwner<'tcx, D, C> +struct JobOwner<'tcx, D, K> where D: Copy + Clone + Eq + Hash, - C: QueryCache, + K: Eq + Hash + Clone, { - state: &'tcx QueryState, - cache: &'tcx QueryCacheStore, - key: C::Key, + state: &'tcx QueryState, + key: K, id: QueryJobId, } #[cold] #[inline(never)] -#[cfg(not(parallel_compiler))] fn mk_cycle( tcx: CTX, - root: QueryJobId, - span: Span, + error: CycleError, handle_cycle_error: fn(CTX, DiagnosticBuilder<'_>) -> V, cache: &dyn crate::query::QueryStorage, ) -> R @@ -173,20 +170,15 @@ where V: std::fmt::Debug, R: Clone, { - let error: CycleError = root.find_cycle_in_stack( - tcx.try_collect_active_jobs().unwrap(), - &tcx.current_query_job(), - span, - ); let error = report_cycle(tcx.dep_context().sess(), error); let value = handle_cycle_error(tcx, error); cache.store_nocache(value) } -impl<'tcx, D, C> JobOwner<'tcx, D, C> +impl<'tcx, D, K> JobOwner<'tcx, D, K> where D: Copy + Clone + Eq + Hash, - C: QueryCache, + K: Eq + Hash + Clone, { /// Either gets a `JobOwner` corresponding the query, allowing us to /// start executing the query, or returns with the result of the query. @@ -198,14 +190,13 @@ where /// for some compile-time benchmarks. #[inline(always)] fn try_start<'b, CTX>( - tcx: CTX, - state: &'b QueryState, - cache: &'b QueryCacheStore, + tcx: &'b CTX, + state: &'b QueryState, span: Span, - key: C::Key, + key: K, lookup: QueryLookup, - query: &QueryVtable, - ) -> TryGetJob<'b, CTX::DepKind, C> + dep_kind: CTX::DepKind, + ) -> TryGetJob<'b, CTX::DepKind, K> where CTX: QueryContext, { @@ -226,26 +217,24 @@ where let key = entry.key().clone(); entry.insert(QueryResult::Started(job)); - let global_id = QueryJobId::new(id, shard, query.dep_kind); - let owner = JobOwner { state, cache, id: global_id, key }; + let global_id = QueryJobId::new(id, shard, dep_kind); + let owner = JobOwner { state, id: global_id, key }; return TryGetJob::NotYetStarted(owner); } Entry::Occupied(mut entry) => { match entry.get_mut() { #[cfg(not(parallel_compiler))] QueryResult::Started(job) => { - let id = QueryJobId::new(job.id, shard, query.dep_kind); + let id = QueryJobId::new(job.id, shard, dep_kind); drop(state_lock); // If we are single-threaded we know that we have cycle error, // so we just return the error. - return TryGetJob::Cycle(mk_cycle( - tcx, - id, + return TryGetJob::Cycle(id.find_cycle_in_stack( + tcx.try_collect_active_jobs().unwrap(), + &tcx.current_query_job(), span, - query.handle_cycle_error, - &cache.cache, )); } #[cfg(parallel_compiler)] @@ -257,7 +246,6 @@ where // Get the latch out let latch = job.latch(); - let key = entry.key().clone(); drop(state_lock); @@ -265,30 +253,10 @@ where // thread. let result = latch.wait_on(tcx.current_query_job(), span); - if let Err(cycle) = result { - let cycle = report_cycle(tcx.dep_context().sess(), cycle); - let value = (query.handle_cycle_error)(tcx, cycle); - let value = cache.cache.store_nocache(value); - return TryGetJob::Cycle(value); + match result { + Ok(()) => TryGetJob::JobCompleted(query_blocked_prof_timer), + Err(cycle) => TryGetJob::Cycle(cycle), } - - let cached = cache - .cache - .lookup(cache, &key, |value, index| { - if unlikely!(tcx.dep_context().profiler().enabled()) { - tcx.dep_context().profiler().query_cache_hit(index.into()); - } - #[cfg(debug_assertions)] - { - cache.cache_hits.fetch_add(1, Ordering::Relaxed); - } - (value.clone(), index) - }) - .unwrap_or_else(|_| panic!("value must be in cache after waiting")); - - query_blocked_prof_timer.finish_with_query_invocation_id(cached.1.into()); - - return TryGetJob::JobCompleted(cached); } QueryResult::Poisoned => FatalError.raise(), } @@ -298,11 +266,18 @@ where /// Completes the query by updating the query cache with the `result`, /// signals the waiter and forgets the JobOwner, so it won't poison the query - fn complete(self, result: C::Value, dep_node_index: DepNodeIndex) -> C::Stored { + fn complete( + self, + cache: &QueryCacheStore, + result: C::Value, + dep_node_index: DepNodeIndex, + ) -> C::Stored + where + C: QueryCache, + { // We can move out of `self` here because we `mem::forget` it below let key = unsafe { ptr::read(&self.key) }; let state = self.state; - let cache = self.cache; // Forget ourself so our destructor won't poison the query mem::forget(self); @@ -338,10 +313,10 @@ where (result, diagnostics.into_inner()) } -impl<'tcx, D, C> Drop for JobOwner<'tcx, D, C> +impl<'tcx, D, K> Drop for JobOwner<'tcx, D, K> where D: Copy + Clone + Eq + Hash, - C: QueryCache, + K: Eq + Hash + Clone, { #[inline(never)] #[cold] @@ -372,22 +347,22 @@ pub(crate) struct CycleError { } /// The result of `try_start`. -enum TryGetJob<'tcx, D, C> +enum TryGetJob<'tcx, D, K> where D: Copy + Clone + Eq + Hash, - C: QueryCache, + K: Eq + Hash + Clone, { /// The query is not yet started. Contains a guard to the cache eventually used to start it. - NotYetStarted(JobOwner<'tcx, D, C>), + NotYetStarted(JobOwner<'tcx, D, K>), /// The query was already completed. /// Returns the result of the query and its dep-node index /// if it succeeded or a cycle error if it failed. #[cfg(parallel_compiler)] - JobCompleted((C::Stored, DepNodeIndex)), + JobCompleted(TimingGuard<'tcx>), /// Trying to execute the query resulted in a cycle. - Cycle(C::Stored), + Cycle(CycleError), } /// Checks if the query is already computed and in the cache. @@ -436,19 +411,35 @@ where C::Key: Clone + DepNodeParams, CTX: QueryContext, { - let job = match JobOwner::<'_, CTX::DepKind, C>::try_start( - tcx, + let job = match JobOwner::<'_, CTX::DepKind, C::Key>::try_start( + &tcx, state, - cache, span, key.clone(), lookup, - query, + query.dep_kind, ) { TryGetJob::NotYetStarted(job) => job, - TryGetJob::Cycle(result) => return (result, None), + TryGetJob::Cycle(error) => { + let result = mk_cycle(tcx, error, query.handle_cycle_error, &cache.cache); + return (result, None); + } #[cfg(parallel_compiler)] - TryGetJob::JobCompleted((v, index)) => { + TryGetJob::JobCompleted(query_blocked_prof_timer) => { + let (v, index) = cache + .cache + .lookup(cache, &key, |value, index| (value.clone(), index)) + .unwrap_or_else(|_| panic!("value must be in cache after waiting")); + + if unlikely!(tcx.dep_context().profiler().enabled()) { + tcx.dep_context().profiler().query_cache_hit(index.into()); + } + #[cfg(debug_assertions)] + { + cache.cache_hits.fetch_add(1, Ordering::Relaxed); + } + query_blocked_prof_timer.finish_with_query_invocation_id(index.into()); + return (v, Some(index)); } }; @@ -461,7 +452,7 @@ where let result = tcx.start_query(job.id, None, || compute(*tcx.dep_context(), key)); let dep_node_index = dep_graph.next_virtual_depnode_index(); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - let result = job.complete(result, dep_node_index); + let result = job.complete(cache, result, dep_node_index); return (result, None); } @@ -504,7 +495,7 @@ where force_query_with_job(tcx, key, job.id, dep_node, query, compute) } }; - let result = job.complete(result, dep_node_index); + let result = job.complete(cache, result, dep_node_index); (result, Some(dep_node_index)) } From ef4becdce4198107507734246f6ffd053d0b8c67 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 11 May 2021 20:24:34 +0200 Subject: [PATCH 10/13] Split try_execute_query. --- .../rustc_query_system/src/query/plumbing.rs | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 204c10e8e990c..79fbde9585045 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -411,7 +411,7 @@ where C::Key: Clone + DepNodeParams, CTX: QueryContext, { - let job = match JobOwner::<'_, CTX::DepKind, C::Key>::try_start( + match JobOwner::<'_, CTX::DepKind, C::Key>::try_start( &tcx, state, span, @@ -419,10 +419,14 @@ where lookup, query.dep_kind, ) { - TryGetJob::NotYetStarted(job) => job, + TryGetJob::NotYetStarted(job) => { + let (result, dep_node_index) = execute_job(tcx, key, dep_node, query, job.id, compute); + let result = job.complete(cache, result, dep_node_index); + (result, Some(dep_node_index)) + } TryGetJob::Cycle(error) => { let result = mk_cycle(tcx, error, query.handle_cycle_error, &cache.cache); - return (result, None); + (result, None) } #[cfg(parallel_compiler)] TryGetJob::JobCompleted(query_blocked_prof_timer) => { @@ -440,27 +444,40 @@ where } query_blocked_prof_timer.finish_with_query_invocation_id(index.into()); - return (v, Some(index)); + (v, Some(index)) } - }; + } +} +fn execute_job( + tcx: CTX, + key: K, + dep_node: Option>, + query: &QueryVtable, + job_id: QueryJobId, + compute: fn(CTX::DepContext, K) -> V, +) -> (V, DepNodeIndex) +where + K: Clone + DepNodeParams, + V: Debug, + CTX: QueryContext, +{ let dep_graph = tcx.dep_context().dep_graph(); // Fast path for when incr. comp. is off. if !dep_graph.is_fully_enabled() { let prof_timer = tcx.dep_context().profiler().query_provider(); - let result = tcx.start_query(job.id, None, || compute(*tcx.dep_context(), key)); + let result = tcx.start_query(job_id, None, || compute(*tcx.dep_context(), key)); let dep_node_index = dep_graph.next_virtual_depnode_index(); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - let result = job.complete(cache, result, dep_node_index); - return (result, None); + return (result, dep_node_index); } - let (result, dep_node_index) = if query.anon { + if query.anon { let prof_timer = tcx.dep_context().profiler().query_provider(); let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { - tcx.start_query(job.id, diagnostics, || { + tcx.start_query(job_id, diagnostics, || { dep_graph.with_anon_task(*tcx.dep_context(), query.dep_kind, || { compute(*tcx.dep_context(), key) }) @@ -479,24 +496,22 @@ where } else if query.eval_always { // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - force_query_with_job(tcx, key, job.id, dep_node, query, compute) + force_query_with_job(tcx, key, job_id, dep_node, query, compute) } else { // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); // The diagnostics for this query will be // promoted to the current session during // `try_mark_green()`, so we can ignore them here. - let loaded = tcx.start_query(job.id, None, || { + let loaded = tcx.start_query(job_id, None, || { try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) }); if let Some((result, dep_node_index)) = loaded { (result, dep_node_index) } else { - force_query_with_job(tcx, key, job.id, dep_node, query, compute) + force_query_with_job(tcx, key, job_id, dep_node, query, compute) } - }; - let result = job.complete(cache, result, dep_node_index); - (result, Some(dep_node_index)) + } } fn try_load_from_disk_and_cache_in_memory( From f2c8707abbedee2587b5653a42e0860a101f0ddf Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 30 Dec 2020 22:08:57 +0100 Subject: [PATCH 11/13] Remove force_query_with_job. --- .../rustc_query_system/src/query/plumbing.rs | 141 +++++++----------- 1 file changed, 53 insertions(+), 88 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 79fbde9585045..e81a09d343df7 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -17,7 +17,7 @@ use rustc_data_structures::profiling::TimingGuard; use rustc_data_structures::sharded::{get_shard_index_by_hash, Sharded}; use rustc_data_structures::sync::{Lock, LockGuard}; use rustc_data_structures::thin_vec::ThinVec; -use rustc_errors::{Diagnostic, DiagnosticBuilder, FatalError}; +use rustc_errors::{DiagnosticBuilder, FatalError}; use rustc_span::{Span, DUMMY_SP}; use std::cell::Cell; use std::collections::hash_map::Entry; @@ -304,15 +304,6 @@ where } } -fn with_diagnostics(f: F) -> (R, ThinVec) -where - F: FnOnce(Option<&Lock>>) -> R, -{ - let diagnostics = Lock::new(ThinVec::new()); - let result = f(Some(&diagnostics)); - (result, diagnostics.into_inner()) -} - impl<'tcx, D, K> Drop for JobOwner<'tcx, D, K> where D: Copy + Clone + Eq + Hash, @@ -452,7 +443,7 @@ where fn execute_job( tcx: CTX, key: K, - dep_node: Option>, + mut dep_node_opt: Option>, query: &QueryVtable, job_id: QueryJobId, compute: fn(CTX::DepContext, K) -> V, @@ -473,45 +464,66 @@ where return (result, dep_node_index); } - if query.anon { - let prof_timer = tcx.dep_context().profiler().query_provider(); - - let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { - tcx.start_query(job_id, diagnostics, || { - dep_graph.with_anon_task(*tcx.dep_context(), query.dep_kind, || { - compute(*tcx.dep_context(), key) - }) - }) - }); + if !query.anon && !query.eval_always { + // `to_dep_node` is expensive for some `DepKind`s. + let dep_node = + dep_node_opt.get_or_insert_with(|| query.to_dep_node(*tcx.dep_context(), &key)); - prof_timer.finish_with_query_invocation_id(dep_node_index.into()); + // The diagnostics for this query will be promoted to the current session during + // `try_mark_green()`, so we can ignore them here. + if let Some(ret) = tcx.start_query(job_id, None, || { + try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) + }) { + return ret; + } + } - let side_effects = QuerySideEffects { diagnostics }; + let prof_timer = tcx.dep_context().profiler().query_provider(); + let diagnostics = Lock::new(ThinVec::new()); - if unlikely!(!side_effects.is_empty()) { - tcx.store_side_effects_for_anon_node(dep_node_index, side_effects); + let (result, dep_node_index) = tcx.start_query(job_id, Some(&diagnostics), || { + if query.anon { + return dep_graph.with_anon_task(*tcx.dep_context(), query.dep_kind, || { + compute(*tcx.dep_context(), key) + }); } - (result, dep_node_index) - } else if query.eval_always { // `to_dep_node` is expensive for some `DepKind`s. - let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - force_query_with_job(tcx, key, job_id, dep_node, query, compute) - } else { - // `to_dep_node` is expensive for some `DepKind`s. - let dep_node = dep_node.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - // The diagnostics for this query will be - // promoted to the current session during - // `try_mark_green()`, so we can ignore them here. - let loaded = tcx.start_query(job_id, None, || { - try_load_from_disk_and_cache_in_memory(tcx, &key, &dep_node, query, compute) - }); - if let Some((result, dep_node_index)) = loaded { - (result, dep_node_index) + let dep_node = dep_node_opt.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); + + if query.eval_always { + tcx.dep_context().dep_graph().with_eval_always_task( + dep_node, + *tcx.dep_context(), + key, + compute, + query.hash_result, + ) + } else { + tcx.dep_context().dep_graph().with_task( + dep_node, + *tcx.dep_context(), + key, + compute, + query.hash_result, + ) + } + }); + + prof_timer.finish_with_query_invocation_id(dep_node_index.into()); + + let diagnostics = diagnostics.into_inner(); + let side_effects = QuerySideEffects { diagnostics }; + + if unlikely!(!side_effects.is_empty()) { + if query.anon { + tcx.store_side_effects_for_anon_node(dep_node_index, side_effects); } else { - force_query_with_job(tcx, key, job_id, dep_node, query, compute) + tcx.store_side_effects(dep_node_index, side_effects); } } + + (result, dep_node_index) } fn try_load_from_disk_and_cache_in_memory( @@ -641,53 +653,6 @@ fn incremental_verify_ich( } } -fn force_query_with_job( - tcx: CTX, - key: K, - job_id: QueryJobId, - dep_node: DepNode, - query: &QueryVtable, - compute: fn(CTX::DepContext, K) -> V, -) -> (V, DepNodeIndex) -where - CTX: QueryContext, - K: Debug, -{ - let prof_timer = tcx.dep_context().profiler().query_provider(); - - let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { - tcx.start_query(job_id, diagnostics, || { - if query.eval_always { - tcx.dep_context().dep_graph().with_eval_always_task( - dep_node, - *tcx.dep_context(), - key, - compute, - query.hash_result, - ) - } else { - tcx.dep_context().dep_graph().with_task( - dep_node, - *tcx.dep_context(), - key, - compute, - query.hash_result, - ) - } - }) - }); - - prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - - let side_effects = QuerySideEffects { diagnostics }; - - if unlikely!(!side_effects.is_empty()) && dep_node.kind != DepKind::NULL { - tcx.store_side_effects(dep_node_index, side_effects); - } - - (result, dep_node_index) -} - /// Ensure that either this query has all green inputs or been executed. /// Executing `query::ensure(D)` is considered a read of the dep-node `D`. /// Returns true if the query should still run. From eeb3c8f4b786cd97028b9f1e134cb628c8879569 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 6 Jan 2021 19:06:34 +0100 Subject: [PATCH 12/13] Unify `with_task` functions. Remove with_eval_always_task. --- .../rustc_query_system/src/dep_graph/graph.rs | 155 ++++++++---------- .../rustc_query_system/src/query/plumbing.rs | 24 +-- 2 files changed, 76 insertions(+), 103 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 16fca7a3cd958..e589d16992f6c 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -215,24 +215,17 @@ impl DepGraph { cx: Ctxt, arg: A, task: fn(Ctxt, A) -> R, - hash_result: impl FnOnce(&mut Ctxt::StableHashingContext, &R) -> Option, + hash_result: fn(&mut Ctxt::StableHashingContext, &R) -> Option, ) -> (R, DepNodeIndex) { - self.with_task_impl( - key, - cx, - arg, - task, - |_key| { - Some(TaskDeps { - #[cfg(debug_assertions)] - node: Some(_key), - reads: SmallVec::new(), - read_set: Default::default(), - phantom_data: PhantomData, - }) - }, - hash_result, - ) + if self.is_fully_enabled() { + self.with_task_impl(key, cx, arg, task, hash_result) + } else { + // Incremental compilation is turned off. We just execute the task + // without tracking. We still provide a dep-node index that uniquely + // identifies the task so that we have a cheap way of referring to + // the query for self-profiling. + (task(cx, arg), self.next_virtual_depnode_index()) + } } fn with_task_impl, A: Debug, R>( @@ -241,71 +234,74 @@ impl DepGraph { cx: Ctxt, arg: A, task: fn(Ctxt, A) -> R, - create_task: fn(DepNode) -> Option>, - hash_result: impl FnOnce(&mut Ctxt::StableHashingContext, &R) -> Option, + hash_result: fn(&mut Ctxt::StableHashingContext, &R) -> Option, ) -> (R, DepNodeIndex) { - if let Some(ref data) = self.data { - // If the following assertion triggers, it can have two reasons: - // 1. Something is wrong with DepNode creation, either here or - // in `DepGraph::try_mark_green()`. - // 2. Two distinct query keys get mapped to the same `DepNode` - // (see for example #48923). - assert!( - !self.dep_node_exists(&key), - "forcing query with already existing `DepNode`\n\ + // This function is only called when the graph is enabled. + let data = self.data.as_ref().unwrap(); + + // If the following assertion triggers, it can have two reasons: + // 1. Something is wrong with DepNode creation, either here or + // in `DepGraph::try_mark_green()`. + // 2. Two distinct query keys get mapped to the same `DepNode` + // (see for example #48923). + assert!( + !self.dep_node_exists(&key), + "forcing query with already existing `DepNode`\n\ - query-key: {:?}\n\ - dep-node: {:?}", - arg, - key - ); + arg, + key + ); - let dcx = cx.dep_context(); - let task_deps = create_task(key).map(Lock::new); - let result = K::with_deps(task_deps.as_ref(), || task(cx, arg)); - let edges = task_deps.map_or_else(|| smallvec![], |lock| lock.into_inner().reads); - - let mut hcx = dcx.create_stable_hashing_context(); - let hashing_timer = dcx.profiler().incr_result_hashing(); - let current_fingerprint = hash_result(&mut hcx, &result); - - let print_status = cfg!(debug_assertions) && dcx.sess().opts.debugging_opts.dep_tasks; - - // Get timer for profiling `DepNode` interning - let node_intern_timer = self - .node_intern_event_id - .map(|eid| dcx.profiler().generic_activity_with_event_id(eid)); - // Intern the new `DepNode`. - let (dep_node_index, prev_and_color) = data.current.intern_node( - dcx.profiler(), - &data.previous, - key, - edges, - current_fingerprint, - print_status, - ); - drop(node_intern_timer); + let task_deps = if key.kind.is_eval_always() { + None + } else { + Some(Lock::new(TaskDeps { + #[cfg(debug_assertions)] + node: Some(key), + reads: SmallVec::new(), + read_set: Default::default(), + phantom_data: PhantomData, + })) + }; + let result = K::with_deps(task_deps.as_ref(), || task(cx, arg)); + let edges = task_deps.map_or_else(|| smallvec![], |lock| lock.into_inner().reads); + + let dcx = cx.dep_context(); + let mut hcx = dcx.create_stable_hashing_context(); + let hashing_timer = dcx.profiler().incr_result_hashing(); + let current_fingerprint = hash_result(&mut hcx, &result); + + let print_status = cfg!(debug_assertions) && dcx.sess().opts.debugging_opts.dep_tasks; + + // Get timer for profiling `DepNode` interning + let node_intern_timer = + self.node_intern_event_id.map(|eid| dcx.profiler().generic_activity_with_event_id(eid)); + // Intern the new `DepNode`. + let (dep_node_index, prev_and_color) = data.current.intern_node( + dcx.profiler(), + &data.previous, + key, + edges, + current_fingerprint, + print_status, + ); + drop(node_intern_timer); - hashing_timer.finish_with_query_invocation_id(dep_node_index.into()); + hashing_timer.finish_with_query_invocation_id(dep_node_index.into()); - if let Some((prev_index, color)) = prev_and_color { - debug_assert!( - data.colors.get(prev_index).is_none(), - "DepGraph::with_task() - Duplicate DepNodeColor \ + if let Some((prev_index, color)) = prev_and_color { + debug_assert!( + data.colors.get(prev_index).is_none(), + "DepGraph::with_task() - Duplicate DepNodeColor \ insertion for {:?}", - key - ); - - data.colors.insert(prev_index, color); - } + key + ); - (result, dep_node_index) - } else { - // Incremental compilation is turned off. We just execute the task - // without tracking. We still provide a dep-node index that uniquely - // identifies the task so that we have a cheap way of referring to - // the query for self-profiling. - (task(cx, arg), self.next_virtual_depnode_index()) + data.colors.insert(prev_index, color); } + + (result, dep_node_index) } /// Executes something within an "anonymous" task, that is, a task the @@ -372,19 +368,6 @@ impl DepGraph { } } - /// Executes something within an "eval-always" task which is a task - /// that runs whenever anything changes. - pub fn with_eval_always_task, A: Debug, R>( - &self, - key: DepNode, - cx: Ctxt, - arg: A, - task: fn(Ctxt, A) -> R, - hash_result: impl FnOnce(&mut Ctxt::StableHashingContext, &R) -> Option, - ) -> (R, DepNodeIndex) { - self.with_task_impl(key, cx, arg, task, |_| None, hash_result) - } - #[inline] pub fn read_index(&self, dep_node_index: DepNodeIndex) { if let Some(ref data) = self.data { diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index e81a09d343df7..32cf724c6d70b 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -491,23 +491,13 @@ where // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node_opt.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - if query.eval_always { - tcx.dep_context().dep_graph().with_eval_always_task( - dep_node, - *tcx.dep_context(), - key, - compute, - query.hash_result, - ) - } else { - tcx.dep_context().dep_graph().with_task( - dep_node, - *tcx.dep_context(), - key, - compute, - query.hash_result, - ) - } + tcx.dep_context().dep_graph().with_task( + dep_node, + *tcx.dep_context(), + key, + compute, + query.hash_result, + ) }); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); From 31330bfce12f59b9c9a4d7b20235fdc38dcf7583 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 12 May 2021 11:21:12 +0200 Subject: [PATCH 13/13] Use variable. --- .../rustc_query_system/src/query/plumbing.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 32cf724c6d70b..95d9406248f72 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -491,13 +491,7 @@ where // `to_dep_node` is expensive for some `DepKind`s. let dep_node = dep_node_opt.unwrap_or_else(|| query.to_dep_node(*tcx.dep_context(), &key)); - tcx.dep_context().dep_graph().with_task( - dep_node, - *tcx.dep_context(), - key, - compute, - query.hash_result, - ) + dep_graph.with_task(dep_node, *tcx.dep_context(), key, compute, query.hash_result) }); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); @@ -531,10 +525,10 @@ where // Note this function can be called concurrently from the same query // We must ensure that this is handled correctly. - let (prev_dep_node_index, dep_node_index) = - tcx.dep_context().dep_graph().try_mark_green(tcx, &dep_node)?; + let dep_graph = tcx.dep_context().dep_graph(); + let (prev_dep_node_index, dep_node_index) = dep_graph.try_mark_green(tcx, &dep_node)?; - debug_assert!(tcx.dep_context().dep_graph().is_green(dep_node)); + debug_assert!(dep_graph.is_green(dep_node)); // First we try to load the result from the on-disk cache. // Some things are never cached on disk. @@ -567,8 +561,7 @@ where let prof_timer = tcx.dep_context().profiler().query_provider(); // The dep-graph for this computation is already in-place. - let result = - tcx.dep_context().dep_graph().with_ignore(|| compute(*tcx.dep_context(), key.clone())); + let result = dep_graph.with_ignore(|| compute(*tcx.dep_context(), key.clone())); prof_timer.finish_with_query_invocation_id(dep_node_index.into());