From 667d75e546e318625f4d0a3c25cb04b78c14826e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 12 Jul 2023 09:55:39 +1000 Subject: [PATCH] Add a cache for `maybe_lint_level_root_bounded`. It's a nice speed win. --- compiler/rustc_mir_build/src/build/mod.rs | 10 +++++ compiler/rustc_mir_build/src/build/scope.rs | 47 ++++++++++++++++----- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/mod.rs b/compiler/rustc_mir_build/src/build/mod.rs index d828e71c7ac..a91ced4a211 100644 --- a/compiler/rustc_mir_build/src/build/mod.rs +++ b/compiler/rustc_mir_build/src/build/mod.rs @@ -10,6 +10,7 @@ use rustc_hir::def::DefKind; use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_hir::{GeneratorKind, Node}; +use rustc_index::bit_set::GrowableBitSet; use rustc_index::{Idx, IndexSlice, IndexVec}; use rustc_infer::infer::{InferCtxt, TyCtxtInferExt}; use rustc_middle::hir::place::PlaceBase as HirPlaceBase; @@ -215,6 +216,14 @@ struct Builder<'a, 'tcx> { unit_temp: Option>, var_debug_info: Vec>, + + // A cache for `maybe_lint_level_roots_bounded`. That function is called + // repeatedly, and each time it effectively traces a path through a tree + // structure from a node towards the root, doing an attribute check on each + // node along the way. This cache records which nodes trace all the way to + // the root (most of them do) and saves us from retracing many sub-paths + // many times, and rechecking many nodes. + lint_level_roots_cache: GrowableBitSet, } type CaptureMap<'tcx> = SortedIndexMultiMap>; @@ -725,6 +734,7 @@ fn new( var_indices: Default::default(), unit_temp: None, var_debug_info: vec![], + lint_level_roots_cache: GrowableBitSet::new_empty(), }; assert_eq!(builder.cfg.start_new_block(), START_BLOCK); diff --git a/compiler/rustc_mir_build/src/build/scope.rs b/compiler/rustc_mir_build/src/build/scope.rs index d7953d0c4af..a96288a11e5 100644 --- a/compiler/rustc_mir_build/src/build/scope.rs +++ b/compiler/rustc_mir_build/src/build/scope.rs @@ -769,12 +769,16 @@ pub(crate) fn maybe_new_source_scope( // the MIR->HIR mapping. (current_id, parent_id) } else { - // Use `maybe_lint_level_root_bounded` with `self.hir_id` as a bound - // to avoid adding Hir dependencies on our parents. - // We estimate the true lint roots here to avoid creating a lot of source scopes. + // Use `maybe_lint_level_root_bounded` to avoid adding Hir dependencies on our + // parents. We estimate the true lint roots here to avoid creating a lot of source + // scopes. ( - self.maybe_lint_level_root_bounded(current_id, self.hir_id), - self.maybe_lint_level_root_bounded(parent_id, self.hir_id), + self.maybe_lint_level_root_bounded(current_id), + if parent_id == self.hir_id { + parent_id // this is very common + } else { + self.maybe_lint_level_root_bounded(parent_id) + }, ) }; @@ -784,16 +788,24 @@ pub(crate) fn maybe_new_source_scope( } } - /// Walks upwards from `id` to find a node which might change lint levels with attributes. - /// It stops at `bound` and just returns it if reached. - fn maybe_lint_level_root_bounded(&self, mut id: HirId, bound: HirId) -> HirId { + /// Walks upwards from `orig_id` to find a node which might change lint levels with attributes. + /// It stops at `self.hir_id` and just returns it if reached. + fn maybe_lint_level_root_bounded(&mut self, orig_id: HirId) -> HirId { + // This assertion lets us just store `ItemLocalId` in the cache, rather + // than the full `HirId`. + assert_eq!(orig_id.owner, self.hir_id.owner); + + let mut id = orig_id; let hir = self.tcx.hir(); loop { - if id == bound { - return bound; + if id == self.hir_id { + // This is a moderately common case, mostly hit for previously unseen nodes. + break; } if hir.attrs(id).iter().any(|attr| Level::from_attr(attr).is_some()) { + // This is a rare case. It's for a node path that doesn't reach the root due to an + // intervening lint level attribute. This result doesn't get cached. return id; } @@ -802,7 +814,22 @@ fn maybe_lint_level_root_bounded(&self, mut id: HirId, bound: HirId) -> HirId { bug!("lint traversal reached the root of the crate"); } id = next; + + // This lookup is just an optimization; it can be removed without affecting + // functionality. It might seem strange to see this at the end of this loop, but the + // `orig_id` passed in to this function is almost always previously unseen, for which a + // lookup will be a miss. So we only do lookups for nodes up the parent chain, where + // cache lookups have a very high hit rate. + if self.lint_level_roots_cache.contains(id.local_id) { + break; + } } + + // `orig_id` traced to `self_id`; record this fact. If `orig_id` is a leaf node it will + // rarely (never?) subsequently be searched for, but it's hard to know if that is the case. + // The performance wins from the cache all come from caching non-leaf nodes. + self.lint_level_roots_cache.insert(orig_id.local_id); + self.hir_id } /// Creates a new source scope, nested in the current one.