From 32741d5d1645a41acd16addc9612b1253e101458 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 2 Jun 2022 17:51:39 +1000 Subject: [PATCH] Split `process_obligation` in two. Because it really has two halves: - A read-only part that checks if further work is needed. - The further work part, which is much less hot. This makes things a bit clearer and nicer. --- .../src/obligation_forest/mod.rs | 15 ++-- .../src/obligation_forest/tests.rs | 4 ++ .../src/traits/fulfill.rs | 71 ++++++++----------- 3 files changed, 44 insertions(+), 46 deletions(-) diff --git a/compiler/rustc_data_structures/src/obligation_forest/mod.rs b/compiler/rustc_data_structures/src/obligation_forest/mod.rs index 60f8f37b226ec..07a96dd7dbbf1 100644 --- a/compiler/rustc_data_structures/src/obligation_forest/mod.rs +++ b/compiler/rustc_data_structures/src/obligation_forest/mod.rs @@ -96,6 +96,8 @@ pub trait ObligationProcessor { type Obligation: ForestObligation; type Error: Debug; + fn needs_process_obligation(&self, obligation: &Self::Obligation) -> bool; + fn process_obligation( &mut self, obligation: &mut Self::Obligation, @@ -143,7 +145,7 @@ pub struct ObligationForest { /// A cache of the nodes in `nodes`, indexed by predicate. Unfortunately, /// its contents are not guaranteed to match those of `nodes`. See the - /// comments in [`Self::process_obligation` for details. + /// comments in `Self::process_obligation` for details. active_cache: FxHashMap, /// A vector reused in [Self::compress()] and [Self::find_cycles_from_node()], @@ -417,15 +419,18 @@ impl ObligationForest { // nodes. Therefore we use a `while` loop. let mut index = 0; while let Some(node) = self.nodes.get_mut(index) { + if node.state.get() != NodeState::Pending + || !processor.needs_process_obligation(&node.obligation) + { + index += 1; + continue; + } + // `processor.process_obligation` can modify the predicate within // `node.obligation`, and that predicate is the key used for // `self.active_cache`. This means that `self.active_cache` can get // out of sync with `nodes`. It's not very common, but it does // happen, and code in `compress` has to allow for it. - if node.state.get() != NodeState::Pending { - index += 1; - continue; - } match processor.process_obligation(&mut node.obligation) { ProcessResult::Unchanged => { diff --git a/compiler/rustc_data_structures/src/obligation_forest/tests.rs b/compiler/rustc_data_structures/src/obligation_forest/tests.rs index 5ecc75736a310..e2991aae1742c 100644 --- a/compiler/rustc_data_structures/src/obligation_forest/tests.rs +++ b/compiler/rustc_data_structures/src/obligation_forest/tests.rs @@ -65,6 +65,10 @@ where type Obligation = O; type Error = E; + fn needs_process_obligation(&self, _obligation: &Self::Obligation) -> bool { + true + } + fn process_obligation( &mut self, obligation: &mut Self::Obligation, diff --git a/compiler/rustc_trait_selection/src/traits/fulfill.rs b/compiler/rustc_trait_selection/src/traits/fulfill.rs index 3ec0d320f8562..50b8baecbcae9 100644 --- a/compiler/rustc_trait_selection/src/traits/fulfill.rs +++ b/compiler/rustc_trait_selection/src/traits/fulfill.rs @@ -253,22 +253,16 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> { type Obligation = PendingPredicateObligation<'tcx>; type Error = FulfillmentErrorCode<'tcx>; - /// Processes a predicate obligation and returns either: - /// - `Changed(v)` if the predicate is true, presuming that `v` are also true - /// - `Unchanged` if we don't have enough info to be sure - /// - `Error(e)` if the predicate does not hold + /// Identifies whether a predicate obligation needs processing. /// /// This is always inlined, despite its size, because it has a single /// callsite and it is called *very* frequently. #[inline(always)] - fn process_obligation( - &mut self, - pending_obligation: &mut Self::Obligation, - ) -> ProcessResult { + fn needs_process_obligation(&self, pending_obligation: &Self::Obligation) -> bool { // If we were stalled on some unresolved variables, first check whether // any of them have been resolved; if not, don't bother doing more work // yet. - let change = match pending_obligation.stalled_on.len() { + match pending_obligation.stalled_on.len() { // Match arms are in order of frequency, which matters because this // code is so hot. 1 and 0 dominate; 2+ is fairly rare. 1 => { @@ -291,42 +285,18 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> { false })() } - }; - - if !change { - debug!( - "process_predicate: pending obligation {:?} still stalled on {:?}", - self.selcx.infcx().resolve_vars_if_possible(pending_obligation.obligation.clone()), - pending_obligation.stalled_on - ); - return ProcessResult::Unchanged; } - - self.process_changed_obligations(pending_obligation) } - fn process_backedge<'c, I>( - &mut self, - cycle: I, - _marker: PhantomData<&'c PendingPredicateObligation<'tcx>>, - ) where - I: Clone + Iterator>, - { - if self.selcx.coinductive_match(cycle.clone().map(|s| s.obligation.predicate)) { - debug!("process_child_obligations: coinductive match"); - } else { - let cycle: Vec<_> = cycle.map(|c| c.obligation.clone()).collect(); - self.selcx.infcx().report_overflow_error_cycle(&cycle); - } - } -} - -impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { - // The code calling this method is extremely hot and only rarely - // actually uses this, so move this part of the code - // out of that loop. + /// Processes a predicate obligation and returns either: + /// - `Changed(v)` if the predicate is true, presuming that `v` are also true + /// - `Unchanged` if we don't have enough info to be sure + /// - `Error(e)` if the predicate does not hold + /// + /// This is called much less often than `needs_process_obligation`, so we + /// never inline it. #[inline(never)] - fn process_changed_obligations( + fn process_obligation( &mut self, pending_obligation: &mut PendingPredicateObligation<'tcx>, ) -> ProcessResult, FulfillmentErrorCode<'tcx>> { @@ -341,6 +311,8 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { self.selcx.infcx().resolve_vars_if_possible(obligation.predicate); } + let obligation = &pending_obligation.obligation; + debug!(?obligation, ?obligation.cause, "process_obligation"); let infcx = self.selcx.infcx(); @@ -655,6 +627,23 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { } } + fn process_backedge<'c, I>( + &mut self, + cycle: I, + _marker: PhantomData<&'c PendingPredicateObligation<'tcx>>, + ) where + I: Clone + Iterator>, + { + if self.selcx.coinductive_match(cycle.clone().map(|s| s.obligation.predicate)) { + debug!("process_child_obligations: coinductive match"); + } else { + let cycle: Vec<_> = cycle.map(|c| c.obligation.clone()).collect(); + self.selcx.infcx().report_overflow_error_cycle(&cycle); + } + } +} + +impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { #[instrument(level = "debug", skip(self, obligation, stalled_on))] fn process_trait_obligation( &mut self,