From fe89f3236c08abd8fd2c81cdd2f41ff2066f13ac Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 18 Mar 2021 19:26:08 +0100 Subject: [PATCH] Address review. --- .../src/persist/dirty_clean.rs | 2 - .../rustc_incremental/src/persist/save.rs | 6 +- .../rustc_query_system/src/dep_graph/graph.rs | 72 +++++++++---------- .../rustc_query_system/src/dep_graph/query.rs | 3 +- .../src/dep_graph/serialized.rs | 17 ++--- 5 files changed, 45 insertions(+), 55 deletions(-) diff --git a/compiler/rustc_incremental/src/persist/dirty_clean.rs b/compiler/rustc_incremental/src/persist/dirty_clean.rs index 145c168f8c4..e7bd488af8e 100644 --- a/compiler/rustc_incremental/src/persist/dirty_clean.rs +++ b/compiler/rustc_incremental/src/persist/dirty_clean.rs @@ -391,8 +391,6 @@ impl DirtyCleanVisitor<'tcx> { fn assert_clean(&self, item_span: Span, dep_node: DepNode) { debug!("assert_clean({:?})", dep_node); - // if the node wasn't previously evaluated and now is (or vice versa), - // then the node isn't actually clean or dirty. if self.tcx.dep_graph.is_red(&dep_node) { let dep_node_str = self.dep_node_str(&dep_node); self.tcx diff --git a/compiler/rustc_incremental/src/persist/save.rs b/compiler/rustc_incremental/src/persist/save.rs index d80397970ac..23bd63a37d6 100644 --- a/compiler/rustc_incremental/src/persist/save.rs +++ b/compiler/rustc_incremental/src/persist/save.rs @@ -34,10 +34,8 @@ pub fn save_dep_graph(tcx: TyCtxt<'_>) { let dep_graph_path = dep_graph_path(sess); let staging_dep_graph_path = staging_dep_graph_path(sess); - join( - || sess.time("assert_dep_graph", || crate::assert_dep_graph(tcx)), - || sess.time("check_dirty_clean", || dirty_clean::check_dirty_clean_annotations(tcx)), - ); + sess.time("assert_dep_graph", || crate::assert_dep_graph(tcx)); + sess.time("check_dirty_clean", || dirty_clean::check_dirty_clean_annotations(tcx)); if sess.opts.debugging_opts.incremental_info { tcx.dep_graph.print_incremental_info() diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 295b2a97e4c..04def909131 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -626,11 +626,10 @@ impl DepGraph { // There may be multiple threads trying to mark the same dep node green concurrently - let dep_node_index = { - // We allocating an entry for the node in the current dependency graph and - // adding all the appropriate edges imported from the previous graph - data.current.intern_dark_green_node(&data.previous, prev_dep_node_index) - }; + // We allocating an entry for the node in the current dependency graph and + // adding all the appropriate edges imported from the previous graph + let dep_node_index = + data.current.promote_node_and_deps_to_current(&data.previous, prev_dep_node_index); // ... emitting any stored diagnostic ... @@ -713,7 +712,7 @@ impl DepGraph { } } - // Returns true if the given node has been marked as green during the + // Returns true if the given node has been marked as red during the // current compilation session. Used in various assertions pub fn is_red(&self, dep_node: &DepNode) -> bool { self.node_color(dep_node) == Some(DepNodeColor::Red) @@ -833,17 +832,11 @@ rustc_index::newtype_index! { /// will be populated as we run queries or tasks. We never remove nodes from the /// graph: they are only added. /// -/// The nodes in it are identified by a `DepNodeIndex`. Internally, this maps to -/// a `HybridIndex`, which identifies which collection in the `data` field -/// contains a node's data. Which collection is used for a node depends on -/// whether the node was present in the `PreviousDepGraph`, and if so, the color -/// of the node. Each type of node can share more or less data with the previous -/// graph. When possible, we can store just the index of the node in the -/// previous graph, rather than duplicating its data in our own collections. -/// This is important, because these graph structures are some of the largest in -/// the compiler. +/// The nodes in it are identified by a `DepNodeIndex`. We avoid keeping the nodes +/// in memory. This is important, because these graph structures are some of the +/// largest in the compiler. /// -/// For the same reason, we also avoid storing `DepNode`s more than once as map +/// 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 /// graph, and we map nodes in the previous graph to indices via a two-step /// mapping. `PreviousDepGraph` maps from `DepNode` to `SerializedDepNodeIndex`, @@ -939,6 +932,15 @@ impl CurrentDepGraph { } } + #[cfg(debug_assertions)] + fn record_edge(&self, dep_node_index: DepNodeIndex, key: DepNode) { + if let Some(forbidden_edge) = &self.forbidden_edge { + forbidden_edge.index_to_node.lock().insert(dep_node_index, key); + } + } + + /// Writes the node to the current dep-graph and allocates a `DepNodeIndex` for it. + /// Assumes that this is a node that has no equivalent in the previous dep-graph. fn intern_new_node( &self, key: DepNode, @@ -951,9 +953,7 @@ impl CurrentDepGraph { let dep_node_index = self.encoder.borrow().send(key, current_fingerprint, edges); entry.insert(dep_node_index); #[cfg(debug_assertions)] - if let Some(forbidden_edge) = &self.forbidden_edge { - forbidden_edge.index_to_node.lock().insert(dep_node_index, key); - } + self.record_edge(dep_node_index, key); dep_node_index } } @@ -964,20 +964,20 @@ impl CurrentDepGraph { prev_graph: &PreviousDepGraph, key: DepNode, edges: EdgesVec, - current_fingerprint: Option, + fingerprint: Option, print_status: bool, ) -> (DepNodeIndex, Option<(SerializedDepNodeIndex, DepNodeColor)>) { let print_status = cfg!(debug_assertions) && print_status; if let Some(prev_index) = prev_graph.node_to_index_opt(&key) { // Determine the color and index of the new `DepNode`. - if let Some(current_fingerprint) = current_fingerprint { - if current_fingerprint == prev_graph.fingerprint_by_index(prev_index) { + if let Some(fingerprint) = fingerprint { + if fingerprint == prev_graph.fingerprint_by_index(prev_index) { if print_status { eprintln!("[task::green] {:?}", key); } - // This is a light green node: it existed in the previous compilation, + // This is a green node: it existed in the previous compilation, // its query was re-executed, and it has the same result as before. let mut prev_index_to_index = self.prev_index_to_index.lock(); @@ -985,16 +985,14 @@ impl CurrentDepGraph { Some(dep_node_index) => dep_node_index, None => { let dep_node_index = - self.encoder.borrow().send(key, current_fingerprint, edges); + self.encoder.borrow().send(key, fingerprint, edges); prev_index_to_index[prev_index] = Some(dep_node_index); dep_node_index } }; #[cfg(debug_assertions)] - if let Some(forbidden_edge) = &self.forbidden_edge { - forbidden_edge.index_to_node.lock().insert(dep_node_index, key); - } + self.record_edge(dep_node_index, key); (dep_node_index, Some((prev_index, DepNodeColor::Green(dep_node_index)))) } else { if print_status { @@ -1009,16 +1007,14 @@ impl CurrentDepGraph { Some(dep_node_index) => dep_node_index, None => { let dep_node_index = - self.encoder.borrow().send(key, current_fingerprint, edges); + self.encoder.borrow().send(key, fingerprint, edges); prev_index_to_index[prev_index] = Some(dep_node_index); dep_node_index } }; #[cfg(debug_assertions)] - if let Some(forbidden_edge) = &self.forbidden_edge { - forbidden_edge.index_to_node.lock().insert(dep_node_index, key); - } + self.record_edge(dep_node_index, key); (dep_node_index, Some((prev_index, DepNodeColor::Red))) } } else { @@ -1043,9 +1039,7 @@ impl CurrentDepGraph { }; #[cfg(debug_assertions)] - if let Some(forbidden_edge) = &self.forbidden_edge { - forbidden_edge.index_to_node.lock().insert(dep_node_index, key); - } + self.record_edge(dep_node_index, key); (dep_node_index, Some((prev_index, DepNodeColor::Red))) } } else { @@ -1053,16 +1047,16 @@ impl CurrentDepGraph { eprintln!("[task::new] {:?}", key); } - let current_fingerprint = current_fingerprint.unwrap_or(Fingerprint::ZERO); + let fingerprint = fingerprint.unwrap_or(Fingerprint::ZERO); // This is a new node: it didn't exist in the previous compilation session. - let dep_node_index = self.intern_new_node(key, edges, current_fingerprint); + let dep_node_index = self.intern_new_node(key, edges, fingerprint); (dep_node_index, None) } } - fn intern_dark_green_node( + fn promote_node_and_deps_to_current( &self, prev_graph: &PreviousDepGraph, prev_index: SerializedDepNodeIndex, @@ -1086,9 +1080,7 @@ impl CurrentDepGraph { ); prev_index_to_index[prev_index] = Some(dep_node_index); #[cfg(debug_assertions)] - if let Some(forbidden_edge) = &self.forbidden_edge { - forbidden_edge.index_to_node.lock().insert(dep_node_index, key); - } + self.record_edge(dep_node_index, key); dep_node_index } } diff --git a/compiler/rustc_query_system/src/dep_graph/query.rs b/compiler/rustc_query_system/src/dep_graph/query.rs index 0fe3748e386..27b3b5e1366 100644 --- a/compiler/rustc_query_system/src/dep_graph/query.rs +++ b/compiler/rustc_query_system/src/dep_graph/query.rs @@ -32,7 +32,8 @@ impl DepGraphQuery { for &target in edges.iter() { let target = self.dep_index_to_index[target]; - // Skip missing edges. + // We may miss the edges that are pushed while the `DepGraphQuery` is being accessed. + // Skip them to issues. if let Some(target) = target { self.graph.add_edge(source, target, ()); } diff --git a/compiler/rustc_query_system/src/dep_graph/serialized.rs b/compiler/rustc_query_system/src/dep_graph/serialized.rs index 663113543fc..aeb0e2b0da1 100644 --- a/compiler/rustc_query_system/src/dep_graph/serialized.rs +++ b/compiler/rustc_query_system/src/dep_graph/serialized.rs @@ -73,7 +73,7 @@ impl<'a, K: DepKind + Decodable>> Decodable) -> Result, String> { - let position = d.position(); + let start_position = d.position(); // The last 16 bytes are the node count and edge count. debug!("position: {:?}", d.position()); @@ -85,7 +85,7 @@ impl<'a, K: DepKind + Decodable>> Decodable { edge_counter: u64, } -struct EncodingStatus { +struct EncoderState { encoder: FileEncoder, total_node_count: usize, total_edge_count: usize, @@ -145,7 +145,7 @@ struct EncodingStatus { stats: Option>>, } -impl EncodingStatus { +impl EncoderState { fn new(encoder: FileEncoder, record_stats: bool) -> Self { Self { encoder, @@ -186,8 +186,9 @@ impl EncodingStatus { debug!(?index, ?node); let encoder = &mut self.encoder; - self.result = - std::mem::replace(&mut self.result, Ok(())).and_then(|()| node.encode(encoder)); + if self.result.is_ok() { + self.result = node.encode(encoder); + } index } @@ -209,7 +210,7 @@ impl EncodingStatus { } pub struct GraphEncoder { - status: Lock>, + status: Lock>, record_graph: Option>>, } @@ -225,7 +226,7 @@ impl> GraphEncoder { } else { None }; - let status = Lock::new(EncodingStatus::new(encoder, record_stats)); + let status = Lock::new(EncoderState::new(encoder, record_stats)); GraphEncoder { status, record_graph } }