From c603b9043fd530c11d3001eb62b1315c5aa1afe0 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 27 Nov 2021 20:50:07 +0300 Subject: [PATCH 1/3] feat: make hightlighting linear In https://youtu.be/qvIZZf5dmTE, we've noticed that AstIdMap does a linear lookup when going from SyntaxNode to Id. This leads to accidentally quadratic overall performance. Replace linear lookup with a O(1) hashmap lookup. Future work: don't duplicate `SyntaxNodePtr` in `AstIdMap` and switch to "call site dependency injection" style storage (eg, store a `HashSet`). See the explanation of the work here on YouTube https://youtu.be/wvEgymUm7cY :-) --- crates/hir_expand/src/ast_id_map.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/hir_expand/src/ast_id_map.rs b/crates/hir_expand/src/ast_id_map.rs index 16cf299076..64387b8148 100644 --- a/crates/hir_expand/src/ast_id_map.rs +++ b/crates/hir_expand/src/ast_id_map.rs @@ -14,6 +14,7 @@ use std::{ use la_arena::{Arena, Idx}; use profile::Count; +use rustc_hash::FxHashMap; use syntax::{ast, match_ast, AstNode, AstPtr, SyntaxNode, SyntaxNodePtr}; /// `AstId` points to an AST node in a specific file. @@ -63,6 +64,7 @@ type ErasedFileAstId = Idx; #[derive(Debug, PartialEq, Eq, Default)] pub struct AstIdMap { arena: Arena, + map: FxHashMap, _c: Count, } @@ -89,6 +91,7 @@ impl AstIdMap { } } }); + res.map.extend(res.arena.iter().map(|(idx, ptr)| (ptr.clone(), idx))); res } @@ -96,16 +99,16 @@ impl AstIdMap { let raw = self.erased_ast_id(item.syntax()); FileAstId { raw, _ty: PhantomData } } + fn erased_ast_id(&self, item: &SyntaxNode) -> ErasedFileAstId { let ptr = SyntaxNodePtr::new(item); - match self.arena.iter().find(|(_id, i)| **i == ptr) { - Some((it, _)) => it, - None => panic!( + *self.map.get(&ptr).unwrap_or_else(|| { + panic!( "Can't find {:?} in AstIdMap:\n{:?}", item, self.arena.iter().map(|(_id, i)| i).collect::>(), - ), - } + ) + }) } pub fn get(&self, id: FileAstId) -> AstPtr { From 4f3fc6fa1a0b76d120626dcf1e2e0a31cc10a54c Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 27 Nov 2021 21:13:07 +0300 Subject: [PATCH 2/3] try to optimize things unsuccessfully Baseline ``` Database loaded: 598.40ms, 304minstr, 118mb (metadata 390.57ms, 21minstr, 841kb; build 111.31ms, 8764kinstr, -214kb) crates: 39, mods: 824, decls: 18647, fns: 13910 Item Collection: 9.70s, 75ginstr, 377mb exprs: 382426, ??ty: 387 (0%), ?ty: 285 (0%), !ty: 145 Inference: 43.16s, 342ginstr, 641mb Total: 52.86s, 417ginstr, 1018mb ``` Eager ``` Database loaded: 625.86ms, 304minstr, 118mb (metadata 414.52ms, 21minstr, 841kb; build 113.81ms, 8764kinstr, -230kb) crates: 39, mods: 824, decls: 18647, fns: 13910 Item Collection: 10.09s, 75ginstr, 389mb exprs: 382426, ??ty: 387 (0%), ?ty: 285 (0%), !ty: 145 Inference: 43.27s, 341ginstr, 644mb Total: 53.37s, 417ginstr, 1034mb ``` Lazy ``` Database loaded: 626.34ms, 304minstr, 118mb (metadata 416.26ms, 21minstr, 841kb; build 113.67ms, 8750kinstr, -209kb) crates: 39, mods: 824, decls: 18647, fns: 13910 Item Collection: 10.16s, 75ginstr, 389mb exprs: 382426, ??ty: 387 (0%), ?ty: 285 (0%), !ty: 145 Inference: 44.51s, 342ginstr, 644mb Total: 54.67s, 417ginstr, 1034mb ``` --- Cargo.lock | 1 + crates/hir_expand/Cargo.toml | 1 + crates/hir_expand/src/ast_id_map.rs | 26 ++++++++++++++++++++++---- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a0d809a1b..4d22ebdc18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -529,6 +529,7 @@ dependencies = [ "la-arena", "limit", "mbe", + "once_cell", "profile", "rustc-hash", "syntax", diff --git a/crates/hir_expand/Cargo.toml b/crates/hir_expand/Cargo.toml index 380b7ef877..7a61036530 100644 --- a/crates/hir_expand/Cargo.toml +++ b/crates/hir_expand/Cargo.toml @@ -16,6 +16,7 @@ either = "1.5.3" rustc-hash = "1.0.0" la-arena = { version = "0.3.0", path = "../../lib/arena" } itertools = "0.10.0" +once_cell = "1" base_db = { path = "../base_db", version = "0.0.0" } cfg = { path = "../cfg", version = "0.0.0" } diff --git a/crates/hir_expand/src/ast_id_map.rs b/crates/hir_expand/src/ast_id_map.rs index 64387b8148..9db2bc6410 100644 --- a/crates/hir_expand/src/ast_id_map.rs +++ b/crates/hir_expand/src/ast_id_map.rs @@ -61,13 +61,29 @@ impl FileAstId { type ErasedFileAstId = Idx; /// Maps items' `SyntaxNode`s to `ErasedFileAstId`s and back. -#[derive(Debug, PartialEq, Eq, Default)] +#[derive(Default)] pub struct AstIdMap { arena: Arena, - map: FxHashMap, + /// Reversed mapping lazily derived from [`self.arena`]. + /// + /// FIXE: Do not store `SyntaxNodePtr` twice. + map: once_cell::sync::OnceCell>, _c: Count, } +impl fmt::Debug for AstIdMap { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("AstIdMap").field("arena", &self.arena).finish() + } +} + +impl PartialEq for AstIdMap { + fn eq(&self, other: &Self) -> bool { + self.arena == other.arena + } +} +impl Eq for AstIdMap {} + impl AstIdMap { pub(crate) fn from_source(node: &SyntaxNode) -> AstIdMap { assert!(node.parent().is_none()); @@ -91,9 +107,11 @@ impl AstIdMap { } } }); - res.map.extend(res.arena.iter().map(|(idx, ptr)| (ptr.clone(), idx))); res } + fn map(&self) -> &FxHashMap { + self.map.get_or_init(|| self.arena.iter().map(|(idx, ptr)| (ptr.clone(), idx)).collect()) + } pub fn ast_id(&self, item: &N) -> FileAstId { let raw = self.erased_ast_id(item.syntax()); @@ -102,7 +120,7 @@ impl AstIdMap { fn erased_ast_id(&self, item: &SyntaxNode) -> ErasedFileAstId { let ptr = SyntaxNodePtr::new(item); - *self.map.get(&ptr).unwrap_or_else(|| { + *self.map().get(&ptr).unwrap_or_else(|| { panic!( "Can't find {:?} in AstIdMap:\n{:?}", item, From 278e7c33115a448eb7bbe6420404331c1820f1af Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 5 Dec 2021 17:19:48 +0300 Subject: [PATCH 3/3] more frugal map --- Cargo.lock | 2 +- crates/hir_expand/Cargo.toml | 2 +- crates/hir_expand/src/ast_id_map.rs | 41 +++++++++++++++++++---------- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4d22ebdc18..173bbb801d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -525,11 +525,11 @@ dependencies = [ "cfg", "cov-mark", "either", + "hashbrown", "itertools", "la-arena", "limit", "mbe", - "once_cell", "profile", "rustc-hash", "syntax", diff --git a/crates/hir_expand/Cargo.toml b/crates/hir_expand/Cargo.toml index 7a61036530..039861f7f7 100644 --- a/crates/hir_expand/Cargo.toml +++ b/crates/hir_expand/Cargo.toml @@ -16,7 +16,7 @@ either = "1.5.3" rustc-hash = "1.0.0" la-arena = { version = "0.3.0", path = "../../lib/arena" } itertools = "0.10.0" -once_cell = "1" +hashbrown = { version = "0.11", features = ["inline-more"], default-features = false } base_db = { path = "../base_db", version = "0.0.0" } cfg = { path = "../cfg", version = "0.0.0" } diff --git a/crates/hir_expand/src/ast_id_map.rs b/crates/hir_expand/src/ast_id_map.rs index 9db2bc6410..43c8a3db5c 100644 --- a/crates/hir_expand/src/ast_id_map.rs +++ b/crates/hir_expand/src/ast_id_map.rs @@ -8,13 +8,13 @@ use std::{ any::type_name, fmt, - hash::{Hash, Hasher}, + hash::{BuildHasher, BuildHasherDefault, Hash, Hasher}, marker::PhantomData, }; use la_arena::{Arena, Idx}; use profile::Count; -use rustc_hash::FxHashMap; +use rustc_hash::FxHasher; use syntax::{ast, match_ast, AstNode, AstPtr, SyntaxNode, SyntaxNodePtr}; /// `AstId` points to an AST node in a specific file. @@ -63,11 +63,10 @@ type ErasedFileAstId = Idx; /// Maps items' `SyntaxNode`s to `ErasedFileAstId`s and back. #[derive(Default)] pub struct AstIdMap { + /// Maps stable id to unstable ptr. arena: Arena, - /// Reversed mapping lazily derived from [`self.arena`]. - /// - /// FIXE: Do not store `SyntaxNodePtr` twice. - map: once_cell::sync::OnceCell>, + /// Reverse: map ptr to id. + map: hashbrown::HashMap, (), ()>, _c: Count, } @@ -107,26 +106,34 @@ impl AstIdMap { } } }); + res.map = hashbrown::HashMap::with_capacity_and_hasher(res.arena.len(), ()); + for (idx, ptr) in res.arena.iter() { + let hash = hash_ptr(ptr); + match res.map.raw_entry_mut().from_hash(hash, |idx2| *idx2 == idx) { + hashbrown::hash_map::RawEntryMut::Occupied(_) => unreachable!(), + hashbrown::hash_map::RawEntryMut::Vacant(entry) => { + entry.insert_with_hasher(hash, idx, (), |&idx| hash_ptr(&res.arena[idx])); + } + } + } res } - fn map(&self) -> &FxHashMap { - self.map.get_or_init(|| self.arena.iter().map(|(idx, ptr)| (ptr.clone(), idx)).collect()) - } pub fn ast_id(&self, item: &N) -> FileAstId { let raw = self.erased_ast_id(item.syntax()); FileAstId { raw, _ty: PhantomData } } - fn erased_ast_id(&self, item: &SyntaxNode) -> ErasedFileAstId { let ptr = SyntaxNodePtr::new(item); - *self.map().get(&ptr).unwrap_or_else(|| { - panic!( + let hash = hash_ptr(&ptr); + match self.map.raw_entry().from_hash(hash, |&idx| self.arena[idx] == ptr) { + Some((&idx, &())) => idx, + None => panic!( "Can't find {:?} in AstIdMap:\n{:?}", item, self.arena.iter().map(|(_id, i)| i).collect::>(), - ) - }) + ), + } } pub fn get(&self, id: FileAstId) -> AstPtr { @@ -138,6 +145,12 @@ impl AstIdMap { } } +fn hash_ptr(ptr: &SyntaxNodePtr) -> u64 { + let mut hasher = BuildHasherDefault::::default().build_hasher(); + ptr.hash(&mut hasher); + hasher.finish() +} + /// Walks the subtree in bdfs order, calling `f` for each node. What is bdfs /// order? It is a mix of breadth-first and depth first orders. Nodes for which /// `f` returns true are visited breadth-first, all the other nodes are explored