From 3291218f476ab5e7d4603212cff91f8435046046 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Jul 2021 10:56:26 -0500 Subject: [PATCH] Improve caching during trait evaluation Previously, we would 'forget' that we had `'static` regions in some place during trait evaluation. This lead to us producing `EvaluatedToOkModuloRegions` when we could have produced `EvaluatedToOk`, causing us to perform unnecessary work. This PR preserves `'static` regions when we canonicalize a predicate for `evaluate_obligation`, and when we 'freshen' a predicate during trait evaluation. Thie ensures that evaluating a predicate containing `'static` regions can produce `EvaluatedToOk` (assuming that we don't end up introducing any region dependencies during evaluation). Building off of this improved caching, we use `predicate_must_hold_considering_regions` during fulfillment of projection predicates to see if we can skip performing additional work. We already do this for trait predicates, but doing this for projection predicates lead to mixed performance results without the above caching improvements. --- .../src/infer/canonical/canonicalizer.rs | 19 +++++-------------- compiler/rustc_infer/src/infer/freshen.rs | 14 +++++++++++--- compiler/rustc_infer/src/infer/mod.rs | 7 ++++++- .../src/traits/fulfill.rs | 14 ++++++++++++++ .../src/traits/query/evaluate_obligation.rs | 6 ++++-- .../src/traits/query/normalize.rs | 4 ++-- .../src/traits/query/type_op/mod.rs | 5 +++-- .../src/traits/select/mod.rs | 8 ++++---- 8 files changed, 49 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs b/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs index ac953f4305c..448dd662348 100644 --- a/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs +++ b/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs @@ -102,20 +102,11 @@ impl<'cx, 'tcx> InferCtxt<'cx, 'tcx> { ) } - /// A hacky variant of `canonicalize_query` that does not - /// canonicalize `'static`. Unfortunately, the existing leak - /// check treats `'static` differently in some cases (see also - /// #33684), so if we are performing an operation that may need to - /// prove "leak-check" related things, we leave `'static` - /// alone. - /// - /// `'static` is also special cased when winnowing candidates when - /// selecting implementation candidates, so we also have to leave `'static` - /// alone for queries that do selection. - // - // FIXME(#48536): once the above issues are resolved, we can remove this - // and just use `canonicalize_query`. - pub fn canonicalize_hr_query_hack( + /// A variant of `canonicalize_query` that does not + /// canonicalize `'static`. This is useful when + /// the query implementation can perform more efficient + /// handling of `'static` regions (e.g. trait evaluation). + pub fn canonicalize_query_keep_static( &self, value: V, query_state: &mut OriginalQueryValues<'tcx>, diff --git a/compiler/rustc_infer/src/infer/freshen.rs b/compiler/rustc_infer/src/infer/freshen.rs index b3d7876c6e8..4af1bdf97a7 100644 --- a/compiler/rustc_infer/src/infer/freshen.rs +++ b/compiler/rustc_infer/src/infer/freshen.rs @@ -47,16 +47,18 @@ pub struct TypeFreshener<'a, 'tcx> { const_freshen_count: u32, ty_freshen_map: FxHashMap>, const_freshen_map: FxHashMap, &'tcx ty::Const<'tcx>>, + keep_static: bool, } impl<'a, 'tcx> TypeFreshener<'a, 'tcx> { - pub fn new(infcx: &'a InferCtxt<'a, 'tcx>) -> TypeFreshener<'a, 'tcx> { + pub fn new(infcx: &'a InferCtxt<'a, 'tcx>, keep_static: bool) -> TypeFreshener<'a, 'tcx> { TypeFreshener { infcx, ty_freshen_count: 0, const_freshen_count: 0, ty_freshen_map: Default::default(), const_freshen_map: Default::default(), + keep_static, } } @@ -124,8 +126,7 @@ impl<'a, 'tcx> TypeFolder<'tcx> for TypeFreshener<'a, 'tcx> { r } - ty::ReStatic - | ty::ReEarlyBound(..) + ty::ReEarlyBound(..) | ty::ReFree(_) | ty::ReVar(_) | ty::RePlaceholder(..) @@ -134,6 +135,13 @@ impl<'a, 'tcx> TypeFolder<'tcx> for TypeFreshener<'a, 'tcx> { // replace all free regions with 'erased self.tcx().lifetimes.re_erased } + ty::ReStatic => { + if self.keep_static { + r + } else { + self.tcx().lifetimes.re_erased + } + } } } diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index f39431f2494..d3bfb2b2e44 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -646,7 +646,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { } pub fn freshener<'b>(&'b self) -> TypeFreshener<'b, 'tcx> { - freshen::TypeFreshener::new(self) + freshen::TypeFreshener::new(self, false) + } + + /// Like `freshener`, but does not replace `'static` regions. + pub fn freshener_keep_static<'b>(&'b self) -> TypeFreshener<'b, 'tcx> { + freshen::TypeFreshener::new(self, true) } pub fn type_is_unconstrained_numeric(&'a self, ty: Ty<'_>) -> UnconstrainedNumeric { diff --git a/compiler/rustc_trait_selection/src/traits/fulfill.rs b/compiler/rustc_trait_selection/src/traits/fulfill.rs index dfe2909498d..9ec1dd5c2ee 100644 --- a/compiler/rustc_trait_selection/src/traits/fulfill.rs +++ b/compiler/rustc_trait_selection/src/traits/fulfill.rs @@ -365,6 +365,7 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { let project_obligation = obligation.with(binder.rebind(data)); self.process_projection_obligation( + obligation, project_obligation, &mut pending_obligation.stalled_on, ) @@ -419,6 +420,7 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { let project_obligation = obligation.with(Binder::dummy(*data)); self.process_projection_obligation( + obligation, project_obligation, &mut pending_obligation.stalled_on, ) @@ -666,10 +668,22 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { fn process_projection_obligation( &mut self, + obligation: &PredicateObligation<'tcx>, project_obligation: PolyProjectionObligation<'tcx>, stalled_on: &mut Vec>, ) -> ProcessResult, FulfillmentErrorCode<'tcx>> { let tcx = self.selcx.tcx(); + + if obligation.predicate.is_global() { + // no type variables present, can use evaluation for better caching. + // FIXME: consider caching errors too. + if self.selcx.infcx().predicate_must_hold_considering_regions(obligation) { + return ProcessResult::Changed(vec![]); + } else { + tracing::debug!("Does NOT hold: {:?}", obligation); + } + } + match project::poly_project_and_unify_type(self.selcx, &project_obligation) { Ok(Ok(Some(os))) => ProcessResult::Changed(mk_pending(os)), Ok(Ok(None)) => { diff --git a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs index b83a4cd1e57..2dc48e47efc 100644 --- a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs +++ b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs @@ -64,8 +64,10 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> { obligation: &PredicateObligation<'tcx>, ) -> Result { let mut _orig_values = OriginalQueryValues::default(); - let c_pred = self - .canonicalize_query(obligation.param_env.and(obligation.predicate), &mut _orig_values); + let c_pred = self.canonicalize_query_keep_static( + obligation.param_env.and(obligation.predicate), + &mut _orig_values, + ); // Run canonical query. If overflow occurs, rerun from scratch but this time // in standard trait query mode so that overflow is handled appropriately // within `SelectionContext`. diff --git a/compiler/rustc_trait_selection/src/traits/query/normalize.rs b/compiler/rustc_trait_selection/src/traits/query/normalize.rs index d65a378b1ed..3f6efa03b3a 100644 --- a/compiler/rustc_trait_selection/src/traits/query/normalize.rs +++ b/compiler/rustc_trait_selection/src/traits/query/normalize.rs @@ -180,7 +180,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> { // so we cannot canonicalize it. let c_data = self .infcx - .canonicalize_hr_query_hack(self.param_env.and(data), &mut orig_values); + .canonicalize_query_keep_static(self.param_env.and(data), &mut orig_values); debug!("QueryNormalizer: c_data = {:#?}", c_data); debug!("QueryNormalizer: orig_values = {:#?}", orig_values); match tcx.normalize_projection_ty(c_data) { @@ -249,7 +249,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> { // so we cannot canonicalize it. let c_data = self .infcx - .canonicalize_hr_query_hack(self.param_env.and(data), &mut orig_values); + .canonicalize_query_keep_static(self.param_env.and(data), &mut orig_values); debug!("QueryNormalizer: c_data = {:#?}", c_data); debug!("QueryNormalizer: orig_values = {:#?}", orig_values); let normalized_ty = match tcx.normalize_projection_ty(c_data) { diff --git a/compiler/rustc_trait_selection/src/traits/query/type_op/mod.rs b/compiler/rustc_trait_selection/src/traits/query/type_op/mod.rs index 130ffa1a33a..fbff86618ad 100644 --- a/compiler/rustc_trait_selection/src/traits/query/type_op/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/query/type_op/mod.rs @@ -77,12 +77,13 @@ pub trait QueryTypeOp<'tcx>: fmt::Debug + Sized + TypeFoldable<'tcx> + 'tcx { } // FIXME(#33684) -- We need to use - // `canonicalize_hr_query_hack` here because of things + // `canonicalize_query_keep_static` here because of things // like the subtype query, which go awry around // `'static` otherwise. let mut canonical_var_values = OriginalQueryValues::default(); let old_param_env = query_key.param_env; - let canonical_self = infcx.canonicalize_hr_query_hack(query_key, &mut canonical_var_values); + let canonical_self = + infcx.canonicalize_query_keep_static(query_key, &mut canonical_var_values); let canonical_result = Self::perform_query(infcx.tcx, canonical_self)?; let InferOk { value, obligations } = infcx diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 1bdc8b34cf6..95611ebc818 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -216,7 +216,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn new(infcx: &'cx InferCtxt<'cx, 'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { infcx, - freshener: infcx.freshener(), + freshener: infcx.freshener_keep_static(), intercrate: false, intercrate_ambiguity_causes: None, allow_negative_impls: false, @@ -227,7 +227,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { infcx, - freshener: infcx.freshener(), + freshener: infcx.freshener_keep_static(), intercrate: true, intercrate_ambiguity_causes: None, allow_negative_impls: false, @@ -242,7 +242,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!(?allow_negative_impls, "with_negative"); SelectionContext { infcx, - freshener: infcx.freshener(), + freshener: infcx.freshener_keep_static(), intercrate: false, intercrate_ambiguity_causes: None, allow_negative_impls, @@ -257,7 +257,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!(?query_mode, "with_query_mode"); SelectionContext { infcx, - freshener: infcx.freshener(), + freshener: infcx.freshener_keep_static(), intercrate: false, intercrate_ambiguity_causes: None, allow_negative_impls: false,