From db1dfb2f36be40eb3ae5aff982f1f6ac34452883 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 4 Jun 2019 12:27:56 -0400 Subject: [PATCH 01/12] rename `evaluate_obligation_recursively` It does not, in fact, execute in a recursive context. --- src/librustc/traits/query/evaluate_obligation.rs | 2 +- src/librustc/traits/select.rs | 9 +++++---- src/librustc_traits/evaluate_obligation.rs | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/librustc/traits/query/evaluate_obligation.rs b/src/librustc/traits/query/evaluate_obligation.rs index d5230f15c2565..5f1af668a9e48 100644 --- a/src/librustc/traits/query/evaluate_obligation.rs +++ b/src/librustc/traits/query/evaluate_obligation.rs @@ -64,7 +64,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { Err(OverflowError) => { let mut selcx = SelectionContext::with_query_mode(&self, TraitQueryMode::Standard); - selcx.evaluate_obligation_recursively(obligation) + selcx.evaluate_root_obligation(obligation) .unwrap_or_else(|r| { span_bug!( obligation.cause.span, diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 7810d65e88cc1..503e0db548e4a 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -649,14 +649,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // where we do not expect overflow to be propagated. assert!(self.query_mode == TraitQueryMode::Standard); - self.evaluate_obligation_recursively(obligation) + self.evaluate_root_obligation(obligation) .expect("Overflow should be caught earlier in standard query mode") .may_apply() } - /// Evaluates whether the obligation `obligation` can be satisfied and returns - /// an `EvaluationResult`. - pub fn evaluate_obligation_recursively( + /// Evaluates whether the obligation `obligation` can be satisfied + /// and returns an `EvaluationResult`. This is meant for the + /// *initial* call. + pub fn evaluate_root_obligation( &mut self, obligation: &PredicateObligation<'tcx>, ) -> Result { diff --git a/src/librustc_traits/evaluate_obligation.rs b/src/librustc_traits/evaluate_obligation.rs index 83aebd16e2400..59b743abaf14b 100644 --- a/src/librustc_traits/evaluate_obligation.rs +++ b/src/librustc_traits/evaluate_obligation.rs @@ -29,7 +29,7 @@ fn evaluate_obligation<'tcx>( let mut selcx = SelectionContext::with_query_mode(&infcx, TraitQueryMode::Canonical); let obligation = Obligation::new(ObligationCause::dummy(), param_env, predicate); - selcx.evaluate_obligation_recursively(&obligation) + selcx.evaluate_root_obligation(&obligation) }, ) } From e64a7bf64b70a9630d1ba8741665158d91f4cca7 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 6 Jun 2019 13:32:00 -0400 Subject: [PATCH 02/12] introduce a stack depth --- src/librustc/traits/select.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 503e0db548e4a..d8ffc55ada5c8 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -185,6 +185,9 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { in_cycle: Cell, previous: TraitObligationStackList<'prev, 'tcx>, + + /// Number of parent frames plus one -- so the topmost frame has depth 1. + depth: usize, } #[derive(Clone, Default)] @@ -662,8 +665,10 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { obligation: &PredicateObligation<'tcx>, ) -> Result { self.evaluation_probe(|this| { - this.evaluate_predicate_recursively(TraitObligationStackList::empty(), - obligation.clone()) + this.evaluate_predicate_recursively( + TraitObligationStackList::empty(), + obligation.clone(), + ) }) } @@ -3743,6 +3748,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { fresh_trait_ref, in_cycle: Cell::new(false), previous: previous_stack, + depth: previous_stack.depth() + 1, } } @@ -3957,6 +3963,14 @@ impl<'o, 'tcx> TraitObligationStackList<'o, 'tcx> { fn head(&self) -> Option<&'o TraitObligationStack<'o, 'tcx>> { self.head } + + fn depth(&self) -> usize { + if let Some(head) = self.head { + head.depth + } else { + 0 + } + } } impl<'o, 'tcx> Iterator for TraitObligationStackList<'o, 'tcx> { From 5486a860f1d9cf5862beacf3ceace9e84e170f85 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 10 Jun 2019 11:46:53 -0400 Subject: [PATCH 03/12] propagate depth that was reached, not just whether there was a cycle I tried to propagate this information with the return value, but I found a curiosity (actually, something that I'm not keen on in general) -- in particular, the candidate cache urrently invokes evaluation, which may detect a cycle, but the "depth" that results from that are is easily propagated back out. This probably means that the candidate caching mechanism *itself* is sort of problematic, but I'm choosing to ignore that in favor of a more ambitious rewrite later. --- src/librustc/traits/select.rs | 61 ++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index d8ffc55ada5c8..7f9a7aaebf39c 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -154,12 +154,12 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { /// selection-context's freshener. Used to check for recursion. fresh_trait_ref: ty::PolyTraitRef<'tcx>, - /// Starts out as false -- if, during evaluation, we encounter a - /// cycle, then we will set this flag to true for all participants - /// in the cycle (apart from the "head" node). These participants - /// will then forego caching their results. This is not the most - /// efficient solution, but it addresses #60010. The problem we - /// are trying to prevent: + /// Starts out equal to `depth` -- if, during evaluation, we + /// encounter a cycle, then we will set this flag to the minimum + /// depth of that cycle for all participants in the cycle. These + /// participants will then forego caching their results. This is + /// not the most efficient solution, but it addresses #60010. The + /// problem we are trying to prevent: /// /// - If you have `A: AutoTrait` requires `B: AutoTrait` and `C: NonAutoTrait` /// - `B: AutoTrait` requires `A: AutoTrait` (coinductive cycle, ok) @@ -182,7 +182,7 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { /// evaluate each member of a cycle up to N times, where N is the /// length of the cycle. This means the performance impact is /// bounded and we shouldn't have any terrible worst-cases. - in_cycle: Cell, + reached_depth: Cell, previous: TraitObligationStackList<'prev, 'tcx>, @@ -877,14 +877,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); let result = result?; - if !stack.in_cycle.get() { + let reached_depth = stack.reached_depth.get(); + if reached_depth >= stack.depth { debug!("CACHE MISS: EVAL({:?})={:?}", fresh_trait_ref, result); self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result); } else { debug!( "evaluate_trait_predicate_recursively: skipping cache because {:?} \ - is a cycle participant", + is a cycle participant (at depth {}, reached depth {})", fresh_trait_ref, + stack.depth, + reached_depth, ); } @@ -986,10 +989,11 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // affect the inferencer state and (b) that if we see two // fresh regions with the same index, they refer to the same // unbound type variable. - if let Some(rec_index) = stack.iter() - .skip(1) // skip top-most frame - .position(|prev| stack.obligation.param_env == prev.obligation.param_env && - stack.fresh_trait_ref == prev.fresh_trait_ref) + if let Some(cycle_depth) = stack.iter() + .skip(1) // skip top-most frame + .find(|prev| stack.obligation.param_env == prev.obligation.param_env && + stack.fresh_trait_ref == prev.fresh_trait_ref) + .map(|stack| stack.depth) { debug!("evaluate_stack({:?}) --> recursive", stack.fresh_trait_ref); @@ -999,9 +1003,9 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // from the cycle head. We mark them as participating in a // cycle. This suppresses caching for those nodes. See // `in_cycle` field for more details. - for item in stack.iter().take(rec_index + 1) { + for item in stack.iter().take_while(|s| s.depth > cycle_depth) { debug!("evaluate_stack: marking {:?} as cycle participant", item.fresh_trait_ref); - item.in_cycle.set(true); + item.update_reached_depth(cycle_depth); } // Subtle: when checking for a coinductive cycle, we do @@ -1009,7 +1013,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // have erased regions) but rather the fully explicit // trait refs. This is important because it's only a cycle // if the regions match exactly. - let cycle = stack.iter().skip(1).take(rec_index + 1); + let cycle = stack.iter().skip(1).take_while(|s| s.depth >= cycle_depth); let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate)); if self.coinductive_match(cycle) { debug!( @@ -1228,6 +1232,11 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } // If no match, compute result and insert into cache. + // + // FIXME(nikomatsakis) -- this cache is not taking into + // account cycles that may have occurred in forming the + // candidate. I don't know of any specific problems that + // result but it seems awfully suspicious. let (candidate, dep_node) = self.in_task(|this| this.candidate_from_obligation_no_cache(stack)); @@ -3743,12 +3752,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { .to_poly_trait_ref() .fold_with(&mut self.freshener); + let depth = previous_stack.depth() + 1; TraitObligationStack { obligation, fresh_trait_ref, - in_cycle: Cell::new(false), + reached_depth: Cell::new(depth), previous: previous_stack, - depth: previous_stack.depth() + 1, + depth, } } @@ -3944,6 +3954,21 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { fn iter(&'o self) -> TraitObligationStackList<'o, 'tcx> { self.list() } + + /// Indicates that attempting to evaluate this stack entry + /// required accessing something from the stack at depth `reached_depth`. + fn update_reached_depth(&self, reached_depth: usize) { + assert!( + self.depth > reached_depth, + "invoked `update_reached_depth` with something under this stack: \ + self.depth={} reached_depth={}", + self.depth, + reached_depth, + ); + debug!("update_reached_depth(reached_depth={})", reached_depth); + let v = self.reached_depth.get(); + self.reached_depth.set(v.min(reached_depth)); + } } #[derive(Copy, Clone)] From 35f5ef65e2c5433adb2907e1c1badd9ecfd07c06 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 10 Jun 2019 15:35:37 -0400 Subject: [PATCH 04/12] execute cycle check before we consider caching --- src/librustc/traits/select.rs | 145 +++++++++++++++++++--------------- 1 file changed, 83 insertions(+), 62 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 7f9a7aaebf39c..8725c6bb71811 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -874,6 +874,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { return Ok(result); } + // Check if this is a match for something already on the + // stack. If so, we don't want to insert the result into the + // main cache (it is cycle dependent) nor the provisional + // cache (which is meant for things that have completed but + // for a "backedge" -- this result *is* the backedge). + if let Some(cycle_result) = self.check_evaluation_cycle(&stack) { + return Ok(cycle_result); + } + let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); let result = result?; @@ -894,6 +903,74 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { Ok(result) } + /// If there is any previous entry on the stack that precisely + /// matches this obligation, then we can assume that the + /// obligation is satisfied for now (still all other conditions + /// must be met of course). One obvious case this comes up is + /// marker traits like `Send`. Think of a linked list: + /// + /// struct List { data: T, next: Option>> } + /// + /// `Box>` will be `Send` if `T` is `Send` and + /// `Option>>` is `Send`, and in turn + /// `Option>>` is `Send` if `Box>` is + /// `Send`. + /// + /// Note that we do this comparison using the `fresh_trait_ref` + /// fields. Because these have all been freshened using + /// `self.freshener`, we can be sure that (a) this will not + /// affect the inferencer state and (b) that if we see two + /// fresh regions with the same index, they refer to the same + /// unbound type variable. + fn check_evaluation_cycle( + &mut self, + stack: &TraitObligationStack<'_, 'tcx>, + ) -> Option { + if let Some(cycle_depth) = stack.iter() + .skip(1) // skip top-most frame + .find(|prev| stack.obligation.param_env == prev.obligation.param_env && + stack.fresh_trait_ref == prev.fresh_trait_ref) + .map(|stack| stack.depth) + { + debug!( + "evaluate_stack({:?}) --> recursive at depth {}", + stack.fresh_trait_ref, + cycle_depth, + ); + + // If we have a stack like `A B C D E A`, where the top of + // the stack is the final `A`, then this will iterate over + // `A, E, D, C, B` -- i.e., all the participants apart + // from the cycle head. We mark them as participating in a + // cycle. This suppresses caching for those nodes. See + // `in_cycle` field for more details. + stack.update_reached_depth(cycle_depth); + + // Subtle: when checking for a coinductive cycle, we do + // not compare using the "freshened trait refs" (which + // have erased regions) but rather the fully explicit + // trait refs. This is important because it's only a cycle + // if the regions match exactly. + let cycle = stack.iter().skip(1).take_while(|s| s.depth >= cycle_depth); + let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate)); + if self.coinductive_match(cycle) { + debug!( + "evaluate_stack({:?}) --> recursive, coinductive", + stack.fresh_trait_ref + ); + Some(EvaluatedToOk) + } else { + debug!( + "evaluate_stack({:?}) --> recursive, inductive", + stack.fresh_trait_ref + ); + Some(EvaluatedToRecur) + } + } else { + None + } + } + fn evaluate_stack<'o>( &mut self, stack: &TraitObligationStack<'o, 'tcx>, @@ -970,66 +1047,6 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { return Ok(EvaluatedToUnknown); } - // If there is any previous entry on the stack that precisely - // matches this obligation, then we can assume that the - // obligation is satisfied for now (still all other conditions - // must be met of course). One obvious case this comes up is - // marker traits like `Send`. Think of a linked list: - // - // struct List { data: T, next: Option>> } - // - // `Box>` will be `Send` if `T` is `Send` and - // `Option>>` is `Send`, and in turn - // `Option>>` is `Send` if `Box>` is - // `Send`. - // - // Note that we do this comparison using the `fresh_trait_ref` - // fields. Because these have all been freshened using - // `self.freshener`, we can be sure that (a) this will not - // affect the inferencer state and (b) that if we see two - // fresh regions with the same index, they refer to the same - // unbound type variable. - if let Some(cycle_depth) = stack.iter() - .skip(1) // skip top-most frame - .find(|prev| stack.obligation.param_env == prev.obligation.param_env && - stack.fresh_trait_ref == prev.fresh_trait_ref) - .map(|stack| stack.depth) - { - debug!("evaluate_stack({:?}) --> recursive", stack.fresh_trait_ref); - - // If we have a stack like `A B C D E A`, where the top of - // the stack is the final `A`, then this will iterate over - // `A, E, D, C, B` -- i.e., all the participants apart - // from the cycle head. We mark them as participating in a - // cycle. This suppresses caching for those nodes. See - // `in_cycle` field for more details. - for item in stack.iter().take_while(|s| s.depth > cycle_depth) { - debug!("evaluate_stack: marking {:?} as cycle participant", item.fresh_trait_ref); - item.update_reached_depth(cycle_depth); - } - - // Subtle: when checking for a coinductive cycle, we do - // not compare using the "freshened trait refs" (which - // have erased regions) but rather the fully explicit - // trait refs. This is important because it's only a cycle - // if the regions match exactly. - let cycle = stack.iter().skip(1).take_while(|s| s.depth >= cycle_depth); - let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate)); - if self.coinductive_match(cycle) { - debug!( - "evaluate_stack({:?}) --> recursive, coinductive", - stack.fresh_trait_ref - ); - return Ok(EvaluatedToOk); - } else { - debug!( - "evaluate_stack({:?}) --> recursive, inductive", - stack.fresh_trait_ref - ); - return Ok(EvaluatedToRecur); - } - } - match self.candidate_from_obligation(stack) { Ok(Some(c)) => self.evaluate_candidate(stack, &c), Ok(None) => Ok(EvaluatedToAmbig), @@ -3966,8 +3983,12 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { reached_depth, ); debug!("update_reached_depth(reached_depth={})", reached_depth); - let v = self.reached_depth.get(); - self.reached_depth.set(v.min(reached_depth)); + let mut p = self; + while reached_depth < p.depth { + debug!("update_reached_depth: marking {:?} as cycle participant", p.fresh_trait_ref); + p.reached_depth.set(p.reached_depth.get().min(reached_depth)); + p = p.previous.head.unwrap(); + } } } From c86f94871459608479373b37fe38e4e522f28114 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 10 Jun 2019 15:36:38 -0400 Subject: [PATCH 05/12] introduce ProvisionalEvaluationCache --- src/librustc/traits/select.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 8725c6bb71811..1de248378bdf5 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -606,7 +606,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { debug!("select({:?})", obligation); debug_assert!(!obligation.predicate.has_escaping_bound_vars()); - let stack = self.push_stack(TraitObligationStackList::empty(), obligation); + let pec = &ProvisionalEvaluationCache::default(); + let stack = self.push_stack(TraitObligationStackList::empty(pec), obligation); let candidate = match self.candidate_from_obligation(&stack) { Err(SelectionError::Overflow) => { @@ -666,7 +667,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { ) -> Result { self.evaluation_probe(|this| { this.evaluate_predicate_recursively( - TraitObligationStackList::empty(), + TraitObligationStackList::empty(&ProvisionalEvaluationCache::default()), obligation.clone(), ) }) @@ -3968,6 +3969,10 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { TraitObligationStackList::with(self) } + fn cache(&self) -> &'o ProvisionalEvaluationCache<'tcx> { + self.previous.cache + } + fn iter(&'o self) -> TraitObligationStackList<'o, 'tcx> { self.list() } @@ -3992,18 +3997,24 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { } } +#[derive(Default)] +struct ProvisionalEvaluationCache<'tcx> { + _dummy: Vec<&'tcx ()>, +} + #[derive(Copy, Clone)] struct TraitObligationStackList<'o, 'tcx: 'o> { + cache: &'o ProvisionalEvaluationCache<'tcx>, head: Option<&'o TraitObligationStack<'o, 'tcx>>, } impl<'o, 'tcx> TraitObligationStackList<'o, 'tcx> { - fn empty() -> TraitObligationStackList<'o, 'tcx> { - TraitObligationStackList { head: None } + fn empty(cache: &'o ProvisionalEvaluationCache<'tcx>) -> TraitObligationStackList<'o, 'tcx> { + TraitObligationStackList { cache, head: None } } fn with(r: &'o TraitObligationStack<'o, 'tcx>) -> TraitObligationStackList<'o, 'tcx> { - TraitObligationStackList { head: Some(r) } + TraitObligationStackList { cache: r.cache(), head: Some(r) } } fn head(&self) -> Option<&'o TraitObligationStack<'o, 'tcx>> { From a1a8a7b9869b4b2627b88f293ad92160ed826688 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 10 Jun 2019 16:22:10 -0400 Subject: [PATCH 06/12] add in a depth-first number for stack entries --- src/librustc/traits/select.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 1de248378bdf5..31ef31808c473 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -188,6 +188,11 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { /// Number of parent frames plus one -- so the topmost frame has depth 1. depth: usize, + + /// Depth-first number of this node in the search graph -- a + /// pre-order index. Basically a freshly incremented counter. + #[allow(dead_code)] // TODO + dfn: usize, } #[derive(Clone, Default)] @@ -3770,12 +3775,14 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { .to_poly_trait_ref() .fold_with(&mut self.freshener); + let dfn = previous_stack.cache.next_dfn(); let depth = previous_stack.depth() + 1; TraitObligationStack { obligation, fresh_trait_ref, reached_depth: Cell::new(depth), previous: previous_stack, + dfn, depth, } } @@ -3999,9 +4006,18 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { #[derive(Default)] struct ProvisionalEvaluationCache<'tcx> { + dfn: Cell, _dummy: Vec<&'tcx ()>, } +impl<'tcx> ProvisionalEvaluationCache<'tcx> { + fn next_dfn(&self) -> usize { + let result = self.dfn.get(); + self.dfn.set(result + 1); + result + } +} + #[derive(Copy, Clone)] struct TraitObligationStackList<'o, 'tcx: 'o> { cache: &'o ProvisionalEvaluationCache<'tcx>, From 9639d8ec34ac5bd7920951087c168decc799723c Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 11 Jun 2019 19:05:08 -0400 Subject: [PATCH 07/12] add the "provisional cache" --- src/librustc/traits/select.rs | 196 +++++++++++++++++++++++++++++++++- 1 file changed, 192 insertions(+), 4 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 31ef31808c473..4411aa6cb9401 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -43,7 +43,7 @@ use crate::hir; use rustc_data_structures::bit_set::GrowableBitSet; use rustc_data_structures::sync::Lock; use rustc_target::spec::abi::Abi; -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::cmp; use std::fmt::{self, Display}; use std::iter; @@ -191,7 +191,6 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { /// Depth-first number of this node in the search graph -- a /// pre-order index. Basically a freshly incremented counter. - #[allow(dead_code)] // TODO dfn: usize, } @@ -880,6 +879,12 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { return Ok(result); } + if let Some(result) = stack.cache().get_provisional(fresh_trait_ref) { + debug!("PROVISIONAL CACHE HIT: EVAL({:?})={:?}", fresh_trait_ref, result); + stack.update_reached_depth(stack.cache().current_reached_depth()); + return Ok(result); + } + // Check if this is a match for something already on the // stack. If so, we don't want to insert the result into the // main cache (it is cycle dependent) nor the provisional @@ -892,20 +897,42 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); let result = result?; + if !result.must_apply_modulo_regions() { + stack.cache().on_failure(stack.dfn); + } + let reached_depth = stack.reached_depth.get(); if reached_depth >= stack.depth { debug!("CACHE MISS: EVAL({:?})={:?}", fresh_trait_ref, result); self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result); + + stack.cache().on_completion(stack.depth, |fresh_trait_ref, provisional_result| { + self.insert_evaluation_cache( + obligation.param_env, + fresh_trait_ref, + dep_node, + provisional_result.max(result), + ); + }); } else { + debug!("PROVISIONAL: {:?}={:?}", fresh_trait_ref, result); debug!( - "evaluate_trait_predicate_recursively: skipping cache because {:?} \ + "evaluate_trait_predicate_recursively: caching provisionally because {:?} \ is a cycle participant (at depth {}, reached depth {})", fresh_trait_ref, stack.depth, reached_depth, ); + + stack.cache().insert_provisional( + stack.dfn, + reached_depth, + fresh_trait_ref, + result, + ); } + Ok(result) } @@ -4004,18 +4031,179 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { } } +/// The "provisional evaluation cache" is used to store intermediate cache results +/// when solving auto traits. Auto traits are unusual in that they can support +/// cycles. So, for example, a "proof tree" like this would be ok: +/// +/// - `Foo: Send` :- +/// - `Bar: Send` :- +/// - `Foo: Send` -- cycle, but ok +/// - `Baz: Send` +/// +/// Here, to prove `Foo: Send`, we have to prove `Bar: Send` and +/// `Baz: Send`. Proving `Bar: Send` in turn required `Foo: Send`. +/// For non-auto traits, this cycle would be an error, but for auto traits (because +/// they are coinductive) it is considered ok. +/// +/// However, there is a complication: at the point where we have +/// "proven" `Bar: Send`, we have in fact only proven it +/// *provisionally*. In particular, we proved that `Bar: Send` +/// *under the assumption* that `Foo: Send`. But what if we later +/// find out this assumption is wrong? Specifically, we could +/// encounter some kind of error proving `Baz: Send`. In that case, +/// `Bar: Send` didn't turn out to be true. +/// +/// In Issue #60010, we found a bug in rustc where it would cache +/// these intermediate results. This was fixed in #60444 by disabling +/// *all* caching for things involved in a cycle -- in our example, +/// that would mean we don't cache that `Bar: Send`. But this led +/// to large slowdowns. +/// +/// Specifically, imagine this scenario, where proving `Baz: Send` +/// first requires proving `Bar: Send` (which is true: +/// +/// - `Foo: Send` :- +/// - `Bar: Send` :- +/// - `Foo: Send` -- cycle, but ok +/// - `Baz: Send` +/// - `Bar: Send` -- would be nice for this to be a cache hit! +/// - `*const T: Send` -- but what if we later encounter an error? +/// +/// The *provisional evaluation cache* resolves this issue. It stores +/// cache results that we've proven but which were involved in a cycle +/// in some way. We track the minimal stack depth (i.e., the +/// farthest from the top of the stack) that we are dependent on. +/// The idea is that the cache results within are all valid -- so long as +/// none of the nodes in between the current node and the node at that minimum +/// depth result in an error (in which case the cached results are just thrown away). +/// +/// During evaluation, we consult this provisional cache and rely on +/// it. Accessing a cached value is considered equivalent to accessing +/// a result at `reached_depth`, so it marks the *current* solution as +/// provisional as well. If an error is encountered, we toss out any +/// provisional results added from the subtree that encountered the +/// error. When we pop the node at `reached_depth` from the stack, we +/// can commit all the things that remain in the provisional cache. #[derive(Default)] struct ProvisionalEvaluationCache<'tcx> { + /// next "depth first number" to issue -- just a counter dfn: Cell, - _dummy: Vec<&'tcx ()>, + + /// Stores the "coldest" depth (bottom of stack) reached by any of + /// the evaluation entries. The idea here is that all things in the provisional + /// cache are always dependent on *something* that is colder in the stack: + /// therefore, if we add a new entry that is dependent on something *colder still*, + /// we have to modify the depth for all entries at once. + /// + /// Example: + /// + /// Imagine we have a stack `A B C D E` (with `E` being the top of + /// the stack). We cache something with depth 2, which means that + /// it was dependent on C. Then we pop E but go on and process a + /// new node F: A B C D F. Now F adds something to the cache with + /// depth 1, meaning it is dependent on B. Our original cache + /// entry is also dependent on B, because there is a path from E + /// to C and then from C to F and from F to B. + reached_depth: Cell, + + /// Map from cache key to the provisionally evaluated thing. + /// The cache entries contain the result but also the DFN in which they + /// were added. The DFN is used to clear out values on failure. + /// + /// Imagine we have a stack like: + /// + /// - `A B C` and we add a cache for the result of C (DFN 2) + /// - Then we have a stack `A B D` where `D` has DFN 3 + /// - We try to solve D by evaluating E: `A B D E` (DFN 4) + /// - `E` generates various cache entries which have cyclic dependices on `B` + /// - `A B D E F` and so forth + /// - the DFN of `F` for example would be 5 + /// - then we determine that `E` is in error -- we will then clear + /// all cache values whose DFN is >= 4 -- in this case, that + /// means the cached value for `F`. + map: RefCell, ProvisionalEvaluation>>, +} + +/// A cache value for the provisional cache: contains the depth-first +/// number (DFN) and result. +#[derive(Copy, Clone)] +struct ProvisionalEvaluation { + from_dfn: usize, + result: EvaluationResult, } impl<'tcx> ProvisionalEvaluationCache<'tcx> { + /// Get the next DFN in sequence (basically a counter). fn next_dfn(&self) -> usize { let result = self.dfn.get(); self.dfn.set(result + 1); result } + + /// Check the provisional cache for any result for + /// `fresh_trait_ref`. If there is a hit, then you must consider + /// it an access to the stack slots at depth + /// `self.current_reached_depth()` and above. + fn get_provisional(&self, fresh_trait_ref: ty::PolyTraitRef<'tcx>) -> Option { + Some(self.map.borrow().get(&fresh_trait_ref)?.result) + } + + /// Current value of the `reached_depth` counter -- all the + /// provisional cache entries are dependent on the item at this + /// depth. + fn current_reached_depth(&self) -> usize { + self.reached_depth.get() + } + + /// Insert a provisional result into the cache. The result came + /// from the node with the given DFN. It accessed a minimum depth + /// of `reached_depth` to compute. It evaluated `fresh_trait_ref` + /// and resulted in `result`. + fn insert_provisional( + &self, + from_dfn: usize, + reached_depth: usize, + fresh_trait_ref: ty::PolyTraitRef<'tcx>, + result: EvaluationResult, + ) { + let r_d = self.reached_depth.get(); + self.reached_depth.set(r_d.min(reached_depth)); + + self.map.borrow_mut().insert(fresh_trait_ref, ProvisionalEvaluation { from_dfn, result }); + } + + /// Invoked when the node with dfn `dfn` does not get a successful + /// result. This will clear out any provisional cache entries + /// that were added since `dfn` was created. This is because the + /// provisional entries are things which must assume that the + /// things on the stack at the time of their creation succeeded -- + /// since the failing node is presently at the top of the stack, + /// these provisional entries must either depend on it or some + /// ancestor of it. + fn on_failure(&self, dfn: usize) { + self.map.borrow_mut().retain(|_key, eval| eval.from_dfn >= dfn) + } + + /// Invoked when the node at depth `depth` completed without + /// depending on anything higher in the stack (if that completion + /// was a failure, then `on_failure` should have been invoked + /// already). The callback `op` will be invoked for each + /// provisional entry that we can now confirm. + fn on_completion( + &self, + depth: usize, + mut op: impl FnMut(ty::PolyTraitRef<'tcx>, EvaluationResult), + ) { + if self.reached_depth.get() < depth { + return; + } + + for (fresh_trait_ref, eval) in self.map.borrow_mut().drain() { + op(fresh_trait_ref, eval.result); + } + + self.reached_depth.set(depth); + } } #[derive(Copy, Clone)] From 6fdcc8281a93c3aa032df245ca5d0342adcb01b1 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 11 Jun 2019 19:09:15 -0400 Subject: [PATCH 08/12] remove hacks that are no longer needed --- src/libsyntax/tokenstream.rs | 11 ----------- src/libsyntax_pos/lib.rs | 8 -------- 2 files changed, 19 deletions(-) diff --git a/src/libsyntax/tokenstream.rs b/src/libsyntax/tokenstream.rs index 2daec9702798f..b32049b1da8c3 100644 --- a/src/libsyntax/tokenstream.rs +++ b/src/libsyntax/tokenstream.rs @@ -59,17 +59,6 @@ where TokenStream: Send + Sync, {} -// These are safe since we ensure that they hold for all fields in the `_dummy` function. -// -// These impls are only here because the compiler takes forever to compute the Send and Sync -// bounds without them. -// FIXME: Remove these impls when the compiler can compute the bounds quickly again. -// See https://github.com/rust-lang/rust/issues/60846 -#[cfg(parallel_compiler)] -unsafe impl Send for TokenTree {} -#[cfg(parallel_compiler)] -unsafe impl Sync for TokenTree {} - impl TokenTree { /// Use this token tree as a matcher to parse given tts. pub fn parse(cx: &base::ExtCtxt<'_>, mtch: &[quoted::TokenTree], tts: TokenStream) diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index 8f5595968a738..d81cb756b0296 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -227,14 +227,6 @@ impl SpanData { } } -// The interner is pointed to by a thread local value which is only set on the main thread -// with parallelization is disabled. So we don't allow `Span` to transfer between threads -// to avoid panics and other errors, even though it would be memory safe to do so. -#[cfg(not(parallel_compiler))] -impl !Send for Span {} -#[cfg(not(parallel_compiler))] -impl !Sync for Span {} - impl PartialOrd for Span { fn partial_cmp(&self, rhs: &Self) -> Option { PartialOrd::partial_cmp(&self.data(), &rhs.data()) From 59f5045a1696018411f10807057e3168aa8a6ee5 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 12 Jun 2019 10:42:21 -0400 Subject: [PATCH 09/12] add more debug logs --- src/librustc/traits/select.rs | 42 +++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 4411aa6cb9401..5a59a13a37c52 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -4126,7 +4126,7 @@ struct ProvisionalEvaluationCache<'tcx> { /// A cache value for the provisional cache: contains the depth-first /// number (DFN) and result. -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] struct ProvisionalEvaluation { from_dfn: usize, result: EvaluationResult, @@ -4145,6 +4145,11 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// it an access to the stack slots at depth /// `self.current_reached_depth()` and above. fn get_provisional(&self, fresh_trait_ref: ty::PolyTraitRef<'tcx>) -> Option { + debug!( + "get_provisional(fresh_trait_ref={:?}) = {:#?}", + fresh_trait_ref, + self.map.borrow().get(&fresh_trait_ref), + ); Some(self.map.borrow().get(&fresh_trait_ref)?.result) } @@ -4166,9 +4171,18 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { fresh_trait_ref: ty::PolyTraitRef<'tcx>, result: EvaluationResult, ) { + debug!( + "insert_provisional(from_dfn={}, reached_depth={}, fresh_trait_ref={:?}, result={:?})", + from_dfn, + reached_depth, + fresh_trait_ref, + result, + ); let r_d = self.reached_depth.get(); self.reached_depth.set(r_d.min(reached_depth)); + debug!("insert_provisional: reached_depth={:?}", self.reached_depth.get()); + self.map.borrow_mut().insert(fresh_trait_ref, ProvisionalEvaluation { from_dfn, result }); } @@ -4181,7 +4195,18 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// these provisional entries must either depend on it or some /// ancestor of it. fn on_failure(&self, dfn: usize) { - self.map.borrow_mut().retain(|_key, eval| eval.from_dfn >= dfn) + debug!( + "on_failure(dfn={:?})", + dfn, + ); + self.map.borrow_mut().retain(|key, eval| { + if !eval.from_dfn >= dfn { + debug!("on_failure: removing {:?}", key); + false + } else { + true + } + }); } /// Invoked when the node at depth `depth` completed without @@ -4194,11 +4219,24 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { depth: usize, mut op: impl FnMut(ty::PolyTraitRef<'tcx>, EvaluationResult), ) { + debug!( + "on_completion(depth={}, reached_depth={})", + depth, + self.reached_depth.get(), + ); + if self.reached_depth.get() < depth { + debug!("on_completion: did not yet reach depth to complete"); return; } for (fresh_trait_ref, eval) in self.map.borrow_mut().drain() { + debug!( + "on_completion: fresh_trait_ref={:?} eval={:?}", + fresh_trait_ref, + eval, + ); + op(fresh_trait_ref, eval.result); } From cb8158d84737e2d36d3e785be494dc49756d6735 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 12 Jun 2019 17:55:10 -0400 Subject: [PATCH 10/12] correctly set the `reached_depth` field --- src/librustc/traits/select.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 5a59a13a37c52..be7765c024626 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -4084,7 +4084,6 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { /// provisional results added from the subtree that encountered the /// error. When we pop the node at `reached_depth` from the stack, we /// can commit all the things that remain in the provisional cache. -#[derive(Default)] struct ProvisionalEvaluationCache<'tcx> { /// next "depth first number" to issue -- just a counter dfn: Cell, @@ -4132,6 +4131,16 @@ struct ProvisionalEvaluation { result: EvaluationResult, } +impl<'tcx> Default for ProvisionalEvaluationCache<'tcx> { + fn default() -> Self { + Self { + dfn: Cell::new(0), + reached_depth: Cell::new(std::usize::MAX), + map: Default::default(), + } + } +} + impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// Get the next DFN in sequence (basically a counter). fn next_dfn(&self) -> usize { @@ -4146,9 +4155,10 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// `self.current_reached_depth()` and above. fn get_provisional(&self, fresh_trait_ref: ty::PolyTraitRef<'tcx>) -> Option { debug!( - "get_provisional(fresh_trait_ref={:?}) = {:#?}", + "get_provisional(fresh_trait_ref={:?}) = {:#?} with reached-depth {}", fresh_trait_ref, self.map.borrow().get(&fresh_trait_ref), + self.reached_depth.get(), ); Some(self.map.borrow().get(&fresh_trait_ref)?.result) } @@ -4240,7 +4250,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { op(fresh_trait_ref, eval.result); } - self.reached_depth.set(depth); + self.reached_depth.set(std::usize::MAX); } } From bb97fe0bae0d4dcfd106e77a951ea8dbb4be2659 Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Thu, 13 Jun 2019 12:05:18 +0200 Subject: [PATCH 11/12] revert change incorrectly identified as "hack" put back negative impls to ensure `Span` is not `Send` nor `Sync` --- src/libsyntax_pos/lib.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index d81cb756b0296..8f5595968a738 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -227,6 +227,14 @@ impl SpanData { } } +// The interner is pointed to by a thread local value which is only set on the main thread +// with parallelization is disabled. So we don't allow `Span` to transfer between threads +// to avoid panics and other errors, even though it would be memory safe to do so. +#[cfg(not(parallel_compiler))] +impl !Send for Span {} +#[cfg(not(parallel_compiler))] +impl !Sync for Span {} + impl PartialOrd for Span { fn partial_cmp(&self, rhs: &Self) -> Option { PartialOrd::partial_cmp(&self.data(), &rhs.data()) From 0baa9258dd2f901a24d744705f514fa678e64940 Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Fri, 14 Jun 2019 12:19:26 +0200 Subject: [PATCH 12/12] put back the workarounds for #60846 based on https://github.com/rust-lang/rust/pull/61754#issuecomment-501743750 I am adding `bootstrap` to the cfg-preconditions for the two manual `unsafe impls`'s of `Send` and `Sync` for `TokenTree`. --- src/libsyntax/tokenstream.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libsyntax/tokenstream.rs b/src/libsyntax/tokenstream.rs index b32049b1da8c3..cd906bb282b7e 100644 --- a/src/libsyntax/tokenstream.rs +++ b/src/libsyntax/tokenstream.rs @@ -59,6 +59,17 @@ where TokenStream: Send + Sync, {} +// These are safe since we ensure that they hold for all fields in the `_dummy` function. +// +// These impls are only here because the compiler takes forever to compute the Send and Sync +// bounds without them. +// FIXME: Remove these impls when the compiler can compute the bounds quickly again. +// See https://github.com/rust-lang/rust/issues/60846 +#[cfg(all(bootstrap, parallel_compiler))] +unsafe impl Send for TokenTree {} +#[cfg(all(bootstrap, parallel_compiler))] +unsafe impl Sync for TokenTree {} + impl TokenTree { /// Use this token tree as a matcher to parse given tts. pub fn parse(cx: &base::ExtCtxt<'_>, mtch: &[quoted::TokenTree], tts: TokenStream)