From 3aaf07b8cb9a17669894db9f3e6afdb302676fdb Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 10 Jun 2021 23:03:16 +0300 Subject: [PATCH 1/2] Add more profiling for flyimports --- crates/base_db/src/lib.rs | 1 + crates/hir/src/lib.rs | 4 + crates/hir_def/src/import_map.rs | 153 ++++++++++++++++-------------- crates/hir_def/src/lang_item.rs | 1 + crates/hir_def/src/per_ns.rs | 2 + crates/ide_db/src/symbol_index.rs | 2 + 6 files changed, 92 insertions(+), 71 deletions(-) diff --git a/crates/base_db/src/lib.rs b/crates/base_db/src/lib.rs index 62bf2a4b2c1f..54baa3a63305 100644 --- a/crates/base_db/src/lib.rs +++ b/crates/base_db/src/lib.rs @@ -120,6 +120,7 @@ impl FileLoader for FileLoaderDelegate<&'_ T> { } fn relevant_crates(&self, file_id: FileId) -> Arc> { + let _p = profile::span("relevant_crates"); let source_root = self.0.file_source_root(file_id); self.0.source_root_crates(source_root) } diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index debc3ee624c0..b9c1dc44df08 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -191,6 +191,7 @@ impl Crate { db: &dyn DefDatabase, query: import_map::Query, ) -> impl Iterator> { + let _p = profile::span("query_external_importables"); import_map::search_dependencies(db, self.into(), query).into_iter().map(|item| match item { ItemInNs::Types(mod_id) | ItemInNs::Values(mod_id) => Either::Left(mod_id.into()), ItemInNs::Macros(mac_id) => Either::Right(mac_id.into()), @@ -2185,6 +2186,7 @@ impl Type { name: Option<&Name>, mut callback: impl FnMut(&Ty, Function) -> Option, ) -> Option { + let _p = profile::span("iterate_method_candidates"); // There should be no inference vars in types passed here // FIXME check that? // FIXME replace Unknown by bound vars here @@ -2218,6 +2220,7 @@ impl Type { name: Option<&Name>, mut callback: impl FnMut(&Ty, AssocItem) -> Option, ) -> Option { + let _p = profile::span("iterate_path_candidates"); let canonical = hir_ty::replace_errors_with_variables(&self.ty); let env = self.env.clone(); @@ -2255,6 +2258,7 @@ impl Type { &'a self, db: &'a dyn HirDatabase, ) -> impl Iterator + 'a { + let _p = profile::span("applicable_inherent_traits"); self.autoderef(db) .filter_map(|derefed_type| derefed_type.ty.dyn_trait()) .flat_map(move |dyn_trait_id| hir_ty::all_super_traits(db.upcast(), dyn_trait_id)) diff --git a/crates/hir_def/src/import_map.rs b/crates/hir_def/src/import_map.rs index 960cabb5fd48..2055edc95af0 100644 --- a/crates/hir_def/src/import_map.rs +++ b/crates/hir_def/src/import_map.rs @@ -69,80 +69,10 @@ pub struct ImportMap { impl ImportMap { pub fn import_map_query(db: &dyn DefDatabase, krate: CrateId) -> Arc { let _p = profile::span("import_map_query"); - let def_map = db.crate_def_map(krate); - let mut import_map = Self::default(); - - // We look only into modules that are public(ly reexported), starting with the crate root. - let empty = ImportPath { segments: vec![] }; - let root = def_map.module_id(def_map.root()); - let mut worklist = vec![(root, empty)]; - while let Some((module, mod_path)) = worklist.pop() { - let ext_def_map; - let mod_data = if module.krate == krate { - &def_map[module.local_id] - } else { - // The crate might reexport a module defined in another crate. - ext_def_map = module.def_map(db); - &ext_def_map[module.local_id] - }; - - let visible_items = mod_data.scope.entries().filter_map(|(name, per_ns)| { - let per_ns = per_ns.filter_visibility(|vis| vis == Visibility::Public); - if per_ns.is_none() { - None - } else { - Some((name, per_ns)) - } - }); - for (name, per_ns) in visible_items { - let mk_path = || { - let mut path = mod_path.clone(); - path.segments.push(name.clone()); - path - }; - - for item in per_ns.iter_items() { - let path = mk_path(); - let path_len = path.len(); - let import_info = - ImportInfo { path, container: module, is_trait_assoc_item: false }; - - if let Some(ModuleDefId::TraitId(tr)) = item.as_module_def_id() { - import_map.collect_trait_assoc_items( - db, - tr, - matches!(item, ItemInNs::Types(_)), - &import_info, - ); - } - - match import_map.map.entry(item) { - Entry::Vacant(entry) => { - entry.insert(import_info); - } - Entry::Occupied(mut entry) => { - // If the new path is shorter, prefer that one. - if path_len < entry.get().path.len() { - *entry.get_mut() = import_info; - } else { - continue; - } - } - } - - // If we've just added a path to a module, descend into it. We might traverse - // modules multiple times, but only if the new path to it is shorter than the - // first (else we `continue` above). - if let Some(ModuleDefId::ModuleId(mod_id)) = item.as_module_def_id() { - worklist.push((mod_id, mk_path())); - } - } - } - } + let mut import_map = collect_import_map(db, krate); let mut importables = import_map.map.iter().collect::>(); - importables.sort_by(cmp); // Build the FST, taking care not to insert duplicate values. @@ -185,6 +115,7 @@ impl ImportMap { is_type_in_ns: bool, original_import_info: &ImportInfo, ) { + let _p = profile::span("collect_trait_assoc_items"); for (assoc_item_name, item) in &db.trait_data(tr).items { let module_def_id = match item { AssocItemId::FunctionId(f) => ModuleDefId::from(*f), @@ -210,6 +141,84 @@ impl ImportMap { } } +fn collect_import_map(db: &dyn DefDatabase, krate: CrateId) -> ImportMap { + let _p = profile::span("collect_import_map"); + + let def_map = db.crate_def_map(krate); + let mut import_map = ImportMap::default(); + + // We look only into modules that are public(ly reexported), starting with the crate root. + let empty = ImportPath { segments: vec![] }; + let root = def_map.module_id(def_map.root()); + let mut worklist = vec![(root, empty)]; + while let Some((module, mod_path)) = worklist.pop() { + let ext_def_map; + let mod_data = if module.krate == krate { + &def_map[module.local_id] + } else { + // The crate might reexport a module defined in another crate. + ext_def_map = module.def_map(db); + &ext_def_map[module.local_id] + }; + + let visible_items = mod_data.scope.entries().filter_map(|(name, per_ns)| { + let per_ns = per_ns.filter_visibility(|vis| vis == Visibility::Public); + if per_ns.is_none() { + None + } else { + Some((name, per_ns)) + } + }); + + for (name, per_ns) in visible_items { + let mk_path = || { + let mut path = mod_path.clone(); + path.segments.push(name.clone()); + path + }; + + for item in per_ns.iter_items() { + let path = mk_path(); + let path_len = path.len(); + let import_info = + ImportInfo { path, container: module, is_trait_assoc_item: false }; + + if let Some(ModuleDefId::TraitId(tr)) = item.as_module_def_id() { + import_map.collect_trait_assoc_items( + db, + tr, + matches!(item, ItemInNs::Types(_)), + &import_info, + ); + } + + match import_map.map.entry(item) { + Entry::Vacant(entry) => { + entry.insert(import_info); + } + Entry::Occupied(mut entry) => { + // If the new path is shorter, prefer that one. + if path_len < entry.get().path.len() { + *entry.get_mut() = import_info; + } else { + continue; + } + } + } + + // If we've just added a path to a module, descend into it. We might traverse + // modules multiple times, but only if the new path to it is shorter than the + // first (else we `continue` above). + if let Some(ModuleDefId::ModuleId(mod_id)) = item.as_module_def_id() { + worklist.push((mod_id, mk_path())); + } + } + } + } + + import_map +} + impl PartialEq for ImportMap { fn eq(&self, other: &Self) -> bool { // `fst` and `importables` are built from `map`, so we don't need to compare them. @@ -240,6 +249,7 @@ impl fmt::Debug for ImportMap { } fn fst_path(path: &ImportPath) -> String { + let _p = profile::span("fst_path"); let mut s = path.to_string(); s.make_ascii_lowercase(); s @@ -338,6 +348,7 @@ impl Query { } fn import_matches(&self, import: &ImportInfo, enforce_lowercase: bool) -> bool { + let _p = profile::span("import_map::Query::import_matches"); if import.is_trait_assoc_item { if self.exclude_import_kinds.contains(&ImportKind::AssociatedItem) { return false; diff --git a/crates/hir_def/src/lang_item.rs b/crates/hir_def/src/lang_item.rs index 9e90f745ce28..3a45cbfa1217 100644 --- a/crates/hir_def/src/lang_item.rs +++ b/crates/hir_def/src/lang_item.rs @@ -141,6 +141,7 @@ impl LangItems { ) where T: Into + Copy, { + let _p = profile::span("collect_lang_item"); if let Some(lang_item_name) = lang_attr(db, item) { self.items.entry(lang_item_name).or_insert_with(|| constructor(item)); } diff --git a/crates/hir_def/src/per_ns.rs b/crates/hir_def/src/per_ns.rs index a594afce6686..a9f13cb82085 100644 --- a/crates/hir_def/src/per_ns.rs +++ b/crates/hir_def/src/per_ns.rs @@ -62,6 +62,7 @@ impl PerNs { } pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> PerNs { + let _p = profile::span("PerNs::filter_visibility"); PerNs { types: self.types.filter(|(_, v)| f(*v)), values: self.values.filter(|(_, v)| f(*v)), @@ -86,6 +87,7 @@ impl PerNs { } pub fn iter_items(self) -> impl Iterator { + let _p = profile::span("PerNs::iter_items"); self.types .map(|it| ItemInNs::Types(it.0)) .into_iter() diff --git a/crates/ide_db/src/symbol_index.rs b/crates/ide_db/src/symbol_index.rs index 5c372a7e5e37..000f87a8588c 100644 --- a/crates/ide_db/src/symbol_index.rs +++ b/crates/ide_db/src/symbol_index.rs @@ -197,6 +197,7 @@ pub fn world_symbols(db: &RootDatabase, query: Query) -> Vec { } pub fn crate_symbols(db: &RootDatabase, krate: CrateId, query: Query) -> Vec { + let _p = profile::span("crate_symbols").detail(|| format!("{:?}", query)); // FIXME(#4842): This now depends on CrateDefMap, why not build the entire symbol index from // that instead? @@ -321,6 +322,7 @@ impl SymbolIndex { impl Query { pub(crate) fn search(self, indices: &[&SymbolIndex]) -> Vec { + let _p = profile::span("symbol_index::Query::search"); let mut op = fst::map::OpBuilder::new(); for file_symbols in indices.iter() { let automaton = fst::automaton::Subsequence::new(&self.lowercased); From 690cd953273317ee4f3eaefd95bbd3538e929522 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 11 Jun 2021 00:10:09 +0300 Subject: [PATCH 2/2] Reduce fst_path calls --- crates/hir_def/src/import_map.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/crates/hir_def/src/import_map.rs b/crates/hir_def/src/import_map.rs index 2055edc95af0..404e3e15394d 100644 --- a/crates/hir_def/src/import_map.rs +++ b/crates/hir_def/src/import_map.rs @@ -1,6 +1,6 @@ //! A map of all publicly exported items in a crate. -use std::{cmp::Ordering, fmt, hash::BuildHasherDefault, sync::Arc}; +use std::{fmt, hash::BuildHasherDefault, sync::Arc}; use base_db::CrateId; use fst::{self, Streamer}; @@ -73,7 +73,7 @@ impl ImportMap { let mut import_map = collect_import_map(db, krate); let mut importables = import_map.map.iter().collect::>(); - importables.sort_by(cmp); + importables.sort_by_cached_key(|(_, import_info)| fst_path(&import_info.path)); // Build the FST, taking care not to insert duplicate values. @@ -81,13 +81,13 @@ impl ImportMap { let mut last_batch_start = 0; for idx in 0..importables.len() { - if let Some(next_item) = importables.get(idx + 1) { - if cmp(&importables[last_batch_start], next_item) == Ordering::Equal { + let key = fst_path(&importables[last_batch_start].1.path); + if let Some((_, next_import_info)) = importables.get(idx + 1) { + if key == fst_path(&next_import_info.path) { continue; } } - let key = fst_path(&importables[last_batch_start].1.path); builder.insert(key, last_batch_start as u64).unwrap(); last_batch_start = idx + 1; @@ -255,12 +255,6 @@ fn fst_path(path: &ImportPath) -> String { s } -fn cmp((_, lhs): &(&ItemInNs, &ImportInfo), (_, rhs): &(&ItemInNs, &ImportInfo)) -> Ordering { - let lhs_str = fst_path(&lhs.path); - let rhs_str = fst_path(&rhs.path); - lhs_str.cmp(&rhs_str) -} - #[derive(Debug, Eq, PartialEq, Hash)] pub enum ImportKind { Module,