Revamp the fulfillment cache tracking to only cache trait-refs, which

was the major use-case, and to update the dep-graph. Other kinds of
predicates are now excluded from the cache because there is no easy way
to make a good dep-graph node for them, and because they are not
believed to be that useful. :)

Fixes #30741. (However, the test still gives wrong result for trans,
for an independent reason which is fixed in the next commit.)

Author: nikomatsakis

src/librustc/middle/traits/fulfill.rs

@@ -8,6 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+use dep_graph::DepGraph;
 use middle::infer::InferCtxt;
 use middle::ty::{self, Ty, TypeFoldable};
 use rustc_data_structures::obligation_forest::{Backtrace, ObligationForest, Error};
@@ -30,7 +31,12 @@ use super::select::SelectionContext;
 use super::Unimplemented;
 use super::util::predicate_for_builtin_bound;
 
-pub struct FulfilledPredicates<'tcx> {
+pub struct GlobalFulfilledPredicates<'tcx> {
+    set: FnvHashSet<ty::PolyTraitPredicate<'tcx>>,
+    dep_graph: DepGraph,
+}
+
+pub struct LocalFulfilledPredicates<'tcx> {
     set: FnvHashSet<ty::Predicate<'tcx>>
 }
 
@@ -56,7 +62,7 @@ pub struct FulfillmentContext<'tcx> {
     // initially-distinct type variables are unified after being
     // inserted. Deduplicating the predicate set on selection had a
     // significant performance cost the last time I checked.
-    duplicate_set: FulfilledPredicates<'tcx>,
+    duplicate_set: LocalFulfilledPredicates<'tcx>,
 
     // A list of all obligations that have been registered with this
     // fulfillment context.
@@ -106,7 +112,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
     /// Creates a new fulfillment context.
     pub fn new() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
-            duplicate_set: FulfilledPredicates::new(),
+            duplicate_set: LocalFulfilledPredicates::new(),
             predicates: ObligationForest::new(),
             region_obligations: NodeMap(),
         }
@@ -240,7 +246,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
         // local cache).  This is because the tcx cache maintains the
         // invariant that it only contains things that have been
         // proven, and we have not yet proven that `predicate` holds.
-        if predicate.is_global() && tcx.fulfilled_predicates.borrow().is_duplicate(predicate) {
+        if tcx.fulfilled_predicates.borrow().check_duplicate(predicate) {
             return true;
         }
 
@@ -283,10 +289,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
             // these are obligations that were proven to be true.
             for pending_obligation in outcome.completed {
                 let predicate = &pending_obligation.obligation.predicate;
-                if predicate.is_global() {
-                    selcx.tcx().fulfilled_predicates.borrow_mut()
-                                                    .is_duplicate_or_add(predicate);
-                }
+                selcx.tcx().fulfilled_predicates.borrow_mut().add_if_global(predicate);
             }
 
             errors.extend(
@@ -329,17 +332,16 @@ fn process_predicate<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>,
             // However, this is a touch tricky, so I'm doing something
             // a bit hackier for now so that the `huge-struct.rs` passes.
 
+            let tcx = selcx.tcx();
+
             let retain_vec: Vec<_> = {
                 let mut dedup = FnvHashSet();
                 v.iter()
                  .map(|o| {
                      // Screen out obligations that we know globally
                      // are true. This should really be the DAG check
                      // mentioned above.
-                     if
-                         o.predicate.is_global() &&
-                         selcx.tcx().fulfilled_predicates.borrow().is_duplicate(&o.predicate)
-                     {
+                     if tcx.fulfilled_predicates.borrow().check_duplicate(&o.predicate) {
                          return false;
                      }
 
@@ -611,22 +613,62 @@ fn register_region_obligation<'tcx>(t_a: Ty<'tcx>,
 
 }
 
-impl<'tcx> FulfilledPredicates<'tcx> {
-    pub fn new() -> FulfilledPredicates<'tcx> {
-        FulfilledPredicates {
+impl<'tcx> LocalFulfilledPredicates<'tcx> {
+    pub fn new() -> LocalFulfilledPredicates<'tcx> {
+        LocalFulfilledPredicates {
             set: FnvHashSet()
         }
     }
 
-    pub fn is_duplicate(&self, key: &ty::Predicate<'tcx>) -> bool {
-        self.set.contains(key)
-    }
-
     fn is_duplicate_or_add(&mut self, key: &ty::Predicate<'tcx>) -> bool {
+        // For a `LocalFulfilledPredicates`, if we find a match, we
+        // don't need to add a read edge to the dep-graph. This is
+        // because it means that the predicate has already been
+        // considered by this `FulfillmentContext`, and hence the
+        // containing task will already have an edge. (Here we are
+        // assuming each `FulfillmentContext` only gets used from one
+        // task; but to do otherwise makes no sense)
         !self.set.insert(key.clone())
     }
 }
 
+impl<'tcx> GlobalFulfilledPredicates<'tcx> {
+    pub fn new(dep_graph: DepGraph) -> GlobalFulfilledPredicates<'tcx> {
+        GlobalFulfilledPredicates {
+            set: FnvHashSet(),
+            dep_graph: dep_graph,
+        }
+    }
+
+    pub fn check_duplicate(&self, key: &ty::Predicate<'tcx>) -> bool {
+        if let ty::Predicate::Trait(ref data) = *key {
+            // For the global predicate registry, when we find a match, it
+            // may have been computed by some other task, so we want to
+            // add a read from the node corresponding to the predicate
+            // processing to make sure we get the transitive dependencies.
+            if self.set.contains(data) {
+                debug_assert!(data.is_global());
+                self.dep_graph.read(data.dep_node());
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    fn add_if_global(&mut self, key: &ty::Predicate<'tcx>) {
+        if let ty::Predicate::Trait(ref data) = *key {
+            // We only add things to the global predicate registry
+            // after the current task has proved them, and hence
+            // already has the required read edges, so we don't need
+            // to add any more edges here.
+            if data.is_global() {
+                self.set.insert(data.clone());
+            }
+        }
+    }
+}
+
 fn to_fulfillment_error<'tcx>(
     error: Error<PendingPredicateObligation<'tcx>, FulfillmentErrorCode<'tcx>>)
     -> FulfillmentError<'tcx>

src/librustc/middle/traits/mod.rs

@@ -15,7 +15,6 @@ pub use self::FulfillmentErrorCode::*;
 pub use self::Vtable::*;
 pub use self::ObligationCauseCode::*;
 
-use dep_graph::DepNode;
 use middle::def_id::DefId;
 use middle::free_region::FreeRegionMap;
 use middle::subst;
@@ -36,7 +35,7 @@ pub use self::error_reporting::report_object_safety_error;
 pub use self::coherence::orphan_check;
 pub use self::coherence::overlapping_impls;
 pub use self::coherence::OrphanCheckErr;
-pub use self::fulfill::{FulfillmentContext, FulfilledPredicates, RegionObligation};
+pub use self::fulfill::{FulfillmentContext, GlobalFulfilledPredicates, RegionObligation};
 pub use self::project::MismatchedProjectionTypes;
 pub use self::project::normalize;
 pub use self::project::Normalized;
@@ -616,11 +615,6 @@ impl<'tcx> FulfillmentError<'tcx> {
 }
 
 impl<'tcx> TraitObligation<'tcx> {
-    /// Creates the dep-node for selecting/evaluating this trait reference.
-    fn dep_node(&self) -> DepNode {
-        DepNode::TraitSelect(self.predicate.def_id())
-    }
-
     fn self_ty(&self) -> ty::Binder<Ty<'tcx>> {
         ty::Binder(self.predicate.skip_binder().self_ty())
     }

src/librustc/middle/traits/select.rs

@@ -307,7 +307,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         debug!("select({:?})", obligation);
         assert!(!obligation.predicate.has_escaping_regions());
 
-        let dep_node = obligation.dep_node();
+        let dep_node = obligation.predicate.dep_node();
         let _task = self.tcx().dep_graph.in_task(dep_node);
 
         let stack = self.push_stack(TraitObligationStackList::empty(), obligation);
@@ -462,7 +462,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         // have been proven elsewhere. This cache only contains
         // predicates that are global in scope and hence unaffected by
         // the current environment.
-        if self.tcx().fulfilled_predicates.borrow().is_duplicate(&obligation.predicate) {
+        if self.tcx().fulfilled_predicates.borrow().check_duplicate(&obligation.predicate) {
             return EvaluatedToOk;
         }
 

src/librustc/middle/ty/context.rs

@@ -367,7 +367,7 @@ pub struct ctxt<'tcx> {
     /// This is used to avoid duplicate work. Predicates are only
     /// added to this set when they mention only "global" names
     /// (i.e., no type or lifetime parameters).
-    pub fulfilled_predicates: RefCell<traits::FulfilledPredicates<'tcx>>,
+    pub fulfilled_predicates: RefCell<traits::GlobalFulfilledPredicates<'tcx>>,
 
     /// Caches the representation hints for struct definitions.
     repr_hint_cache: RefCell<DepTrackingMap<maps::ReprHints<'tcx>>>,
@@ -510,6 +510,7 @@ impl<'tcx> ctxt<'tcx> {
         let interner = RefCell::new(FnvHashMap());
         let common_types = CommonTypes::new(&arenas.type_, &interner);
         let dep_graph = DepGraph::new(s.opts.incremental_compilation);
+        let fulfilled_predicates = traits::GlobalFulfilledPredicates::new(dep_graph.clone());
         tls::enter(ctxt {
             arenas: arenas,
             interner: interner,
@@ -532,7 +533,7 @@ impl<'tcx> ctxt<'tcx> {
             adt_defs: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
             predicates: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
             super_predicates: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
-            fulfilled_predicates: RefCell::new(traits::FulfilledPredicates::new()),
+            fulfilled_predicates: RefCell::new(fulfilled_predicates),
             map: map,
             freevars: RefCell::new(freevars),
             tcache: RefCell::new(DepTrackingMap::new(dep_graph.clone())),

src/librustc/middle/ty/mod.rs

@@ -838,6 +838,11 @@ impl<'tcx> TraitPredicate<'tcx> {
         self.trait_ref.def_id
     }
 
+    /// Creates the dep-node for selecting/evaluating this trait reference.
+    fn dep_node(&self) -> DepNode {
+        DepNode::TraitSelect(self.def_id())
+    }
+
     pub fn input_types(&self) -> &[Ty<'tcx>] {
         self.trait_ref.substs.types.as_slice()
     }
@@ -849,8 +854,14 @@ impl<'tcx> TraitPredicate<'tcx> {
 
 impl<'tcx> PolyTraitPredicate<'tcx> {
     pub fn def_id(&self) -> DefId {
+        // ok to skip binder since trait def-id does not care about regions
         self.0.def_id()
     }
+
+    pub fn dep_node(&self) -> DepNode {
+        // ok to skip binder since depnode does not care about regions
+        self.0.dep_node()
+    }
 }
 
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]

src/test/compile-fail/dep-graph-trait-impl.rs

@@ -41,7 +41,7 @@ mod y {
     }
 
     // FIXME(#30741) tcx fulfillment cache not tracked
-    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR no path
+    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR OK
     #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR no path
     pub fn take_foo_with_char() {
         take_foo::<char>('a');
@@ -54,7 +54,7 @@ mod y {
     }
 
     // FIXME(#30741) tcx fulfillment cache not tracked
-    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR no path
+    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR OK
     #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR no path
     pub fn take_foo_with_u32() {
         take_foo::<u32>(22);