From 8f294066b32c13fb9865aecfc1ce9cd30e956555 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sun, 19 Mar 2023 23:48:08 +0100 Subject: [PATCH 1/4] Optimize `incremental_verify_ich` --- compiler/rustc_middle/src/dep_graph/mod.rs | 6 ++ .../rustc_query_system/src/dep_graph/graph.rs | 25 ++++-- .../rustc_query_system/src/dep_graph/mod.rs | 4 + .../src/dep_graph/serialized.rs | 5 -- .../rustc_query_system/src/query/plumbing.rs | 83 +++++++++++-------- 5 files changed, 74 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_middle/src/dep_graph/mod.rs b/compiler/rustc_middle/src/dep_graph/mod.rs index 84510fe218c..6ecfda4a1bf 100644 --- a/compiler/rustc_middle/src/dep_graph/mod.rs +++ b/compiler/rustc_middle/src/dep_graph/mod.rs @@ -71,6 +71,7 @@ impl rustc_query_system::dep_graph::DepKind for DepKind { } impl<'tcx> DepContext for TyCtxt<'tcx> { + type Implicit<'a> = TyCtxt<'a>; type DepKind = DepKind; #[inline] @@ -78,6 +79,11 @@ impl<'tcx> DepContext for TyCtxt<'tcx> { TyCtxt::with_stable_hashing_context(self, f) } + #[inline] + fn with_context(f: impl FnOnce(TyCtxt<'_>) -> R) -> R { + ty::tls::with(|tcx| f(tcx)) + } + #[inline] fn dep_graph(&self) -> &DepGraph { &self.dep_graph diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 3dbcc4d2e8a..09b85010666 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -535,13 +535,14 @@ impl DepGraph { // value to an existing node. // // For sanity, we still check that the loaded stable hash and the new one match. - if let Some(dep_node_index) = data.dep_node_index_of_opt(&node) { - let _current_fingerprint = - crate::query::incremental_verify_ich(cx, data, result, &node, hash_result); + if let Some(prev_index) = data.previous.node_to_index_opt(&node) + && let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] } + { + crate::query::incremental_verify_ich(cx, data, result, prev_index, hash_result); #[cfg(debug_assertions)] if hash_result.is_some() { - data.current.record_edge(dep_node_index, node, _current_fingerprint); + data.current.record_edge(dep_node_index, node, data.prev_fingerprint_of(prev_index)); } return dep_node_index; @@ -626,13 +627,19 @@ impl DepGraphData { /// Returns true if the given node has been marked as green during the /// current compilation session. Used in various assertions - pub fn is_green(&self, dep_node: &DepNode) -> bool { - self.node_color(dep_node).map_or(false, |c| c.is_green()) + #[inline] + pub fn is_index_green(&self, prev_index: SerializedDepNodeIndex) -> bool { + self.colors.get(prev_index).map_or(false, |c| c.is_green()) } #[inline] - pub fn prev_fingerprint_of(&self, dep_node: &DepNode) -> Option { - self.previous.fingerprint_of(dep_node) + pub fn prev_fingerprint_of(&self, prev_index: SerializedDepNodeIndex) -> Fingerprint { + self.previous.fingerprint_by_index(prev_index) + } + + #[inline] + pub fn prev_node_of(&self, prev_index: SerializedDepNodeIndex) -> DepNode { + self.previous.index_to_node(prev_index) } pub fn mark_debug_loaded_from_disk(&self, dep_node: DepNode) { @@ -643,7 +650,7 @@ impl DepGraphData { impl DepGraph { #[inline] pub fn dep_node_exists(&self, dep_node: &DepNode) -> bool { - self.data.as_ref().and_then(|data| data.dep_node_index_of_opt(dep_node)).is_some() + self.data.as_ref().map_or(false, |data| data.dep_node_exists(dep_node)) } /// Checks whether a previous work product exists for `v` and, if diff --git a/compiler/rustc_query_system/src/dep_graph/mod.rs b/compiler/rustc_query_system/src/dep_graph/mod.rs index 40e7131987f..246ec994e57 100644 --- a/compiler/rustc_query_system/src/dep_graph/mod.rs +++ b/compiler/rustc_query_system/src/dep_graph/mod.rs @@ -23,11 +23,15 @@ use std::{fmt, panic}; use self::graph::{print_markframe_trace, MarkFrame}; pub trait DepContext: Copy { + type Implicit<'a>: DepContext; type DepKind: self::DepKind; /// Create a hashing context for hashing new results. fn with_stable_hashing_context(self, f: impl FnOnce(StableHashingContext<'_>) -> R) -> R; + /// Access the implicit context. + fn with_context(f: impl FnOnce(Self::Implicit<'_>) -> R) -> R; + /// Access the DepGraph. fn dep_graph(&self) -> &DepGraph; diff --git a/compiler/rustc_query_system/src/dep_graph/serialized.rs b/compiler/rustc_query_system/src/dep_graph/serialized.rs index 29513df460f..3d19a84915a 100644 --- a/compiler/rustc_query_system/src/dep_graph/serialized.rs +++ b/compiler/rustc_query_system/src/dep_graph/serialized.rs @@ -79,11 +79,6 @@ impl SerializedDepGraph { self.index.get(dep_node).cloned() } - #[inline] - pub fn fingerprint_of(&self, dep_node: &DepNode) -> Option { - self.index.get(dep_node).map(|&node_index| self.fingerprints[node_index]) - } - #[inline] pub fn fingerprint_by_index(&self, dep_node_index: SerializedDepNodeIndex) -> Fingerprint { self.fingerprints[dep_node_index] diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index ba2f859ff0f..e016d948db5 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -7,6 +7,7 @@ use crate::dep_graph::{DepGraphData, HasDepContext}; use crate::ich::StableHashingContext; use crate::query::caches::QueryCache; use crate::query::job::{report_cycle, QueryInfo, QueryJob, QueryJobId, QueryJobInfo}; +use crate::query::SerializedDepNodeIndex; use crate::query::{QueryContext, QueryMap, QuerySideEffects, QueryStackFrame}; use crate::values::Value; use crate::HandleCycleError; @@ -19,7 +20,6 @@ use rustc_data_structures::sharded::Sharded; use rustc_data_structures::stack::ensure_sufficient_stack; use rustc_data_structures::sync::{Lock, LockGuard}; use rustc_errors::{DiagnosticBuilder, ErrorGuaranteed, FatalError}; -use rustc_session::Session; use rustc_span::{Span, DUMMY_SP}; use std::cell::Cell; use std::collections::hash_map::Entry; @@ -537,7 +537,7 @@ where let (prev_dep_node_index, dep_node_index) = dep_graph_data.try_mark_green(qcx, &dep_node)?; - debug_assert!(dep_graph_data.is_green(dep_node)); + debug_assert!(dep_graph_data.is_index_green(prev_dep_node_index)); // First we try to load the result from the on-disk cache. // Some things are never cached on disk. @@ -561,8 +561,7 @@ where dep_graph_data.mark_debug_loaded_from_disk(*dep_node) } - let prev_fingerprint = - dep_graph_data.prev_fingerprint_of(dep_node).unwrap_or(Fingerprint::ZERO); + let prev_fingerprint = dep_graph_data.prev_fingerprint_of(prev_dep_node_index); // If `-Zincremental-verify-ich` is specified, re-hash results from // the cache and make sure that they have the expected fingerprint. // @@ -578,7 +577,7 @@ where *qcx.dep_context(), dep_graph_data, &result, - dep_node, + prev_dep_node_index, query.hash_result(), ); } @@ -623,7 +622,7 @@ where *qcx.dep_context(), dep_graph_data, &result, - dep_node, + prev_dep_node_index, query.hash_result(), ); @@ -636,32 +635,38 @@ pub(crate) fn incremental_verify_ich( tcx: Tcx, dep_graph_data: &DepGraphData, result: &V, - dep_node: &DepNode, + prev_index: SerializedDepNodeIndex, hash_result: Option, &V) -> Fingerprint>, -) -> Fingerprint -where +) where Tcx: DepContext, { - assert!( - dep_graph_data.is_green(dep_node), - "fingerprint for green query instance not loaded from cache: {dep_node:?}", - ); + if !dep_graph_data.is_index_green(prev_index) { + incremental_verify_ich_not_green::(prev_index) + } let new_hash = hash_result.map_or(Fingerprint::ZERO, |f| { tcx.with_stable_hashing_context(|mut hcx| f(&mut hcx, result)) }); - let old_hash = dep_graph_data.prev_fingerprint_of(dep_node); + let old_hash = dep_graph_data.prev_fingerprint_of(prev_index); - if Some(new_hash) != old_hash { - incremental_verify_ich_failed( - tcx.sess(), - DebugArg::from(&dep_node), - DebugArg::from(&result), - ); + if new_hash != old_hash { + incremental_verify_ich_failed::(prev_index, DebugArg::from(&result)); } +} - new_hash +#[cold] +#[inline(never)] +fn incremental_verify_ich_not_green(prev_index: SerializedDepNodeIndex) +where + Tcx: DepContext, +{ + Tcx::with_context(|tcx| { + panic!( + "fingerprint for green query instance not loaded from cache: {:?}", + tcx.dep_graph().data().unwrap().prev_node_of(prev_index) + ) + }) } // This DebugArg business is largely a mirror of std::fmt::ArgumentV1, which is @@ -706,7 +711,11 @@ impl std::fmt::Debug for DebugArg<'_> { // different implementations for LLVM to chew on (and filling up the final // binary, too). #[cold] -fn incremental_verify_ich_failed(sess: &Session, dep_node: DebugArg<'_>, result: DebugArg<'_>) { +#[inline(never)] +fn incremental_verify_ich_failed(prev_index: SerializedDepNodeIndex, result: DebugArg<'_>) +where + Tcx: DepContext, +{ // When we emit an error message and panic, we try to debug-print the `DepNode` // and query result. Unfortunately, this can cause us to run additional queries, // which may result in another fingerprint mismatch while we're in the middle @@ -719,21 +728,25 @@ fn incremental_verify_ich_failed(sess: &Session, dep_node: DebugArg<'_>, result: let old_in_panic = INSIDE_VERIFY_PANIC.with(|in_panic| in_panic.replace(true)); - if old_in_panic { - sess.emit_err(crate::error::Reentrant); - } else { - let run_cmd = if let Some(crate_name) = &sess.opts.crate_name { - format!("`cargo clean -p {crate_name}` or `cargo clean`") + Tcx::with_context(|tcx| { + if old_in_panic { + tcx.sess().emit_err(crate::error::Reentrant); } else { - "`cargo clean`".to_string() - }; + let run_cmd = if let Some(crate_name) = &tcx.sess().opts.crate_name { + format!("`cargo clean -p {crate_name}` or `cargo clean`") + } else { + "`cargo clean`".to_string() + }; - sess.emit_err(crate::error::IncrementCompilation { - run_cmd, - dep_node: format!("{dep_node:?}"), - }); - panic!("Found unstable fingerprints for {dep_node:?}: {result:?}"); - } + let dep_node = tcx.dep_graph().data().unwrap().prev_node_of(prev_index); + + let dep_node = tcx.sess().emit_err(crate::error::IncrementCompilation { + run_cmd, + dep_node: format!("{dep_node:?}"), + }); + panic!("Found unstable fingerprints for {dep_node:?}: {result:?}"); + } + }); INSIDE_VERIFY_PANIC.with(|in_panic| in_panic.set(old_in_panic)); } From dfae9c993dc6ab685bc63c684c1ce66dfa596809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Mon, 20 Mar 2023 03:49:19 +0100 Subject: [PATCH 2/4] Remove `DebugArg` --- .../rustc_query_system/src/query/plumbing.rs | 48 ++----------------- 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index e016d948db5..8465696d301 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -651,7 +651,7 @@ pub(crate) fn incremental_verify_ich( let old_hash = dep_graph_data.prev_fingerprint_of(prev_index); if new_hash != old_hash { - incremental_verify_ich_failed::(prev_index, DebugArg::from(&result)); + incremental_verify_ich_failed::(prev_index, result); } } @@ -669,50 +669,12 @@ where }) } -// This DebugArg business is largely a mirror of std::fmt::ArgumentV1, which is -// currently not exposed publicly. -// -// The PR which added this attempted to use `&dyn Debug` instead, but that -// showed statistically significant worse compiler performance. It's not -// actually clear what the cause there was -- the code should be cold. If this -// can be replaced with `&dyn Debug` with on perf impact, then it probably -// should be. -extern "C" { - type Opaque; -} - -struct DebugArg<'a> { - value: &'a Opaque, - fmt: fn(&Opaque, &mut std::fmt::Formatter<'_>) -> std::fmt::Result, -} - -impl<'a, T> From<&'a T> for DebugArg<'a> -where - T: std::fmt::Debug, -{ - fn from(value: &'a T) -> DebugArg<'a> { - DebugArg { - value: unsafe { std::mem::transmute(value) }, - fmt: unsafe { - std::mem::transmute(::fmt as fn(_, _) -> std::fmt::Result) - }, - } - } -} - -impl std::fmt::Debug for DebugArg<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - (self.fmt)(self.value, f) - } -} - -// Note that this is marked #[cold] and intentionally takes the equivalent of -// `dyn Debug` for its arguments, as we want to avoid generating a bunch of -// different implementations for LLVM to chew on (and filling up the final -// binary, too). +// Note that this is marked #[cold] and intentionally takes `dyn Debug` for `result`, +// as we want to avoid generating a bunch of different implementations for LLVM to +// chew on (and filling up the final binary, too). #[cold] #[inline(never)] -fn incremental_verify_ich_failed(prev_index: SerializedDepNodeIndex, result: DebugArg<'_>) +fn incremental_verify_ich_failed(prev_index: SerializedDepNodeIndex, result: &dyn Debug) where Tcx: DepContext, { From afe4c16b294312b7aeb920e9a988a3eaca580d3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 25 Mar 2023 03:12:41 +0100 Subject: [PATCH 3/4] Split the `if` to release the lock earlier --- .../rustc_query_system/src/dep_graph/graph.rs | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 09b85010666..80618fd1abe 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -535,17 +535,22 @@ impl DepGraph { // value to an existing node. // // For sanity, we still check that the loaded stable hash and the new one match. - if let Some(prev_index) = data.previous.node_to_index_opt(&node) - && let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] } - { - crate::query::incremental_verify_ich(cx, data, result, prev_index, hash_result); + if let Some(prev_index) = data.previous.node_to_index_opt(&node) { + let dep_node_index = data.current.prev_index_to_index.lock()[prev_index]; + if let Some(dep_node_index) = dep_node_index { + crate::query::incremental_verify_ich(cx, data, result, prev_index, hash_result); - #[cfg(debug_assertions)] - if hash_result.is_some() { - data.current.record_edge(dep_node_index, node, data.prev_fingerprint_of(prev_index)); + #[cfg(debug_assertions)] + if hash_result.is_some() { + data.current.record_edge( + dep_node_index, + node, + data.prev_fingerprint_of(prev_index), + ); + } + + return dep_node_index; } - - return dep_node_index; } let mut edges = SmallVec::new(); From 820e3a8d6aec53d8f5f451e43cd1ef87bd29dc0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 25 Mar 2023 03:13:05 +0100 Subject: [PATCH 4/4] Pass `tcx` directly --- compiler/rustc_middle/src/dep_graph/mod.rs | 6 --- .../rustc_query_system/src/dep_graph/mod.rs | 4 -- .../rustc_query_system/src/query/plumbing.rs | 53 +++++++++---------- 3 files changed, 26 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_middle/src/dep_graph/mod.rs b/compiler/rustc_middle/src/dep_graph/mod.rs index 6ecfda4a1bf..84510fe218c 100644 --- a/compiler/rustc_middle/src/dep_graph/mod.rs +++ b/compiler/rustc_middle/src/dep_graph/mod.rs @@ -71,7 +71,6 @@ impl rustc_query_system::dep_graph::DepKind for DepKind { } impl<'tcx> DepContext for TyCtxt<'tcx> { - type Implicit<'a> = TyCtxt<'a>; type DepKind = DepKind; #[inline] @@ -79,11 +78,6 @@ impl<'tcx> DepContext for TyCtxt<'tcx> { TyCtxt::with_stable_hashing_context(self, f) } - #[inline] - fn with_context(f: impl FnOnce(TyCtxt<'_>) -> R) -> R { - ty::tls::with(|tcx| f(tcx)) - } - #[inline] fn dep_graph(&self) -> &DepGraph { &self.dep_graph diff --git a/compiler/rustc_query_system/src/dep_graph/mod.rs b/compiler/rustc_query_system/src/dep_graph/mod.rs index 246ec994e57..40e7131987f 100644 --- a/compiler/rustc_query_system/src/dep_graph/mod.rs +++ b/compiler/rustc_query_system/src/dep_graph/mod.rs @@ -23,15 +23,11 @@ use std::{fmt, panic}; use self::graph::{print_markframe_trace, MarkFrame}; pub trait DepContext: Copy { - type Implicit<'a>: DepContext; type DepKind: self::DepKind; /// Create a hashing context for hashing new results. fn with_stable_hashing_context(self, f: impl FnOnce(StableHashingContext<'_>) -> R) -> R; - /// Access the implicit context. - fn with_context(f: impl FnOnce(Self::Implicit<'_>) -> R) -> R; - /// Access the DepGraph. fn dep_graph(&self) -> &DepGraph; diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 8465696d301..186417e862a 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -641,7 +641,7 @@ pub(crate) fn incremental_verify_ich( Tcx: DepContext, { if !dep_graph_data.is_index_green(prev_index) { - incremental_verify_ich_not_green::(prev_index) + incremental_verify_ich_not_green(tcx, prev_index) } let new_hash = hash_result.map_or(Fingerprint::ZERO, |f| { @@ -651,22 +651,20 @@ pub(crate) fn incremental_verify_ich( let old_hash = dep_graph_data.prev_fingerprint_of(prev_index); if new_hash != old_hash { - incremental_verify_ich_failed::(prev_index, result); + incremental_verify_ich_failed(tcx, prev_index, result); } } #[cold] #[inline(never)] -fn incremental_verify_ich_not_green(prev_index: SerializedDepNodeIndex) +fn incremental_verify_ich_not_green(tcx: Tcx, prev_index: SerializedDepNodeIndex) where Tcx: DepContext, { - Tcx::with_context(|tcx| { - panic!( - "fingerprint for green query instance not loaded from cache: {:?}", - tcx.dep_graph().data().unwrap().prev_node_of(prev_index) - ) - }) + panic!( + "fingerprint for green query instance not loaded from cache: {:?}", + tcx.dep_graph().data().unwrap().prev_node_of(prev_index) + ) } // Note that this is marked #[cold] and intentionally takes `dyn Debug` for `result`, @@ -674,8 +672,11 @@ where // chew on (and filling up the final binary, too). #[cold] #[inline(never)] -fn incremental_verify_ich_failed(prev_index: SerializedDepNodeIndex, result: &dyn Debug) -where +fn incremental_verify_ich_failed( + tcx: Tcx, + prev_index: SerializedDepNodeIndex, + result: &dyn Debug, +) where Tcx: DepContext, { // When we emit an error message and panic, we try to debug-print the `DepNode` @@ -690,25 +691,23 @@ where let old_in_panic = INSIDE_VERIFY_PANIC.with(|in_panic| in_panic.replace(true)); - Tcx::with_context(|tcx| { - if old_in_panic { - tcx.sess().emit_err(crate::error::Reentrant); + if old_in_panic { + tcx.sess().emit_err(crate::error::Reentrant); + } else { + let run_cmd = if let Some(crate_name) = &tcx.sess().opts.crate_name { + format!("`cargo clean -p {crate_name}` or `cargo clean`") } else { - let run_cmd = if let Some(crate_name) = &tcx.sess().opts.crate_name { - format!("`cargo clean -p {crate_name}` or `cargo clean`") - } else { - "`cargo clean`".to_string() - }; + "`cargo clean`".to_string() + }; - let dep_node = tcx.dep_graph().data().unwrap().prev_node_of(prev_index); + let dep_node = tcx.dep_graph().data().unwrap().prev_node_of(prev_index); - let dep_node = tcx.sess().emit_err(crate::error::IncrementCompilation { - run_cmd, - dep_node: format!("{dep_node:?}"), - }); - panic!("Found unstable fingerprints for {dep_node:?}: {result:?}"); - } - }); + let dep_node = tcx.sess().emit_err(crate::error::IncrementCompilation { + run_cmd, + dep_node: format!("{dep_node:?}"), + }); + panic!("Found unstable fingerprints for {dep_node:?}: {result:?}"); + } INSIDE_VERIFY_PANIC.with(|in_panic| in_panic.set(old_in_panic)); }