Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tombstone checks to make sure incr-comp reads aren't dirtied #40362

Closed
wants to merge 1 commit into from
Closed

Add tombstone checks to make sure incr-comp reads aren't dirtied #40362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 59 additions & 9 deletions src/librustc/dep_graph/dep_tracking_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

use hir::def_id::DefId;
use rustc_data_structures::fx::FxHashMap;

#[cfg(debug_assertions)]
use rustc_data_structures::fx::FxHashSet;

use std::cell::RefCell;
use std::collections::hash_map::Entry;
use std::ops::Index;
Expand All @@ -26,6 +30,8 @@ pub struct DepTrackingMap<M: DepTrackingMapConfig> {
phantom: PhantomData<M>,
graph: DepGraph,
map: FxHashMap<M::Key, M::Value>,
#[cfg(debug_assertions)]
tombstones: RefCell<FxHashSet<M::Key>>,
}

pub trait DepTrackingMapConfig {
Expand All @@ -35,11 +41,22 @@ pub trait DepTrackingMapConfig {
}

impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
#[cfg(debug_assertions)]
pub fn new(graph: DepGraph) -> DepTrackingMap<M> {
DepTrackingMap {
phantom: PhantomData,
graph: graph,
map: FxHashMap(),
tombstones: RefCell::new(FxHashSet()),
}
}

#[cfg(not(debug_assertions))]
pub fn new(graph: DepGraph) -> DepTrackingMap<M> {
DepTrackingMap {
phantom: PhantomData,
graph: graph,
map: FxHashMap()
map: FxHashMap(),
}
}

Expand All @@ -59,13 +76,31 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {

pub fn get(&self, k: &M::Key) -> Option<&M::Value> {
self.read(k);
self.map.get(k)
let result = self.map.get(k);

#[cfg(debug_assertions)]
{
if result.is_none() {
self.tombstones.borrow_mut().insert(k.clone());
}
}

result
}

pub fn insert(&mut self, k: M::Key, v: M::Value) {
self.write(&k);

// If we ever read a `None` value for this key, we do not want
// to permit a later write to it. The only valid use case for
// this is the memoization pattern: for that, use `memoize()`
// below
#[cfg(debug_assertions)]
assert!(!self.tombstones.borrow().contains(&k),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following lines should probably be wrapped in a block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misread. But a debug_assert() would be more concise.

"inserting to `{0:?}` after reading from `{0:?}`",
M::to_dep_node(&k));
let old_value = self.map.insert(k, v);
assert!(old_value.is_none());
assert!(old_value.is_none(), "inserted value twice");
}

pub fn entry(&mut self, k: M::Key) -> Entry<M::Key, M::Value> {
Expand All @@ -75,7 +110,13 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {

pub fn contains_key(&self, k: &M::Key) -> bool {
self.read(k);
self.map.contains_key(k)
if self.map.contains_key(k) {
true
} else {
#[cfg(debug_assertions)]
self.tombstones.borrow_mut().insert(k.clone());
false
}
}

pub fn keys(&self) -> Vec<M::Key> {
Expand Down Expand Up @@ -125,7 +166,7 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
/// let item_def_id = ccx.tcx.hir.local_def_id(it.id);
/// ccx.tcx.item_types.memoized(item_def_id, || {
/// ccx.tcx.dep_graph.read(DepNode::Hir(item_def_id)); // (*)
/// compute_type_of_item(ccx, item)
/// Some(compute_type_of_item(ccx, item))
/// });
/// }
/// ```
Expand All @@ -134,7 +175,7 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
/// accesses the body of the item `item`, so we register a read
/// from `Hir(item_def_id)`.
fn memoize<OP>(&self, key: M::Key, op: OP) -> M::Value
where OP: FnOnce() -> M::Value
where OP: FnOnce() -> Option<M::Value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks to me like there is no longer a need for this part of the diff -- that is, we can make memoize() take a closure that returns just M::Value again

Copy link
Member Author

@cramertj cramertj Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little hard to see because of how the diff reads on Github, but this call to memoize has a closure that returns None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought @eddyb refactored that use. I wonder if we want to do so in this PR instead. Right now what happens is that we request AssociatedItem(X), which is some summary information about some item X that is part of an impl or trait:

impl Foo {
    type A;
    type X;
    fn b() { ... }
}

To do this, we go up to the enclosing impl (the one shown there) and then iterate over its list of items. Right now, since we are iterating anyway, we would compute the associated item info for A and b() while we're at it (to avoid an O(n^2) blow-up, basically). We have been talking though about trying to refactor into finer-grained tasks that don't have this idea of generating "N results" from one execution.

We could thus change this code to not generate N outputs but just find the one item we actually want. It would be O(N^2) but N here is typically very small.

And we do plan to rework things over time that would mitigate -- for example, moving to a newer system where we wouldn't have to be so careful about our dependencies. The whole motivation here was to only read from the impl when computing this data, and not the items themselves. This way, if the item X changes, we don't have to consider the value as dirty (the parts of X that would affect this value are copied into the impl and hashed there too). But we want to move to a system where we would recompute this value (which is cheap) and then see that, indeed, it has not changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best fix IMO is to move AssociatedItem into the HIR where it belongs.

{
let graph;
{
Expand All @@ -147,9 +188,18 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
}

let _task = graph.in_task(M::to_dep_node(&key));
let result = op();
self.borrow_mut().map.insert(key, result.clone());
result
if let Some(result) = op() {
let old_value = self.borrow_mut().map.insert(key, result.clone());
assert!(old_value.is_none());
result
} else {
self.borrow().map
.get(&key)
.unwrap_or_else(|| bug!(
"memoize closure failed to write to {:?}", M::to_dep_node(&key)
))
.clone()
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/contents.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl fmt::Debug for TypeContents {

impl<'a, 'tcx> ty::TyS<'tcx> {
pub fn type_contents(&'tcx self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> TypeContents {
return tcx.tc_cache.memoize(self, || tc_ty(tcx, self, &mut FxHashMap()));
return tcx.tc_cache.memoize(self, || Some(tc_ty(tcx, self, &mut FxHashMap())));

fn tc_ty<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
ty: Ty<'tcx>,
Expand Down
60 changes: 22 additions & 38 deletions src/librustc/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1633,65 +1633,51 @@ impl<'a, 'gcx, 'tcx> AdtDef {
/// check should catch this case.
fn calculate_sized_constraint_inner(&self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
stack: &mut Vec<DefId>)
stack: &mut Vec<(DefId, bool)>)
-> Ty<'tcx>
{
if let Some(ty) = tcx.maps.adt_sized_constraint.borrow().get(&self.did) {
return ty;
}

// Follow the memoization pattern: push the computation of
// DepNode::SizedConstraint as our current task.
let _task = tcx.dep_graph.in_task(DepNode::SizedConstraint(self.did));
if let Some(index) = stack.iter().position(|pair| pair.0 == self.did) {
stack[index].1 = true;

if stack.contains(&self.did) {
debug!("calculate_sized_constraint: {:?} is recursive", self);
// This should be reported as an error by `check_representable`.
//
// Consider the type as Sized in the meanwhile to avoid
// further errors.
tcx.maps.adt_sized_constraint.borrow_mut().insert(self.did, tcx.types.err);
return tcx.types.err;
}

stack.push(self.did);
tcx.maps.adt_sized_constraint.memoize(self.did, || {
stack.push((self.did, false));

let tys : Vec<_> =
let tys: Vec<_> =
self.variants.iter().flat_map(|v| {
v.fields.last()
}).flat_map(|f| {
let ty = tcx.item_type(f.did);
self.sized_constraint_for_ty(tcx, stack, ty)
}).collect();

let self_ = stack.pop().unwrap();
assert_eq!(self_, self.did);
let (self_, cycle) = stack.pop().unwrap();
assert_eq!(self_, self.did);

let ty = match tys.len() {
_ if tys.references_error() => tcx.types.err,
0 => tcx.types.bool,
1 => tys[0],
_ => tcx.intern_tup(&tys[..], false)
};
let ty = if cycle || tys.references_error() {
tcx.types.err
} else {
match tys.len() {
0 => tcx.types.bool,
1 => tys[0],
_ => tcx.intern_tup(&tys[..], false)
}
};

let old = tcx.maps.adt_sized_constraint.borrow().get(&self.did).cloned();
match old {
Some(old_ty) => {
debug!("calculate_sized_constraint: {:?} recurred", self);
assert_eq!(old_ty, tcx.types.err);
old_ty
}
None => {
debug!("calculate_sized_constraint: {:?} => {:?}", self, ty);
tcx.maps.adt_sized_constraint.borrow_mut().insert(self.did, ty);
ty
}
}
Some(ty)
})
}

fn sized_constraint_for_ty(&self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
stack: &mut Vec<DefId>,
stack: &mut Vec<(DefId, bool)>,
ty: Ty<'tcx>)
-> Vec<Ty<'tcx>> {
let result = match ty.sty {
Expand Down Expand Up @@ -2094,9 +2080,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

// memoize wants us to return something, so return
// the one we generated for this def-id
*self.maps.associated_item.borrow().get(&def_id).unwrap()
None
})
}

Expand Down Expand Up @@ -2176,7 +2160,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
_ => span_bug!(item.span, "associated_item_def_ids: not impl or trait")
};
Rc::new(vec)
Some(Rc::new(vec))
})
}

Expand Down
13 changes: 8 additions & 5 deletions src/librustc/util/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ pub trait MemoizationMap {
/// added into the dep graph. See the `DepTrackingMap` impl for
/// more details!
fn memoize<OP>(&self, key: Self::Key, op: OP) -> Self::Value
where OP: FnOnce() -> Self::Value;
where OP: FnOnce() -> Option<Self::Value>;
}

impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>
Expand All @@ -218,15 +218,18 @@ impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>
type Value = V;

fn memoize<OP>(&self, key: K, op: OP) -> V
where OP: FnOnce() -> V
where OP: FnOnce() -> Option<V>
{
let result = self.borrow().get(&key).cloned();
match result {
Some(result) => result,
None => {
let result = op();
self.borrow_mut().insert(key, result.clone());
result
if let Some(result) = op() {
self.borrow_mut().insert(key, result.clone());
result
} else {
self.borrow().get(&key).unwrap().clone()
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ pub fn fulfill_obligation<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
let vtable = infcx.drain_fulfillment_cx_or_panic(span, &mut fulfill_cx, &vtable);

info!("Cache miss: {:?} => {:?}", trait_ref, vtable);
vtable
Some(vtable)
})
})
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/monomorphize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl<'a, 'b, 'gcx> TypeFolder<'gcx, 'gcx> for AssociatedTypeNormalizer<'a, 'b, '
} else {
self.shared.project_cache().memoize(ty, || {
debug!("AssociatedTypeNormalizer: ty={:?}", ty);
self.shared.tcx().normalize_associated_type(&ty)
Some(self.shared.tcx().normalize_associated_type(&ty))
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ fn convert_enum_variant_types<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
prev_discr = Some(if let Some(e) = variant.node.disr_expr {
let expr_did = tcx.hir.local_def_id(e.node_id);
let result = tcx.maps.monomorphic_const_eval.memoize(expr_did, || {
evaluate_disr_expr(tcx, e)
Some(evaluate_disr_expr(tcx, e))
});

match result {
Expand Down