From 8b4fa75acae5c449cbe0ccadbb941a1a2eaf5560 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 12 Mar 2023 10:56:20 +0000 Subject: [PATCH 1/5] Only use the new node hashmap for anonymous nodes. --- .../rustc_codegen_cranelift/src/driver/aot.rs | 11 +- compiler/rustc_codegen_ssa/src/base.rs | 11 +- .../rustc_incremental/src/persist/save.rs | 2 +- .../rustc_query_system/src/dep_graph/graph.rs | 174 ++++++++++-------- compiler/rustc_session/src/options.rs | 4 +- 5 files changed, 110 insertions(+), 92 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/driver/aot.rs b/compiler/rustc_codegen_cranelift/src/driver/aot.rs index d143bcc96ef93..af9297ee6800f 100644 --- a/compiler/rustc_codegen_cranelift/src/driver/aot.rs +++ b/compiler/rustc_codegen_cranelift/src/driver/aot.rs @@ -522,11 +522,12 @@ fn determine_cgu_reuse<'tcx>(tcx: TyCtxt<'tcx>, cgu: &CodegenUnit<'tcx>) -> CguR // know that later). If we are not doing LTO, there is only one optimized // version of each module, so we re-use that. let dep_node = cgu.codegen_dep_node(tcx); - assert!( - !tcx.dep_graph.dep_node_exists(&dep_node), - "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", - cgu.name() - ); + tcx.dep_graph.assert_nonexistent_node(&dep_node, || { + format!( + "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", + cgu.name() + ) + }); if tcx.try_mark_green(&dep_node) { CguReuse::PostLto } else { CguReuse::No } } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 9133601ecd126..e3d46b8c052e4 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -1031,11 +1031,12 @@ fn determine_cgu_reuse<'tcx>(tcx: TyCtxt<'tcx>, cgu: &CodegenUnit<'tcx>) -> CguR // know that later). If we are not doing LTO, there is only one optimized // version of each module, so we re-use that. let dep_node = cgu.codegen_dep_node(tcx); - assert!( - !tcx.dep_graph.dep_node_exists(&dep_node), - "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", - cgu.name() - ); + tcx.dep_graph.assert_nonexistent_node(&dep_node, || { + format!( + "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", + cgu.name() + ) + }); if tcx.try_mark_green(&dep_node) { // We can re-use either the pre- or the post-thinlto state. If no LTO is diff --git a/compiler/rustc_incremental/src/persist/save.rs b/compiler/rustc_incremental/src/persist/save.rs index bfaa52f9c8134..8ce41aafe650a 100644 --- a/compiler/rustc_incremental/src/persist/save.rs +++ b/compiler/rustc_incremental/src/persist/save.rs @@ -170,7 +170,7 @@ pub fn build_dep_graph( sess.opts.dep_tracking_hash(false).encode(&mut encoder); Some(DepGraph::new( - &sess.prof, + sess, prev_graph, prev_work_products, encoder, diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index c9e80a6d9bc13..9380e62f73539 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -9,6 +9,7 @@ use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock, Lrc, Ordering}; use rustc_data_structures::unord::UnordMap; use rustc_index::IndexVec; use rustc_serialize::opaque::{FileEncodeResult, FileEncoder}; +use rustc_session::Session; use smallvec::{smallvec, SmallVec}; use std::assert_matches::assert_matches; use std::collections::hash_map::Entry; @@ -115,7 +116,7 @@ where impl DepGraph { pub fn new( - profiler: &SelfProfilerRef, + session: &Session, prev_graph: SerializedDepGraph, prev_work_products: FxIndexMap, encoder: FileEncoder, @@ -125,7 +126,7 @@ impl DepGraph { let prev_graph_node_count = prev_graph.node_count(); let current = CurrentDepGraph::new( - profiler, + session, prev_graph_node_count, encoder, record_graph, @@ -136,7 +137,7 @@ impl DepGraph { // Instantiate a dependy-less node only once for anonymous queries. let _green_node_index = current.intern_new_node( - profiler, + &session.prof, DepNode { kind: DepKind::NULL, hash: current.anon_id_seed.into() }, smallvec![], Fingerprint::ZERO, @@ -145,7 +146,7 @@ impl DepGraph { // Instantiate a dependy-less red node only once for anonymous queries. let (red_node_index, red_node_prev_index_and_color) = current.intern_node( - profiler, + &session.prof, &prev_graph, DepNode { kind: DepKind::RED, hash: Fingerprint::ZERO.into() }, smallvec![], @@ -343,17 +344,13 @@ impl DepGraphData { task: fn(Ctxt, A) -> R, hash_result: Option, &R) -> Fingerprint>, ) -> (R, DepNodeIndex) { - // 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: {arg:?}\n\ - - dep-node: {key:?}" - ); + self.assert_nonexistent_node(&key, || { + format!( + "forcing query with already existing `DepNode`\n\ + - query-key: {arg:?}\n\ + - dep-node: {key:?}" + ) + }); let with_deps = |task_deps| K::with_deps(task_deps, || task(cx, arg)); let (result, edges) = if cx.dep_context().is_eval_always(key.kind) { @@ -449,12 +446,32 @@ impl DepGraphData { hash: self.current.anon_id_seed.combine(hasher.finish()).into(), }; - self.current.intern_new_node( - cx.profiler(), - target_dep_node, - task_deps, - Fingerprint::ZERO, - ) + // The DepNodes generated by the process above are not unique. 2 queries could + // have exactly the same dependencies. However, deserialization does not handle + // duplicated nodes, so we do the deduplication here directly. + // + // As anonymous nodes are a small quantity compared to the full dep-graph, the + // memory impact of this `anon_node_to_index` map remains tolerable, and helps + // us avoid useless growth of the graph with almost-equivalent nodes. + match self + .current + .anon_node_to_index + .get_shard_by_value(&target_dep_node) + .lock() + .entry(target_dep_node) + { + Entry::Occupied(entry) => *entry.get(), + Entry::Vacant(entry) => { + let dep_node_index = self.current.intern_new_node( + cx.profiler(), + target_dep_node, + task_deps, + Fingerprint::ZERO, + ); + entry.insert(dep_node_index); + dep_node_index + } + } } }; @@ -625,25 +642,18 @@ impl DepGraph { } impl DepGraphData { - #[inline] - pub fn dep_node_index_of_opt(&self, dep_node: &DepNode) -> Option { - if let Some(prev_index) = self.previous.node_to_index_opt(dep_node) { - self.current.prev_index_to_index.lock()[prev_index] - } else { - self.current - .new_node_to_index - .get_shard_by_value(dep_node) - .lock() - .get(dep_node) - .copied() + fn assert_nonexistent_node( + &self, + _dep_node: &DepNode, + _msg: impl FnOnce() -> S, + ) { + #[cfg(debug_assertions)] + if let Some(seen_dep_nodes) = &self.current.seen_dep_nodes { + let seen = seen_dep_nodes.lock().contains(_dep_node); + assert!(!seen, "{}", _msg()); } } - #[inline] - pub fn dep_node_exists(&self, dep_node: &DepNode) -> bool { - self.dep_node_index_of_opt(dep_node).is_some() - } - fn node_color(&self, dep_node: &DepNode) -> Option { if let Some(prev_index) = self.previous.node_to_index_opt(dep_node) { self.colors.get(prev_index) @@ -676,11 +686,6 @@ impl DepGraphData { } impl DepGraph { - #[inline] - pub fn dep_node_exists(&self, dep_node: &DepNode) -> bool { - self.data.as_ref().is_some_and(|data| data.dep_node_exists(dep_node)) - } - /// Checks whether a previous work product exists for `v` and, if /// so, return the path that leads to it. Used to skip doing work. pub fn previous_work_product(&self, v: &WorkProductId) -> Option { @@ -762,7 +767,7 @@ impl DepGraphData { } } - #[instrument(skip(self, qcx, parent_dep_node_index, frame), level = "debug")] + #[instrument(skip(self, qcx, frame), level = "debug")] fn try_mark_parent_green>( &self, qcx: Qcx, @@ -861,10 +866,11 @@ impl DepGraphData { let frame = MarkFrame { index: prev_dep_node_index, parent: frame }; #[cfg(not(parallel_compiler))] - { - debug_assert!(!self.dep_node_exists(dep_node)); - debug_assert!(self.colors.get(prev_dep_node_index).is_none()); - } + self.assert_nonexistent_node(dep_node, || { + format!("trying to mark existing {dep_node:?} as green") + }); + #[cfg(not(parallel_compiler))] + debug_assert!(self.colors.get(prev_dep_node_index).is_none()); // We never try to mark eval_always nodes as green debug_assert!(!qcx.dep_context().is_eval_always(dep_node.kind)); @@ -959,6 +965,18 @@ impl DepGraph { self.node_color(dep_node).is_some_and(|c| c.is_green()) } + pub fn assert_nonexistent_node( + &self, + dep_node: &DepNode, + msg: impl FnOnce() -> S, + ) { + if cfg!(debug_assertions) + && let Some(data) = &self.data + { + data.assert_nonexistent_node(dep_node, msg) + } + } + /// This method loads all on-disk cacheable query results into memory, so /// they can be written out to the new cache file again. Most query results /// will already be in memory but in the case where we marked something as @@ -1066,24 +1084,24 @@ rustc_index::newtype_index! { /// largest in the compiler. /// /// For this reason, we avoid storing `DepNode`s more than once as map -/// keys. The `new_node_to_index` map only contains nodes not in the previous +/// keys. The `anon_node_to_index` map only contains nodes of anonymous queries not in the previous /// graph, and we map nodes in the previous graph to indices via a two-step /// mapping. `SerializedDepGraph` maps from `DepNode` to `SerializedDepNodeIndex`, /// and the `prev_index_to_index` vector (which is more compact and faster than /// using a map) maps from `SerializedDepNodeIndex` to `DepNodeIndex`. /// -/// This struct uses three locks internally. The `data`, `new_node_to_index`, +/// This struct uses three locks internally. The `data`, `anon_node_to_index`, /// and `prev_index_to_index` fields are locked separately. Operations that take /// a `DepNodeIndex` typically just access the `data` field. /// /// We only need to manipulate at most two locks simultaneously: -/// `new_node_to_index` and `data`, or `prev_index_to_index` and `data`. When -/// manipulating both, we acquire `new_node_to_index` or `prev_index_to_index` +/// `anon_node_to_index` and `data`, or `prev_index_to_index` and `data`. When +/// manipulating both, we acquire `anon_node_to_index` or `prev_index_to_index` /// first, and `data` second. pub(super) struct CurrentDepGraph { encoder: Steal>, - new_node_to_index: Sharded, DepNodeIndex>>, prev_index_to_index: Lock>>, + anon_node_to_index: Sharded, DepNodeIndex>>, /// This is used to verify that fingerprints do not change between the creation of a node /// and its recomputation. @@ -1095,6 +1113,11 @@ pub(super) struct CurrentDepGraph { #[cfg(debug_assertions)] forbidden_edge: Option>, + /// Used to verify the absence of hash collisions among DepNodes. + /// This field is only `Some` if the `-Z incremental_verify_ich` option is present. + #[cfg(debug_assertions)] + seen_dep_nodes: Option>>>, + /// Anonymous `DepNode`s are nodes whose IDs we compute from the list of /// their edges. This has the beneficial side-effect that multiple anonymous /// nodes can be coalesced into one without changing the semantics of the @@ -1122,7 +1145,7 @@ pub(super) struct CurrentDepGraph { impl CurrentDepGraph { fn new( - profiler: &SelfProfilerRef, + session: &Session, prev_graph_node_count: usize, encoder: FileEncoder, record_graph: bool, @@ -1152,7 +1175,8 @@ impl CurrentDepGraph { let new_node_count_estimate = 102 * prev_graph_node_count / 100 + 200; - let node_intern_event_id = profiler + let node_intern_event_id = session + .prof .get_or_alloc_cached_string("incr_comp_intern_dep_graph_node") .map(EventId::from_label); @@ -1163,7 +1187,7 @@ impl CurrentDepGraph { record_graph, record_stats, )), - new_node_to_index: Sharded::new(|| { + anon_node_to_index: Sharded::new(|| { FxHashMap::with_capacity_and_hasher( new_node_count_estimate / sharded::SHARDS, Default::default(), @@ -1175,6 +1199,13 @@ impl CurrentDepGraph { forbidden_edge, #[cfg(debug_assertions)] fingerprints: Lock::new(IndexVec::from_elem_n(None, new_node_count_estimate)), + #[cfg(debug_assertions)] + seen_dep_nodes: session.opts.unstable_opts.incremental_verify_ich.then(|| { + Lock::new(FxHashSet::with_capacity_and_hasher( + new_node_count_estimate, + Default::default(), + )) + }), total_read_count: AtomicU64::new(0), total_duplicate_read_count: AtomicU64::new(0), node_intern_event_id, @@ -1200,20 +1231,18 @@ impl CurrentDepGraph { edges: EdgesVec, current_fingerprint: Fingerprint, ) -> DepNodeIndex { - let dep_node_index = match self.new_node_to_index.get_shard_by_value(&key).lock().entry(key) - { - Entry::Occupied(entry) => *entry.get(), - Entry::Vacant(entry) => { - let dep_node_index = - self.encoder.borrow().send(profiler, key, current_fingerprint, edges); - entry.insert(dep_node_index); - dep_node_index - } - }; + let dep_node_index = self.encoder.borrow().send(profiler, key, current_fingerprint, edges); #[cfg(debug_assertions)] self.record_edge(dep_node_index, key, current_fingerprint); + #[cfg(debug_assertions)] + if let Some(ref seen_dep_nodes) = self.seen_dep_nodes { + if !seen_dep_nodes.lock().insert(key) { + panic!("Found duplicate dep-node {key:?}"); + } + } + dep_node_index } @@ -1297,8 +1326,6 @@ impl CurrentDepGraph { prev_graph: &SerializedDepGraph, prev_index: SerializedDepNodeIndex, ) -> DepNodeIndex { - self.debug_assert_not_in_new_nodes(prev_graph, prev_index); - let mut prev_index_to_index = self.prev_index_to_index.lock(); match prev_index_to_index[prev_index] { @@ -1319,19 +1346,6 @@ impl CurrentDepGraph { } } } - - #[inline] - fn debug_assert_not_in_new_nodes( - &self, - prev_graph: &SerializedDepGraph, - prev_index: SerializedDepNodeIndex, - ) { - let node = &prev_graph.index_to_node(prev_index); - debug_assert!( - !self.new_node_to_index.get_shard_by_value(node).lock().contains_key(node), - "node from previous graph present in new node collection" - ); - } } /// The capacity of the `reads` field `SmallVec` diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 87d67c099cedb..b7b217913a006 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -1531,7 +1531,9 @@ options! { incremental_relative_spans: bool = (false, parse_bool, [TRACKED], "hash spans relative to their parent item for incr. comp. (default: no)"), incremental_verify_ich: bool = (false, parse_bool, [UNTRACKED], - "verify incr. comp. hashes of green query instances (default: no)"), + "verify extended properties for incr. comp. (default: no): + - hashes of green query instances; + - hash collisions when creating dep-nodes."), inline_in_all_cgus: Option = (None, parse_opt_bool, [TRACKED], "control whether `#[inline]` functions are in all CGUs"), inline_llvm: bool = (true, parse_bool, [TRACKED], From 4409a516033fed42ad303bc57e5c11be7b0e079b Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 14 Jul 2023 09:15:26 +0000 Subject: [PATCH 2/5] Recover comment. --- compiler/rustc_query_system/src/dep_graph/graph.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 9380e62f73539..788fc48626b74 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -344,6 +344,11 @@ impl DepGraphData { task: fn(Ctxt, A) -> R, hash_result: Option, &R) -> Fingerprint>, ) -> (R, DepNodeIndex) { + // 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). self.assert_nonexistent_node(&key, || { format!( "forcing query with already existing `DepNode`\n\ From 3ca19f63313f5a15d314bf292129b300190d7538 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 14 Jul 2023 09:23:56 +0000 Subject: [PATCH 3/5] Fortify check. --- .../rustc_codegen_cranelift/src/driver/aot.rs | 2 +- compiler/rustc_codegen_ssa/src/base.rs | 2 +- .../rustc_query_system/src/dep_graph/graph.rs | 46 ++++++++++++------- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/driver/aot.rs b/compiler/rustc_codegen_cranelift/src/driver/aot.rs index af9297ee6800f..0425b86665f19 100644 --- a/compiler/rustc_codegen_cranelift/src/driver/aot.rs +++ b/compiler/rustc_codegen_cranelift/src/driver/aot.rs @@ -522,7 +522,7 @@ fn determine_cgu_reuse<'tcx>(tcx: TyCtxt<'tcx>, cgu: &CodegenUnit<'tcx>) -> CguR // know that later). If we are not doing LTO, there is only one optimized // version of each module, so we re-use that. let dep_node = cgu.codegen_dep_node(tcx); - tcx.dep_graph.assert_nonexistent_node(&dep_node, || { + tcx.dep_graph.assert_dep_node_not_yet_allocated_in_current_session(&dep_node, || { format!( "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", cgu.name() diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index e3d46b8c052e4..aa587ba658a2e 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -1031,7 +1031,7 @@ fn determine_cgu_reuse<'tcx>(tcx: TyCtxt<'tcx>, cgu: &CodegenUnit<'tcx>) -> CguR // know that later). If we are not doing LTO, there is only one optimized // version of each module, so we re-use that. let dep_node = cgu.codegen_dep_node(tcx); - tcx.dep_graph.assert_nonexistent_node(&dep_node, || { + tcx.dep_graph.assert_dep_node_not_yet_allocated_in_current_session(&dep_node, || { format!( "CompileCodegenUnit dep-node for CGU `{}` already exists before marking.", cgu.name() diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 788fc48626b74..15cb3a06a6457 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -349,7 +349,7 @@ impl DepGraphData { // in `DepGraph::try_mark_green()`. // 2. Two distinct query keys get mapped to the same `DepNode` // (see for example #48923). - self.assert_nonexistent_node(&key, || { + self.assert_dep_node_not_yet_allocated_in_current_session(&key, || { format!( "forcing query with already existing `DepNode`\n\ - query-key: {arg:?}\n\ @@ -647,14 +647,19 @@ impl DepGraph { } impl DepGraphData { - fn assert_nonexistent_node( + fn assert_dep_node_not_yet_allocated_in_current_session( &self, _dep_node: &DepNode, _msg: impl FnOnce() -> S, ) { #[cfg(debug_assertions)] - if let Some(seen_dep_nodes) = &self.current.seen_dep_nodes { - let seen = seen_dep_nodes.lock().contains(_dep_node); + if let Some(prev_index) = self.previous.node_to_index_opt(_dep_node) { + let current = self.current.prev_index_to_index.lock()[prev_index]; + assert!(current.is_none(), "{}", _msg()) + } else if let Some(nodes_newly_allocated_in_current_session) = + &self.current.nodes_newly_allocated_in_current_session + { + let seen = nodes_newly_allocated_in_current_session.lock().contains(_dep_node); assert!(!seen, "{}", _msg()); } } @@ -871,7 +876,7 @@ impl DepGraphData { let frame = MarkFrame { index: prev_dep_node_index, parent: frame }; #[cfg(not(parallel_compiler))] - self.assert_nonexistent_node(dep_node, || { + self.assert_dep_node_not_yet_allocated_in_current_session(dep_node, || { format!("trying to mark existing {dep_node:?} as green") }); #[cfg(not(parallel_compiler))] @@ -970,7 +975,7 @@ impl DepGraph { self.node_color(dep_node).is_some_and(|c| c.is_green()) } - pub fn assert_nonexistent_node( + pub fn assert_dep_node_not_yet_allocated_in_current_session( &self, dep_node: &DepNode, msg: impl FnOnce() -> S, @@ -978,7 +983,7 @@ impl DepGraph { if cfg!(debug_assertions) && let Some(data) = &self.data { - data.assert_nonexistent_node(dep_node, msg) + data.assert_dep_node_not_yet_allocated_in_current_session(dep_node, msg) } } @@ -1120,8 +1125,11 @@ pub(super) struct CurrentDepGraph { /// Used to verify the absence of hash collisions among DepNodes. /// This field is only `Some` if the `-Z incremental_verify_ich` option is present. + /// + /// The map contains all DepNodes that have been allocated in the current session so far and + /// for which there is no equivalent in the previous session. #[cfg(debug_assertions)] - seen_dep_nodes: Option>>>, + nodes_newly_allocated_in_current_session: Option>>>, /// Anonymous `DepNode`s are nodes whose IDs we compute from the list of /// their edges. This has the beneficial side-effect that multiple anonymous @@ -1205,12 +1213,16 @@ impl CurrentDepGraph { #[cfg(debug_assertions)] fingerprints: Lock::new(IndexVec::from_elem_n(None, new_node_count_estimate)), #[cfg(debug_assertions)] - seen_dep_nodes: session.opts.unstable_opts.incremental_verify_ich.then(|| { - Lock::new(FxHashSet::with_capacity_and_hasher( - new_node_count_estimate, - Default::default(), - )) - }), + nodes_newly_allocated_in_current_session: session + .opts + .unstable_opts + .incremental_verify_ich + .then(|| { + Lock::new(FxHashSet::with_capacity_and_hasher( + new_node_count_estimate, + Default::default(), + )) + }), total_read_count: AtomicU64::new(0), total_duplicate_read_count: AtomicU64::new(0), node_intern_event_id, @@ -1242,8 +1254,10 @@ impl CurrentDepGraph { self.record_edge(dep_node_index, key, current_fingerprint); #[cfg(debug_assertions)] - if let Some(ref seen_dep_nodes) = self.seen_dep_nodes { - if !seen_dep_nodes.lock().insert(key) { + if let Some(ref nodes_newly_allocated_in_current_session) = + self.nodes_newly_allocated_in_current_session + { + if !nodes_newly_allocated_in_current_session.lock().insert(key) { panic!("Found duplicate dep-node {key:?}"); } } From 3a8b357bba1fa8c97fa95a9a745b3dc972a91725 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 14 Jul 2023 09:25:47 +0000 Subject: [PATCH 4/5] Do not cfg-gate the check. --- .../rustc_query_system/src/dep_graph/graph.rs | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 15cb3a06a6457..d0b5a7a1860f0 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -649,18 +649,17 @@ impl DepGraph { impl DepGraphData { fn assert_dep_node_not_yet_allocated_in_current_session( &self, - _dep_node: &DepNode, - _msg: impl FnOnce() -> S, + dep_node: &DepNode, + msg: impl FnOnce() -> S, ) { - #[cfg(debug_assertions)] - if let Some(prev_index) = self.previous.node_to_index_opt(_dep_node) { + if let Some(prev_index) = self.previous.node_to_index_opt(dep_node) { let current = self.current.prev_index_to_index.lock()[prev_index]; - assert!(current.is_none(), "{}", _msg()) + assert!(current.is_none(), "{}", msg()) } else if let Some(nodes_newly_allocated_in_current_session) = &self.current.nodes_newly_allocated_in_current_session { - let seen = nodes_newly_allocated_in_current_session.lock().contains(_dep_node); - assert!(!seen, "{}", _msg()); + let seen = nodes_newly_allocated_in_current_session.lock().contains(dep_node); + assert!(!seen, "{}", msg()); } } @@ -980,9 +979,7 @@ impl DepGraph { dep_node: &DepNode, msg: impl FnOnce() -> S, ) { - if cfg!(debug_assertions) - && let Some(data) = &self.data - { + if let Some(data) = &self.data { data.assert_dep_node_not_yet_allocated_in_current_session(dep_node, msg) } } @@ -1128,7 +1125,6 @@ pub(super) struct CurrentDepGraph { /// /// The map contains all DepNodes that have been allocated in the current session so far and /// for which there is no equivalent in the previous session. - #[cfg(debug_assertions)] nodes_newly_allocated_in_current_session: Option>>>, /// Anonymous `DepNode`s are nodes whose IDs we compute from the list of @@ -1212,7 +1208,6 @@ impl CurrentDepGraph { forbidden_edge, #[cfg(debug_assertions)] fingerprints: Lock::new(IndexVec::from_elem_n(None, new_node_count_estimate)), - #[cfg(debug_assertions)] nodes_newly_allocated_in_current_session: session .opts .unstable_opts @@ -1253,7 +1248,6 @@ impl CurrentDepGraph { #[cfg(debug_assertions)] self.record_edge(dep_node_index, key, current_fingerprint); - #[cfg(debug_assertions)] if let Some(ref nodes_newly_allocated_in_current_session) = self.nodes_newly_allocated_in_current_session { From 9227452836289c7a75769c912d90e51b8eeda185 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 14 Jul 2023 09:29:22 +0000 Subject: [PATCH 5/5] Recover another check. --- .../rustc_query_system/src/dep_graph/graph.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index d0b5a7a1860f0..7853d7fa86cab 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -1339,6 +1339,8 @@ impl CurrentDepGraph { prev_graph: &SerializedDepGraph, prev_index: SerializedDepNodeIndex, ) -> DepNodeIndex { + self.debug_assert_not_in_new_nodes(prev_graph, prev_index); + let mut prev_index_to_index = self.prev_index_to_index.lock(); match prev_index_to_index[prev_index] { @@ -1359,6 +1361,22 @@ impl CurrentDepGraph { } } } + + #[inline] + fn debug_assert_not_in_new_nodes( + &self, + prev_graph: &SerializedDepGraph, + prev_index: SerializedDepNodeIndex, + ) { + let node = &prev_graph.index_to_node(prev_index); + debug_assert!( + !self + .nodes_newly_allocated_in_current_session + .as_ref() + .map_or(false, |set| set.lock().contains(node)), + "node from previous graph present in new node collection" + ); + } } /// The capacity of the `reads` field `SmallVec`