Auto merge of #26326 - nikomatsakis:optimize-fulfillment-cache-in-tcx…

…, r=pcwalton

When we successfully resolve a trait reference with no type/lifetime parameters, like `i32: Foo` or `Box<u32>: Sized`, this is in fact globally true. This patch adds a simple global to the tcx to cache such cases. The main advantage of this is really about caching things like `Box<Vec<Foo>>: Sized`. It also points to the need to revamp our caching infrastructure -- the current caches make selection cost cheaper, but we still wind up paying a high cost in the confirmation process, and in particular unrolling out dependent obligations. Moreover, we should probably do caching more uniformly and with a key that takes the where-clauses into account. But that's for later.

For me, this shows up as a reasonably nice win (20%) on Servo's script crate (when built in dev mode). This is not as big as my initial measurements suggested, I think because I was building my rustc with more debugging enabled at the time. I've not yet done follow-up profiling and so forth to see where the new hot spots are. Bootstrap times seem to be largely unaffected.

cc @pcwalton 

This is technically a [breaking-change] in that functions with unsatisfiable where-clauses may now yield errors where before they may have been accepted. Even before, these functions could never have been *called* by actual code. In the future, such functions will probably become illegal altogether, but in this commit they are still accepted, so long as they do not rely on the unsatisfiable where-clauses. As before, the functions still cannot be called in any case.

src/librustc/middle/check_const.rs

@@ -285,7 +285,7 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
     fn check_static_type(&self, e: &ast::Expr) {
         let ty = ty::node_id_to_type(self.tcx, e.id);
         let infcx = infer::new_infer_ctxt(self.tcx);
-        let mut fulfill_cx = traits::FulfillmentContext::new();
+        let mut fulfill_cx = traits::FulfillmentContext::new(false);
         let cause = traits::ObligationCause::new(e.span, e.id, traits::SharedStatic);
         fulfill_cx.register_builtin_bound(&infcx, ty, ty::BoundSync, cause);
         let env = ty::empty_parameter_environment(self.tcx);

src/librustc/middle/traits/fulfill.rs

@@ -28,6 +28,10 @@ use super::select::SelectionContext;
 use super::Unimplemented;
 use super::util::predicate_for_builtin_bound;
 
+pub struct FulfilledPredicates<'tcx> {
+    set: HashSet<ty::Predicate<'tcx>>
+}
+
 /// The fulfillment context is used to drive trait resolution.  It
 /// consists of a list of obligations that must be (eventually)
 /// satisfied. The job is to track which are satisfied, which yielded
@@ -44,7 +48,7 @@ pub struct FulfillmentContext<'tcx> {
     // than the `SelectionCache`: it avoids duplicate errors and
     // permits recursive obligations, which are often generated from
     // traits like `Send` et al.
-    duplicate_set: HashSet<ty::Predicate<'tcx>>,
+    duplicate_set: FulfilledPredicates<'tcx>,
 
     // A list of all obligations that have been registered with this
     // fulfillment context.
@@ -80,6 +84,8 @@ pub struct FulfillmentContext<'tcx> {
     // obligations (otherwise, it's easy to fail to walk to a
     // particular node-id).
     region_obligations: NodeMap<Vec<RegionObligation<'tcx>>>,
+
+    errors_will_be_reported: bool,
 }
 
 #[derive(Clone)]
@@ -90,12 +96,30 @@ pub struct RegionObligation<'tcx> {
 }
 
 impl<'tcx> FulfillmentContext<'tcx> {
-    pub fn new() -> FulfillmentContext<'tcx> {
+    /// Creates a new fulfillment context.
+    ///
+    /// `errors_will_be_reported` indicates whether ALL errors that
+    /// are generated by this fulfillment context will be reported to
+    /// the end user. This is used to inform caching, because it
+    /// allows us to conclude that traits that resolve successfully
+    /// will in fact always resolve successfully (in particular, it
+    /// guarantees that if some dependent obligation encounters a
+    /// problem, compilation will be aborted).  If you're not sure of
+    /// the right value here, pass `false`, as that is the more
+    /// conservative option.
+    ///
+    /// FIXME -- a better option would be to hold back on modifying
+    /// the global cache until we know that all dependent obligations
+    /// are also satisfied. In that case, we could actually remove
+    /// this boolean flag, and we'd also avoid the problem of squelching
+    /// duplicate errors that occur across fns.
+    pub fn new(errors_will_be_reported: bool) -> FulfillmentContext<'tcx> {
         FulfillmentContext {
-            duplicate_set: HashSet::new(),
+            duplicate_set: FulfilledPredicates::new(),
             predicates: Vec::new(),
             attempted_mark: 0,
             region_obligations: NodeMap(),
+            errors_will_be_reported: errors_will_be_reported,
         }
     }
 
@@ -165,7 +189,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
 
         assert!(!obligation.has_escaping_regions());
 
-        if !self.duplicate_set.insert(obligation.predicate.clone()) {
+        if self.is_duplicate_or_add(infcx.tcx, &obligation.predicate) {
             debug!("register_predicate({}) -- already seen, skip", obligation.repr(infcx.tcx));
             return;
         }
@@ -231,6 +255,28 @@ impl<'tcx> FulfillmentContext<'tcx> {
         &self.predicates
     }
 
+    fn is_duplicate_or_add(&mut self, tcx: &ty::ctxt<'tcx>,
+                           predicate: &ty::Predicate<'tcx>)
+                           -> bool {
+        // This is a kind of dirty hack to allow us to avoid "rederiving"
+        // things that we have already proven in other methods.
+        //
+        // The idea is that any predicate that doesn't involve type
+        // parameters and which only involves the 'static region (and
+        // no other regions) is universally solvable, since impls are global.
+        //
+        // This is particularly important since even if we have a
+        // cache hit in the selection context, we still wind up
+        // evaluating the 'nested obligations'.  This cache lets us
+        // skip those.
+
+        if self.errors_will_be_reported && predicate.is_global() {
+            tcx.fulfilled_predicates.borrow_mut().is_duplicate_or_add(predicate)
+        } else {
+            self.duplicate_set.is_duplicate_or_add(predicate)
+        }
+    }
+
     /// Attempts to select obligations using `selcx`. If `only_new_obligations` is true, then it
     /// only attempts to select obligations that haven't been seen before.
     fn select<'a>(&mut self,
@@ -442,3 +488,21 @@ fn register_region_obligation<'tcx>(tcx: &ty::ctxt<'tcx>,
         .push(region_obligation);
 
 }
+
+impl<'tcx> FulfilledPredicates<'tcx> {
+    pub fn new() -> FulfilledPredicates<'tcx> {
+        FulfilledPredicates {
+            set: HashSet::new()
+        }
+    }
+
+    pub fn is_duplicate(&self, p: &ty::Predicate<'tcx>) -> bool {
+        self.set.contains(p)
+    }
+
+    fn is_duplicate_or_add(&mut self, p: &ty::Predicate<'tcx>) -> bool {
+        !self.set.insert(p.clone())
+    }
+}
+
+

src/librustc/middle/traits/mod.rs

@@ -32,7 +32,7 @@ pub use self::error_reporting::suggest_new_overflow_limit;
 pub use self::coherence::orphan_check;
 pub use self::coherence::overlapping_impls;
 pub use self::coherence::OrphanCheckErr;
-pub use self::fulfill::{FulfillmentContext, RegionObligation};
+pub use self::fulfill::{FulfillmentContext, FulfilledPredicates, RegionObligation};
 pub use self::project::MismatchedProjectionTypes;
 pub use self::project::normalize;
 pub use self::project::Normalized;
@@ -315,7 +315,7 @@ pub fn evaluate_builtin_bound<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>,
            ty.repr(infcx.tcx),
            bound);
 
-    let mut fulfill_cx = FulfillmentContext::new();
+    let mut fulfill_cx = FulfillmentContext::new(false);
 
     // We can use a dummy node-id here because we won't pay any mind
     // to region obligations that arise (there shouldn't really be any
@@ -414,9 +414,27 @@ pub fn normalize_param_env_or_error<'a,'tcx>(unnormalized_env: ty::ParameterEnvi
     debug!("normalize_param_env_or_error(unnormalized_env={})",
            unnormalized_env.repr(tcx));
 
+    let predicates: Vec<_> =
+        util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.clone())
+        .filter(|p| !p.is_global()) // (*)
+        .collect();
+
+    // (*) Any predicate like `i32: Trait<u32>` or whatever doesn't
+    // need to be in the *environment* to be proven, so screen those
+    // out. This is important for the soundness of inter-fn
+    // caching. Note though that we should probably check that these
+    // predicates hold at the point where the environment is
+    // constructed, but I am not currently doing so out of laziness.
+    // -nmatsakis
+
+    debug!("normalize_param_env_or_error: elaborated-predicates={}",
+           predicates.repr(tcx));
+
+    let elaborated_env = unnormalized_env.with_caller_bounds(predicates);
+
     let infcx = infer::new_infer_ctxt(tcx);
-    let predicates = match fully_normalize(&infcx, &unnormalized_env, cause,
-                                           &unnormalized_env.caller_bounds) {
+    let predicates = match fully_normalize(&infcx, &elaborated_env, cause,
+                                           &elaborated_env.caller_bounds) {
         Ok(predicates) => predicates,
         Err(errors) => {
             report_fulfillment_errors(&infcx, &errors);
@@ -438,14 +456,11 @@ pub fn normalize_param_env_or_error<'a,'tcx>(unnormalized_env: ty::ParameterEnvi
             // all things considered.
             let err_msg = fixup_err_to_string(fixup_err);
             tcx.sess.span_err(span, &err_msg);
-            return unnormalized_env; // an unnormalized env is better than nothing
+            return elaborated_env; // an unnormalized env is better than nothing
         }
     };
 
-    debug!("normalize_param_env_or_error: predicates={}",
-           predicates.repr(tcx));
-
-    unnormalized_env.with_caller_bounds(predicates)
+    elaborated_env.with_caller_bounds(predicates)
 }
 
 pub fn fully_normalize<'a,'tcx,T>(infcx: &InferCtxt<'a,'tcx>,
@@ -460,7 +475,7 @@ pub fn fully_normalize<'a,'tcx,T>(infcx: &InferCtxt<'a,'tcx>,
     debug!("normalize_param_env(value={})", value.repr(tcx));
 
     let mut selcx = &mut SelectionContext::new(infcx, closure_typer);
-    let mut fulfill_cx = FulfillmentContext::new();
+    let mut fulfill_cx = FulfillmentContext::new(false);
     let Normalized { value: normalized_value, obligations } =
         project::normalize(selcx, cause, value);
     debug!("normalize_param_env: normalized_value={} obligations={}",

src/librustc/middle/traits/project.rs

@@ -535,7 +535,7 @@ fn assemble_candidates_from_param_env<'cx,'tcx>(
     obligation_trait_ref: &ty::TraitRef<'tcx>,
     candidate_set: &mut ProjectionTyCandidateSet<'tcx>)
 {
-    let env_predicates = selcx.param_env().caller_bounds.clone();
+    let env_predicates = selcx.param_env().caller_bounds.iter().cloned();
     assemble_candidates_from_predicates(selcx, obligation, obligation_trait_ref,
                                         candidate_set, env_predicates);
 }
@@ -571,22 +571,25 @@ fn assemble_candidates_from_trait_def<'cx,'tcx>(
     // If so, extract what we know from the trait and try to come up with a good answer.
     let trait_predicates = ty::lookup_predicates(selcx.tcx(), trait_ref.def_id);
     let bounds = trait_predicates.instantiate(selcx.tcx(), trait_ref.substs);
+    let bounds = elaborate_predicates(selcx.tcx(), bounds.predicates.into_vec());
     assemble_candidates_from_predicates(selcx, obligation, obligation_trait_ref,
-                                        candidate_set, bounds.predicates.into_vec());
+                                        candidate_set, bounds)
 }
 
-fn assemble_candidates_from_predicates<'cx,'tcx>(
+fn assemble_candidates_from_predicates<'cx,'tcx,I>(
     selcx: &mut SelectionContext<'cx,'tcx>,
     obligation: &ProjectionTyObligation<'tcx>,
     obligation_trait_ref: &ty::TraitRef<'tcx>,
     candidate_set: &mut ProjectionTyCandidateSet<'tcx>,
-    env_predicates: Vec<ty::Predicate<'tcx>>)
+    env_predicates: I)
+    where I: Iterator<Item=ty::Predicate<'tcx>>
 {
-    debug!("assemble_candidates_from_predicates(obligation={}, env_predicates={})",
-           obligation.repr(selcx.tcx()),
-           env_predicates.repr(selcx.tcx()));
+    debug!("assemble_candidates_from_predicates(obligation={})",
+           obligation.repr(selcx.tcx()));
     let infcx = selcx.infcx();
-    for predicate in elaborate_predicates(selcx.tcx(), env_predicates) {
+    for predicate in env_predicates {
+        debug!("assemble_candidates_from_predicates: predicate={}",
+               predicate.repr(selcx.tcx()));
         match predicate {
             ty::Predicate::Projection(ref data) => {
                 let same_name = data.item_name() == obligation.predicate.item_name;
@@ -641,6 +644,7 @@ fn assemble_candidates_from_object_type<'cx,'tcx>(
     let env_predicates = projection_bounds.iter()
                                           .map(|p| p.as_predicate())
                                           .collect();
+    let env_predicates = elaborate_predicates(selcx.tcx(), env_predicates);
     assemble_candidates_from_predicates(selcx, obligation, obligation_trait_ref,
                                         candidate_set, env_predicates)
 }

src/librustc/middle/traits/select.rs

@@ -435,6 +435,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         debug!("evaluate_predicate_recursively({})",
                obligation.repr(self.tcx()));
 
+        // Check the cache from the tcx of predicates that we know
+        // 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) {
+            return EvaluatedToOk;
+        }
+
         match obligation.predicate {
             ty::Predicate::Trait(ref t) => {
                 assert!(!t.has_escaping_regions());
@@ -1075,14 +1083,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         debug!("assemble_candidates_from_caller_bounds({})",
                stack.obligation.repr(self.tcx()));
 
-        let caller_trait_refs: Vec<_> =
-            self.param_env().caller_bounds.iter()
-            .filter_map(|o| o.to_opt_poly_trait_ref())
-            .collect();
-
         let all_bounds =
-            util::transitive_bounds(
-                self.tcx(), &caller_trait_refs[..]);
+            self.param_env().caller_bounds
+                            .iter()
+                            .filter_map(|o| o.to_opt_poly_trait_ref());
 
         let matching_bounds =
             all_bounds.filter(