incr.comp.: Address review comments.

Author: michaelwoerister

src/librustc/dep_graph/graph.rs

@@ -52,7 +52,7 @@ pub struct DepNodeIndex {
 
 impl Idx for DepNodeIndex {
     fn new(idx: usize) -> Self {
-        assert!((idx & 0xFFFF_FFFF) == idx);
+        debug_assert!((idx & 0xFFFF_FFFF) == idx);
         DepNodeIndex { index: idx as u32 }
     }
     fn index(self) -> usize {
@@ -228,20 +228,31 @@ impl DepGraph {
 
             let current_fingerprint = stable_hasher.finish();
 
-            assert!(self.fingerprints
-                        .borrow_mut()
-                        .insert(key, current_fingerprint)
-                        .is_none());
+            // Store the current fingerprint
+            {
+                let old_value = self.fingerprints
+                                    .borrow_mut()
+                                    .insert(key, current_fingerprint);
+                debug_assert!(old_value.is_none(),
+                              "DepGraph::with_task() - Duplicate fingerprint \
+                               insertion for {:?}", key);
+            }
 
-            let prev_fingerprint = data.previous.fingerprint_of(&key);
+            // Determine the color of the new DepNode.
+            {
+                let prev_fingerprint = data.previous.fingerprint_of(&key);
 
-            let color = if Some(current_fingerprint) == prev_fingerprint {
-                DepNodeColor::Green(dep_node_index)
-            } else {
-                DepNodeColor::Red
-            };
+                let color = if Some(current_fingerprint) == prev_fingerprint {
+                    DepNodeColor::Green(dep_node_index)
+                } else {
+                    DepNodeColor::Red
+                };
 
-            assert!(data.colors.borrow_mut().insert(key, color).is_none());
+                let old_value = data.colors.borrow_mut().insert(key, color);
+                debug_assert!(old_value.is_none(),
+                              "DepGraph::with_task() - Duplicate DepNodeColor \
+                               insertion for {:?}", key);
+            }
 
             (result, dep_node_index)
         } else {
@@ -250,10 +261,12 @@ impl DepGraph {
                 let result = task(cx, arg);
                 let mut stable_hasher = StableHasher::new();
                 result.hash_stable(&mut hcx, &mut stable_hasher);
-                assert!(self.fingerprints
-                            .borrow_mut()
-                            .insert(key, stable_hasher.finish())
-                            .is_none());
+                let old_value = self.fingerprints
+                                    .borrow_mut()
+                                    .insert(key, stable_hasher.finish());
+                debug_assert!(old_value.is_none(),
+                              "DepGraph::with_task() - Duplicate fingerprint \
+                               insertion for {:?}", key);
                 (result, DepNodeIndex::INVALID)
             } else {
                 (task(cx, arg), DepNodeIndex::INVALID)
@@ -549,16 +562,20 @@ impl DepGraph {
         // ... copying the fingerprint from the previous graph too, so we don't
         // have to recompute it ...
         let fingerprint = data.previous.fingerprint_by_index(prev_dep_node_index);
-        assert!(self.fingerprints
-                    .borrow_mut()
-                    .insert(*dep_node, fingerprint)
-                    .is_none());
+        let old_fingerprint = self.fingerprints
+                                  .borrow_mut()
+                                  .insert(*dep_node, fingerprint);
+        debug_assert!(old_fingerprint.is_none(),
+                      "DepGraph::try_mark_green() - Duplicate fingerprint \
+                      insertion for {:?}", dep_node);
 
         // ... and finally storing a "Green" entry in the color map.
-        assert!(data.colors
-                    .borrow_mut()
-                    .insert(*dep_node, DepNodeColor::Green(dep_node_index))
-                    .is_none());
+        let old_color = data.colors
+                            .borrow_mut()
+                            .insert(*dep_node, DepNodeColor::Green(dep_node_index));
+        debug_assert!(old_color.is_none(),
+                      "DepGraph::try_mark_green() - Duplicate DepNodeColor \
+                      insertion for {:?}", dep_node);
 
         debug!("try_mark_green({:?}) - END - successfully marked as green", dep_node.kind);
         Some(dep_node_index)
@@ -637,9 +654,21 @@ pub(super) struct CurrentDepGraph {
     nodes: IndexVec<DepNodeIndex, DepNode>,
     edges: IndexVec<DepNodeIndex, Vec<DepNodeIndex>>,
     node_to_node_index: FxHashMap<DepNode, DepNodeIndex>,
-    anon_id_seed: Fingerprint,
     task_stack: Vec<OpenTask>,
     forbidden_edge: Option<EdgeFilter>,
+
+    // Anonymous DepNodes are nodes the ID of which 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
+    // dependency graph. However, the merging of nodes can lead to a subtle
+    // problem during red-green marking: The color of an anonymous node from
+    // the current session might "shadow" the color of the node with the same
+    // ID from the previous session. In order to side-step this problem, we make
+    // sure that anon-node IDs allocated in different sessions don't overlap.
+    // This is implemented by mixing a session-key into the ID fingerprint of
+    // each anon node. The session-key is just a random number generated when
+    // the DepGraph is created.
+    anon_id_seed: Fingerprint,
 }
 
 impl CurrentDepGraph {

src/librustc/ty/maps/plumbing.rs

@@ -609,10 +609,19 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
     use ty::maps::keys::Key;
     use hir::def_id::LOCAL_CRATE;
 
-    // We should never get into the situation of having to force this from the DepNode.
-    // Since we cannot reconstruct the query key, we would always end up having to evaluate
-    // the first caller of this query that *is* reconstructible. This might very well be
-    // compile_codegen_unit() in which case we'd lose all re-use.
+    // We must avoid ever having to call force_from_dep_node() for a
+    // DepNode::CodegenUnit:
+    // Since we cannot reconstruct the query key of a DepNode::CodegenUnit, we
+    // would always end up having to evaluate the first caller of the
+    // `codegen_unit` query that *is* reconstructible. This might very well be
+    // the `compile_codegen_unit` query, thus re-translating the whole CGU just
+    // to re-trigger calling the `codegen_unit` query with the right key. At
+    // that point we would already have re-done all the work we are trying to
+    // avoid doing in the first place.
+    // The solution is simple: Just explicitly call the `codegen_unit` query for
+    // each CGU, right after partitioning. This way `try_mark_green` will always
+    // hit the cache instead of having to go through `force_from_dep_node`.
+    // This assertion makes sure, we actually keep applying the solution above.
     debug_assert!(dep_node.kind != DepKind::CodegenUnit,
                   "calling force_from_dep_node() on DepKind::CodegenUnit");
 
@@ -666,6 +675,8 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
         }
     };
 
+    // FIXME(#45015): We should try move this boilerplate code into a macro
+    //                somehow.
     match dep_node.kind {
         // These are inputs that are expected to be pre-allocated and that
         // should therefore always be red or green already
@@ -695,13 +706,13 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
         DepKind::CodegenUnit |
         DepKind::CompileCodegenUnit |
 
-        // This one is just odd
+        // These are just odd
         DepKind::Null |
         DepKind::WorkProduct => {
             bug!("force_from_dep_node() - Encountered {:?}", dep_node.kind)
         }
 
-        // These is not a queries
+        // These are not queries
         DepKind::CoherenceCheckTrait |
         DepKind::ItemVarianceConstraints => {
             return false