Skip to content

Commit

Permalink
incr.comp.: Address review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@michaelwoerister
michaelwoerister committed Oct 4, 2017
1 parent c96d0bf commit 0454a41
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 31 deletions.
79 changes: 54 additions & 25 deletions src/librustc/dep_graph/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
23 changes: 17 additions & 6 deletions src/librustc/ty/maps/plumbing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 0454a41

Please sign in to comment.