Merge #9206

9206: minor: Speed up fst items lookup during completions r=SomeoneToIgnore a=SomeoneToIgnore

Part of https://github.com/rust-analyzer/rust-analyzer/issues/7542

A number of profile calls added for `import_on_the_fly` contents.
Before:
<img width="606" alt="Screenshot 2021-06-11 at 00 19 13" src="https://user-images.githubusercontent.com/2690773/121598998-22321e80-ca4b-11eb-9a3d-dc9cb2936705.png">

After:
<img width="859" alt="Screenshot 2021-06-11 at 00 19 27" src="https://user-images.githubusercontent.com/2690773/121599022-2a8a5980-ca4b-11eb-82b6-13ab0ed56d58.png">

As a result, low hanging fruit was spotted: crazy amount of `fst_path` calls. Reducing that won ~200ms in the `import_on_the_fly @ sel` case in the `integrated_completion_benchmark`:

<img width="861" alt="Screenshot 2021-06-11 at 00 19 38" src="https://user-images.githubusercontent.com/2690773/121599277-7d641100-ca4b-11eb-8667-53206994de27.png">

I'm not sure how to proceed with the remaining `???` marks in such methods as `collect_import_map` though: there's nothing but library calls in cycles, but maybe I'll come up with something later.

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>

crates/base_db/src/lib.rs

@@ -120,6 +120,7 @@ impl<T: SourceDatabaseExt> FileLoader for FileLoaderDelegate<&'_ T> {
     }
 
     fn relevant_crates(&self, file_id: FileId) -> Arc<FxHashSet<CrateId>> {
+        let _p = profile::span("relevant_crates");
         let source_root = self.0.file_source_root(file_id);
         self.0.source_root_crates(source_root)
     }

crates/hir/src/lib.rs

@@ -191,6 +191,7 @@ impl Crate {
         db: &dyn DefDatabase,
         query: import_map::Query,
     ) -> impl Iterator<Item = Either<ModuleDef, MacroDef>> {
+        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<T>,
     ) -> Option<T> {
+        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<T>,
     ) -> Option<T> {
+        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<Item = Trait> + '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))

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};
@@ -69,8 +69,83 @@ pub struct ImportMap {
 impl ImportMap {
     pub fn import_map_query(db: &dyn DefDatabase, krate: CrateId) -> Arc<Self> {
         let _p = profile::span("import_map_query");
+
+        let mut import_map = collect_import_map(db, krate);
+
+        let mut importables = import_map.map.iter().collect::<Vec<_>>();
+        importables.sort_by_cached_key(|(_, import_info)| fst_path(&import_info.path));
+
+        // Build the FST, taking care not to insert duplicate values.
+
+        let mut builder = fst::MapBuilder::memory();
+        let mut last_batch_start = 0;
+
+        for idx in 0..importables.len() {
+            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;
+                }
+            }
+
+            builder.insert(key, last_batch_start as u64).unwrap();
+
+            last_batch_start = idx + 1;
+        }
+
+        import_map.fst = fst::Map::new(builder.into_inner().unwrap()).unwrap();
+        import_map.importables = importables.iter().map(|(item, _)| **item).collect();
+
+        Arc::new(import_map)
+    }
+
+    /// Returns the `ModPath` needed to import/mention `item`, relative to this crate's root.
+    pub fn path_of(&self, item: ItemInNs) -> Option<&ImportPath> {
+        self.import_info_for(item).map(|it| &it.path)
+    }
+
+    pub fn import_info_for(&self, item: ItemInNs) -> Option<&ImportInfo> {
+        self.map.get(&item)
+    }
+
+    fn collect_trait_assoc_items(
+        &mut self,
+        db: &dyn DefDatabase,
+        tr: TraitId,
+        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),
+                AssocItemId::ConstId(c) => ModuleDefId::from(*c),
+                // cannot use associated type aliases directly: need a `<Struct as Trait>::TypeAlias`
+                // qualifier, ergo no need to store it for imports in import_map
+                AssocItemId::TypeAliasId(_) => {
+                    cov_mark::hit!(type_aliases_ignored);
+                    continue;
+                }
+            };
+            let assoc_item = if is_type_in_ns {
+                ItemInNs::Types(module_def_id)
+            } else {
+                ItemInNs::Values(module_def_id)
+            };
+
+            let mut assoc_item_info = original_import_info.clone();
+            assoc_item_info.path.segments.push(assoc_item_name.to_owned());
+            assoc_item_info.is_trait_assoc_item = true;
+            self.map.insert(assoc_item, assoc_item_info);
+        }
+    }
+}
+
+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 = Self::default();
+    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![] };
@@ -141,73 +216,7 @@ impl ImportMap {
         }
     }
 
-        let mut importables = import_map.map.iter().collect::<Vec<_>>();
+    import_map
-
-        importables.sort_by(cmp);
-
-        // Build the FST, taking care not to insert duplicate values.
-
-        let mut builder = fst::MapBuilder::memory();
-        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 {
-                    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;
-        }
-
-        import_map.fst = fst::Map::new(builder.into_inner().unwrap()).unwrap();
-        import_map.importables = importables.iter().map(|(item, _)| **item).collect();
-
-        Arc::new(import_map)
-    }
-
-    /// Returns the `ModPath` needed to import/mention `item`, relative to this crate's root.
-    pub fn path_of(&self, item: ItemInNs) -> Option<&ImportPath> {
-        self.import_info_for(item).map(|it| &it.path)
-    }
-
-    pub fn import_info_for(&self, item: ItemInNs) -> Option<&ImportInfo> {
-        self.map.get(&item)
-    }
-
-    fn collect_trait_assoc_items(
-        &mut self,
-        db: &dyn DefDatabase,
-        tr: TraitId,
-        is_type_in_ns: bool,
-        original_import_info: &ImportInfo,
-    ) {
-        for (assoc_item_name, item) in &db.trait_data(tr).items {
-            let module_def_id = match item {
-                AssocItemId::FunctionId(f) => ModuleDefId::from(*f),
-                AssocItemId::ConstId(c) => ModuleDefId::from(*c),
-                // cannot use associated type aliases directly: need a `<Struct as Trait>::TypeAlias`
-                // qualifier, ergo no need to store it for imports in import_map
-                AssocItemId::TypeAliasId(_) => {
-                    cov_mark::hit!(type_aliases_ignored);
-                    continue;
-                }
-            };
-            let assoc_item = if is_type_in_ns {
-                ItemInNs::Types(module_def_id)
-            } else {
-                ItemInNs::Values(module_def_id)
-            };
-
-            let mut assoc_item_info = original_import_info.clone();
-            assoc_item_info.path.segments.push(assoc_item_name.to_owned());
-            assoc_item_info.is_trait_assoc_item = true;
-            self.map.insert(assoc_item, assoc_item_info);
-        }
-    }
 }
 
 impl PartialEq for ImportMap {
@@ -240,17 +249,12 @@ 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
 }
 
-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,
@@ -338,6 +342,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;

crates/hir_def/src/lang_item.rs

@@ -141,6 +141,7 @@ impl LangItems {
     ) where
         T: Into<AttrDefId> + 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));
         }

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<Item = ItemInNs> {
+        let _p = profile::span("PerNs::iter_items");
         self.types
             .map(|it| ItemInNs::Types(it.0))
             .into_iter()

crates/ide_db/src/symbol_index.rs

@@ -197,6 +197,7 @@ pub fn world_symbols(db: &RootDatabase, query: Query) -> Vec<FileSymbol> {
 }
 
 pub fn crate_symbols(db: &RootDatabase, krate: CrateId, query: Query) -> Vec<FileSymbol> {
+    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<FileSymbol> {
+        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);