Skip to content

Commit

Permalink
refactor(semantic): dereference IDs as quickly as possible (#6303)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
overlookmotel committed Oct 6, 2024
1 parent 56d50cf commit d110700
Show file tree
Hide file tree
Showing 8 changed files with 27 additions and 29 deletions.
10 changes: 5 additions & 5 deletions crates/oxc_semantic/src/binder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ 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);

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;
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl<'a> AstNodes<'a> {
/// [`Program`]: oxc_ast::ast::Program
pub fn ancestors(&self, ast_node_id: NodeId) -> impl Iterator<Item = NodeId> + '_ {
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`].
Expand Down
8 changes: 4 additions & 4 deletions crates/oxc_semantic/src/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Item = ScopeId> + '_ {
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<Item = ScopeId> + '_ {
Expand Down Expand Up @@ -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<SymbolId> {
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
Expand All @@ -234,7 +234,7 @@ impl ScopeTree {
/// [`iter_bindings_in`]: ScopeTree::iter_bindings_in
pub fn iter_bindings(&self) -> impl Iterator<Item = (ScopeId, SymbolId, &'_ CompactStr)> + '_ {
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))
})
}

Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_semantic/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
Expand Down Expand Up @@ -238,7 +238,7 @@ impl SymbolTable {
) -> impl DoubleEndedIterator<Item = &Reference> + '_ {
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.
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_semantic/tests/integration/util/class_tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
10 changes: 4 additions & 6 deletions crates/oxc_semantic/tests/integration/util/symbol_tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 6 additions & 6 deletions crates/oxc_semantic/tests/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator<Item = ScopeId>
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());
Expand All @@ -63,7 +63,7 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator<Item = ScopeId>
"\"node\": {:?},",
semantic
.nodes()
.kind(semantic.symbols().get_declaration(*symbol_id))
.kind(semantic.symbols().get_declaration(symbol_id))
.debug_name()
)
.as_str(),
Expand All @@ -73,14 +73,14 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator<Item = ScopeId>
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());
Expand Down

0 comments on commit d110700

Please sign in to comment.