From 0fcbc716fd56bcf4ecda5089e651be94c60efc3a Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 30 Jun 2020 13:23:42 +0200 Subject: [PATCH 1/4] Split namespace maps in `ItemScope` Reduces memory usage of the CrateDefMap query by ~130 MB on r-a. --- crates/ra_hir_def/src/item_scope.rs | 115 +++++++++++++++++++--------- 1 file changed, 80 insertions(+), 35 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index 4d446c7073..7fc53d86df 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -11,6 +11,7 @@ use crate::{ db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId, LocalModuleId, Lookup, MacroDefId, ModuleDefId, TraitId, }; +use std::collections::hash_map::Entry; #[derive(Copy, Clone)] pub(crate) enum ImportType { @@ -27,7 +28,11 @@ pub struct PerNsGlobImports { #[derive(Debug, Default, PartialEq, Eq)] pub struct ItemScope { - visible: FxHashMap, + types: FxHashMap, + values: FxHashMap, + macros: FxHashMap, + unresolved: FxHashSet, + defs: Vec, impls: Vec, /// Macros visible in current module in legacy textual scope @@ -65,14 +70,22 @@ pub(crate) enum BuiltinShadowMode { /// Other methods will only resolve values, types and module scoped macros only. impl ItemScope { pub fn entries<'a>(&'a self) -> impl Iterator + 'a { - //FIXME: shadowing - self.visible.iter().map(|(n, def)| (n, *def)) + // FIXME: shadowing + let keys: FxHashSet<_> = self + .types + .keys() + .chain(self.values.keys()) + .chain(self.macros.keys()) + .chain(self.unresolved.iter()) + .collect(); + + keys.into_iter().map(move |name| (name, self.get(name))) } pub fn entries_without_primitives<'a>( &'a self, ) -> impl Iterator + 'a { - self.visible.iter().map(|(n, def)| (n, *def)) + self.entries() } pub fn declarations(&self) -> impl Iterator + '_ { @@ -91,7 +104,7 @@ impl ItemScope { /// Iterate over all module scoped macros pub(crate) fn macros<'a>(&'a self) -> impl Iterator + 'a { - self.visible.iter().filter_map(|(name, def)| def.take_macros().map(|macro_| (name, macro_))) + self.entries().filter_map(|(name, def)| def.take_macros().map(|macro_| (name, macro_))) } /// Iterate over all legacy textual scoped macros visible at the end of the module @@ -101,12 +114,16 @@ impl ItemScope { /// Get a name from current module scope, legacy macros are not included pub(crate) fn get(&self, name: &Name) -> PerNs { - self.visible.get(name).copied().unwrap_or_else(PerNs::none) + PerNs { + types: self.types.get(name).copied(), + values: self.values.get(name).copied(), + macros: self.macros.get(name).copied(), + } } pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> { - for (name, per_ns) in &self.visible { - if let Some(vis) = item.match_with(*per_ns) { + for (name, per_ns) in self.entries() { + if let Some(vis) = item.match_with(per_ns) { return Some((name, vis)); } } @@ -114,8 +131,8 @@ impl ItemScope { } pub(crate) fn traits<'a>(&'a self) -> impl Iterator + 'a { - self.visible.values().filter_map(|def| match def.take_types() { - Some(ModuleDefId::TraitId(t)) => Some(t), + self.types.values().filter_map(|(def, _)| match def { + ModuleDefId::TraitId(t) => Some(*t), _ => None, }) } @@ -138,21 +155,39 @@ impl ItemScope { pub(crate) fn push_res(&mut self, name: Name, def: PerNs) -> bool { let mut changed = false; - let existing = self.visible.entry(name).or_default(); - if existing.types.is_none() && def.types.is_some() { - existing.types = def.types; - changed = true; + if let Some(types) = def.types { + match self.types.entry(name.clone()) { + Entry::Occupied(_) => {} + Entry::Vacant(e) => { + e.insert(types); + changed = true; + } + } + } + if let Some(values) = def.values { + match self.values.entry(name.clone()) { + Entry::Occupied(_) => {} + Entry::Vacant(e) => { + e.insert(values); + changed = true; + } + } + } + if let Some(macros) = def.macros { + match self.macros.entry(name.clone()) { + Entry::Occupied(_) => {} + Entry::Vacant(e) => { + e.insert(macros); + changed = true; + } + } } - if existing.values.is_none() && def.values.is_some() { - existing.values = def.values; - changed = true; - } - - if existing.macros.is_none() && def.macros.is_some() { - existing.macros = def.macros; - changed = true; + if def.is_none() { + if self.unresolved.insert(name) { + changed = true; + } } changed @@ -166,17 +201,17 @@ impl ItemScope { def_import_type: ImportType, ) -> bool { let mut changed = false; - let existing = self.visible.entry(lookup.1.clone()).or_default(); macro_rules! check_changed { ( $changed:ident, - ( $existing:ident / $def:ident ) . $field:ident, + ( $this:ident / $def:ident ) . $field:ident, $glob_imports:ident [ $lookup:ident ], $def_import_type:ident - ) => { - match ($existing.$field, $def.$field) { - (None, Some(_)) => { + ) => {{ + let existing = $this.$field.entry($lookup.1.clone()); + match (existing, $def.$field) { + (Entry::Vacant(entry), Some(_)) => { match $def_import_type { ImportType::Glob => { $glob_imports.$field.insert($lookup.clone()); @@ -186,32 +221,42 @@ impl ItemScope { } } - $existing.$field = $def.$field; + if let Some(fld) = $def.$field { + entry.insert(fld); + } $changed = true; } - (Some(_), Some(_)) + (Entry::Occupied(mut entry), Some(_)) if $glob_imports.$field.contains(&$lookup) && matches!($def_import_type, ImportType::Named) => { mark::hit!(import_shadowed); $glob_imports.$field.remove(&$lookup); - $existing.$field = $def.$field; + if let Some(fld) = $def.$field { + entry.insert(fld); + } $changed = true; } _ => {} } - }; + }}; } - check_changed!(changed, (existing / def).types, glob_imports[lookup], def_import_type); - check_changed!(changed, (existing / def).values, glob_imports[lookup], def_import_type); - check_changed!(changed, (existing / def).macros, glob_imports[lookup], def_import_type); + check_changed!(changed, (self / def).types, glob_imports[lookup], def_import_type); + check_changed!(changed, (self / def).values, glob_imports[lookup], def_import_type); + check_changed!(changed, (self / def).macros, glob_imports[lookup], def_import_type); + + if def.is_none() { + if self.unresolved.insert(lookup.1) { + changed = true; + } + } changed } pub(crate) fn resolutions<'a>(&'a self) -> impl Iterator + 'a { - self.visible.iter().map(|(name, res)| (name.clone(), *res)) + self.entries().map(|(name, res)| (name.clone(), res)) } pub(crate) fn collect_legacy_macros(&self) -> FxHashMap { From 56fb8a401ad38e766030755a19115251e66aafd6 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 30 Jun 2020 13:25:15 +0200 Subject: [PATCH 2/4] Reorder imports --- crates/ra_hir_def/src/item_scope.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index 7fc53d86df..f36477c03e 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -1,6 +1,8 @@ //! Describes items defined or visible (ie, imported) in a certain scope. //! This is shared between modules and blocks. +use std::collections::hash_map::Entry; + use hir_expand::name::Name; use once_cell::sync::Lazy; use ra_db::CrateId; @@ -11,7 +13,6 @@ use crate::{ db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId, LocalModuleId, Lookup, MacroDefId, ModuleDefId, TraitId, }; -use std::collections::hash_map::Entry; #[derive(Copy, Clone)] pub(crate) enum ImportType { From a80e8fea858ff789ed9fc490c2c1eb2b4eed6e82 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 30 Jun 2020 13:54:40 +0200 Subject: [PATCH 3/4] Simplify entry API usage --- crates/ra_hir_def/src/item_scope.rs | 33 +++++++++++------------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index f36477c03e..c8dde6f8da 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -158,31 +158,22 @@ impl ItemScope { let mut changed = false; if let Some(types) = def.types { - match self.types.entry(name.clone()) { - Entry::Occupied(_) => {} - Entry::Vacant(e) => { - e.insert(types); - changed = true; - } - } + self.types.entry(name.clone()).or_insert_with(|| { + changed = true; + types + }); } if let Some(values) = def.values { - match self.values.entry(name.clone()) { - Entry::Occupied(_) => {} - Entry::Vacant(e) => { - e.insert(values); - changed = true; - } - } + self.values.entry(name.clone()).or_insert_with(|| { + changed = true; + values + }); } if let Some(macros) = def.macros { - match self.macros.entry(name.clone()) { - Entry::Occupied(_) => {} - Entry::Vacant(e) => { - e.insert(macros); - changed = true; - } - } + self.macros.entry(name.clone()).or_insert_with(|| { + changed = true; + macros + }); } if def.is_none() { From 7c9b3d154c4d826d4623891c69a60d1a69031e5b Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 30 Jun 2020 14:06:23 +0200 Subject: [PATCH 4/4] Remove `entries_without_primitives` --- crates/ra_hir_def/src/item_scope.rs | 6 ------ crates/ra_hir_def/src/resolver.rs | 8 +++----- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index c8dde6f8da..beeb985595 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -83,12 +83,6 @@ impl ItemScope { keys.into_iter().map(move |name| (name, self.get(name))) } - pub fn entries_without_primitives<'a>( - &'a self, - ) -> impl Iterator + 'a { - self.entries() - } - pub fn declarations(&self) -> impl Iterator + '_ { self.defs.iter().copied() } diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index 15fdd9019b..0bf51eb7b8 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -511,11 +511,9 @@ impl Scope { }); } } - Scope::LocalItemsScope(body) => { - body.item_scope.entries_without_primitives().for_each(|(name, def)| { - f(name.clone(), ScopeDef::PerNs(def)); - }) - } + Scope::LocalItemsScope(body) => body.item_scope.entries().for_each(|(name, def)| { + f(name.clone(), ScopeDef::PerNs(def)); + }), Scope::GenericParams { params, def } => { for (local_id, param) in params.types.iter() { if let Some(name) = ¶m.name {