From d11070051b4f29f6cc776efa660a8cff5a4d5545 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Sun, 6 Oct 2024 01:03:47 +0000 Subject: [PATCH] refactor(semantic): dereference IDs as quickly as possible (#6303) It's better to pass around `SymbolId`s (4 bytes) than `&SymbolId`s (8 bytes). In my view, the style of dereferencing immediately is preferable. --- crates/oxc_semantic/src/binder.rs | 10 +++++----- crates/oxc_semantic/src/builder.rs | 4 ++-- crates/oxc_semantic/src/node.rs | 2 +- crates/oxc_semantic/src/scope.rs | 8 ++++---- crates/oxc_semantic/src/symbol.rs | 6 +++--- .../tests/integration/util/class_tester.rs | 4 ++-- .../tests/integration/util/symbol_tester.rs | 10 ++++------ crates/oxc_semantic/tests/main.rs | 12 ++++++------ 8 files changed, 27 insertions(+), 29 deletions(-) diff --git a/crates/oxc_semantic/src/binder.rs b/crates/oxc_semantic/src/binder.rs index 611449bc6b7bb..bdf9bfad16cb3 100644 --- a/crates/oxc_semantic/src/binder.rs +++ b/crates/oxc_semantic/src/binder.rs @@ -65,9 +65,9 @@ impl<'a> Binder<'a> for VariableDeclarator<'a> { let name = &ident.name; let mut declared_symbol_id = None; - for scope_id in &var_scope_ids { + for &scope_id in &var_scope_ids { if let Some(symbol_id) = - builder.check_redeclaration(*scope_id, span, name, excludes, true) + builder.check_redeclaration(scope_id, span, name, excludes, true) { builder.add_redeclare_variable(symbol_id, span); declared_symbol_id = Some(symbol_id); @@ -75,7 +75,7 @@ impl<'a> Binder<'a> for VariableDeclarator<'a> { let name = name.to_compact_str(); // remove current scope binding and add to target scope // avoid same symbols appear in multi-scopes - builder.scope.remove_binding(*scope_id, &name); + builder.scope.remove_binding(scope_id, &name); builder.scope.add_binding(target_scope_id, name, symbol_id); builder.symbols.scope_ids[symbol_id] = target_scope_id; break; @@ -92,10 +92,10 @@ impl<'a> Binder<'a> for VariableDeclarator<'a> { // Finally, add the variable to all hoisted scopes // to support redeclaration checks when declaring variables with the same name later. - for scope_id in &var_scope_ids { + for &scope_id in &var_scope_ids { builder .hoisting_variables - .entry(*scope_id) + .entry(scope_id) .or_default() .insert(name.clone(), symbol_id); } diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index 516e56c32cf23..2ef8f1368659b 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -2025,8 +2025,8 @@ impl<'a> SemanticBuilder<'a> { } fn make_all_namespaces_valuelike(&mut self) { - for symbol_id in &self.namespace_stack { - let Some(symbol_id) = *symbol_id else { + for &symbol_id in &self.namespace_stack { + let Some(symbol_id) = symbol_id else { continue; }; diff --git a/crates/oxc_semantic/src/node.rs b/crates/oxc_semantic/src/node.rs index 89439410dcd06..cf47749c77fef 100644 --- a/crates/oxc_semantic/src/node.rs +++ b/crates/oxc_semantic/src/node.rs @@ -222,7 +222,7 @@ impl<'a> AstNodes<'a> { /// [`Program`]: oxc_ast::ast::Program pub fn ancestors(&self, ast_node_id: NodeId) -> impl Iterator + '_ { let parent_ids = &self.parent_ids; - std::iter::successors(Some(ast_node_id), |node_id| parent_ids[*node_id]) + std::iter::successors(Some(ast_node_id), |&node_id| parent_ids[node_id]) } /// Create and add an [`AstNode`] to the [`AstNodes`] tree and get its [`NodeId`]. diff --git a/crates/oxc_semantic/src/scope.rs b/crates/oxc_semantic/src/scope.rs index 51c33343a8f10..cb433e85b6dd7 100644 --- a/crates/oxc_semantic/src/scope.rs +++ b/crates/oxc_semantic/src/scope.rs @@ -73,7 +73,7 @@ impl ScopeTree { /// The first element of this iterator will be the scope itself. This /// guarantees the iterator will have at least 1 element. pub fn ancestors(&self, scope_id: ScopeId) -> impl Iterator + '_ { - std::iter::successors(Some(scope_id), |scope_id| self.parent_ids[*scope_id]) + std::iter::successors(Some(scope_id), |&scope_id| self.parent_ids[scope_id]) } pub fn descendants_from_root(&self) -> impl Iterator + '_ { @@ -206,8 +206,8 @@ impl ScopeTree { /// found. If no binding is found, [`None`] is returned. pub fn find_binding(&self, scope_id: ScopeId, name: &str) -> Option { for scope_id in self.ancestors(scope_id) { - if let Some(symbol_id) = self.bindings[scope_id].get(name) { - return Some(*symbol_id); + if let Some(&symbol_id) = self.bindings[scope_id].get(name) { + return Some(symbol_id); } } None @@ -234,7 +234,7 @@ impl ScopeTree { /// [`iter_bindings_in`]: ScopeTree::iter_bindings_in pub fn iter_bindings(&self) -> impl Iterator + '_ { self.bindings.iter_enumerated().flat_map(|(scope_id, bindings)| { - bindings.iter().map(move |(name, symbol_id)| (scope_id, *symbol_id, name)) + bindings.iter().map(move |(name, &symbol_id)| (scope_id, symbol_id, name)) }) } diff --git a/crates/oxc_semantic/src/symbol.rs b/crates/oxc_semantic/src/symbol.rs index 1042ce9527a31..111f03340649e 100644 --- a/crates/oxc_semantic/src/symbol.rs +++ b/crates/oxc_semantic/src/symbol.rs @@ -77,8 +77,8 @@ impl SymbolTable { /// let classes = semantic /// .scopes() /// .symbol_ids() - /// .filter(|symbol_id| { - /// let flags = semantic.symbols().get_flags(*symbol_id); + /// .filter(|&symbol_id| { + /// let flags = semantic.symbols().get_flags(symbol_id); /// flags.is_class() /// }) /// .collect::>(); @@ -238,7 +238,7 @@ impl SymbolTable { ) -> impl DoubleEndedIterator + '_ { self.resolved_references[symbol_id] .iter() - .map(|reference_id| &self.references[*reference_id]) + .map(|&reference_id| &self.references[reference_id]) } /// Delete a reference to a symbol. diff --git a/crates/oxc_semantic/tests/integration/util/class_tester.rs b/crates/oxc_semantic/tests/integration/util/class_tester.rs index 757bd51b5ce21..5637e17a48622 100644 --- a/crates/oxc_semantic/tests/integration/util/class_tester.rs +++ b/crates/oxc_semantic/tests/integration/util/class_tester.rs @@ -11,8 +11,8 @@ pub struct ClassTester<'a> { impl<'a> ClassTester<'a> { pub(super) fn has_class(semantic: Semantic<'a>, name: &str) -> Self { - let class_id = semantic.classes().iter_enumerated().find_map(|(class_id, ast_node_id)| { - let kind = semantic.nodes().kind(*ast_node_id); + let class_id = semantic.classes().iter_enumerated().find_map(|(class_id, &ast_node_id)| { + let kind = semantic.nodes().kind(ast_node_id); let class = kind.as_class()?; if class.id.clone().is_some_and(|id| id.name == name) { diff --git a/crates/oxc_semantic/tests/integration/util/symbol_tester.rs b/crates/oxc_semantic/tests/integration/util/symbol_tester.rs index a6e883dbb6b38..b46ea82324887 100644 --- a/crates/oxc_semantic/tests/integration/util/symbol_tester.rs +++ b/crates/oxc_semantic/tests/integration/util/symbol_tester.rs @@ -75,7 +75,7 @@ impl<'a> SymbolTester<'a> { 0 => Err(OxcDiagnostic::error(format!("Could not find declaration for {target}"))), 1 => Ok(symbols_with_target_name .iter() - .map(|(_, symbol_id, _)| *symbol_id) + .map(|&(_, symbol_id, _)| symbol_id) .next() .unwrap()), n if n > 1 => Err(OxcDiagnostic::error(format!( @@ -183,11 +183,9 @@ impl<'a> SymbolTester<'a> { self.test_result = match self.test_result { Ok(symbol_id) => { let refs = { - self.semantic - .symbols() - .get_resolved_reference_ids(symbol_id) - .iter() - .map(|r_id| self.semantic.symbols().get_reference(*r_id).clone()) + self.semantic.symbols().get_resolved_reference_ids(symbol_id).iter().map( + |&reference_id| self.semantic.symbols().get_reference(reference_id).clone(), + ) }; let num_accepted = refs.filter(filter).count(); if num_accepted == ref_count { diff --git a/crates/oxc_semantic/tests/main.rs b/crates/oxc_semantic/tests/main.rs index 81cf7801e87be..000d394a08d59 100644 --- a/crates/oxc_semantic/tests/main.rs +++ b/crates/oxc_semantic/tests/main.rs @@ -48,13 +48,13 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator result.push_str("\"symbols\": "); let bindings = scope_tree.get_bindings(scope_id); result.push('['); - bindings.iter().enumerate().for_each(|(index, (name, symbol_id))| { + bindings.iter().enumerate().for_each(|(index, (name, &symbol_id))| { if index != 0 { result.push(','); } result.push('{'); result.push_str( - format!("\"flags\": \"{:?}\",", semantic.symbols().get_flags(*symbol_id)).as_str(), + format!("\"flags\": \"{:?}\",", semantic.symbols().get_flags(symbol_id)).as_str(), ); result.push_str(format!("\"id\": {},", symbol_id.index()).as_str()); result.push_str(format!("\"name\": {name:?},").as_str()); @@ -63,7 +63,7 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator "\"node\": {:?},", semantic .nodes() - .kind(semantic.symbols().get_declaration(*symbol_id)) + .kind(semantic.symbols().get_declaration(symbol_id)) .debug_name() ) .as_str(), @@ -73,14 +73,14 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator result.push('['); semantic .symbols() - .get_resolved_reference_ids(*symbol_id) + .get_resolved_reference_ids(symbol_id) .iter() .enumerate() - .for_each(|(index, reference_id)| { + .for_each(|(index, &reference_id)| { if index != 0 { result.push(','); } - let reference = &semantic.symbols().references[*reference_id]; + let reference = &semantic.symbols().references[reference_id]; result.push('{'); result .push_str(format!("\"flags\": \"{:?}\",", reference.flags()).as_str());