Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(semantic): allocate Bindings in allocator #8021

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ futures = "0.3.31"
glob = "0.3.1"
globset = "0.4.15"
handlebars = "6.2.0"
hashbrown = "0.15.2"
humansize = "2.1.3"
ignore = "0.4.23"
insta = "1.41.1"
Expand Down
3 changes: 3 additions & 0 deletions crates/oxc_semantic/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,12 @@ oxc_span = { workspace = true }
oxc_syntax = { workspace = true }

assert-unchecked = { workspace = true }
bumpalo = { workspace = true, features = ["allocator-api2"] }
hashbrown = { workspace = true, features = ["allocator-api2"] }
itertools = { workspace = true }
phf = { workspace = true, features = ["macros"] }
rustc-hash = { workspace = true }
self_cell = { workspace = true }

[dev-dependencies]
insta = { workspace = true, features = ["glob"] }
Expand Down
3 changes: 1 addition & 2 deletions crates/oxc_semantic/src/binder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,9 @@ impl<'a> Binder<'a> for VariableDeclarator<'a> {
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 Down
40 changes: 25 additions & 15 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,18 @@ use oxc_syntax::{
};

use crate::{
binder::Binder, checker, class::ClassTableBuilder, diagnostics::redeclaration,
jsdoc::JSDocBuilder, label::UnusedLabels, node::AstNodes, scope::ScopeTree, stats::Stats,
symbol::SymbolTable, unresolved_stack::UnresolvedReferencesStack, JSDocFinder, Semantic,
binder::Binder,
checker,
class::ClassTableBuilder,
diagnostics::redeclaration,
jsdoc::JSDocBuilder,
label::UnusedLabels,
node::AstNodes,
scope::{Bindings, ScopeTree},
stats::Stats,
symbol::SymbolTable,
unresolved_stack::UnresolvedReferencesStack,
JSDocFinder, Semantic,
};

macro_rules! control_flow {
Expand Down Expand Up @@ -389,10 +398,9 @@ impl<'a> SemanticBuilder<'a> {
return symbol_id;
}

let name = CompactStr::new(name);
let symbol_id = self.symbols.create_symbol(
span,
name.clone(),
CompactStr::new(name),
includes,
scope_id,
self.current_node_id,
Expand Down Expand Up @@ -456,15 +464,14 @@ impl<'a> SemanticBuilder<'a> {
scope_id: ScopeId,
includes: SymbolFlags,
) -> SymbolId {
let name = CompactStr::new(name);
let symbol_id = self.symbols.create_symbol(
span,
name.clone(),
CompactStr::new(name),
includes,
self.current_scope_id,
self.current_node_id,
);
self.scope.get_bindings_mut(scope_id).insert(name, symbol_id);
self.scope.insert_binding(scope_id, name, symbol_id);
symbol_id
}

Expand Down Expand Up @@ -702,14 +709,17 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
// Move all bindings from catch clause param scope to catch clause body scope
// to make it easier to resolve references and check redeclare errors
if self.scope.get_flags(parent_scope_id).is_catch_clause() {
let parent_bindings = self.scope.get_bindings_mut(parent_scope_id);
if !parent_bindings.is_empty() {
let parent_bindings = mem::take(parent_bindings);
for &symbol_id in parent_bindings.values() {
self.symbols.set_scope_id(symbol_id, self.current_scope_id);
self.scope.cell.with_dependent_mut(|allocator, inner| {
if !inner.bindings[parent_scope_id].is_empty() {
let mut parent_bindings =
Bindings::with_hasher_in(rustc_hash::FxBuildHasher, allocator);
mem::swap(&mut inner.bindings[parent_scope_id], &mut parent_bindings);
for &symbol_id in parent_bindings.values() {
self.symbols.set_scope_id(symbol_id, self.current_scope_id);
}
inner.bindings[self.current_scope_id] = parent_bindings;
}
*self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings;
}
});
}

self.visit_statements(&it.body);
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ mod tests {
let top_level_a = semantic
.scopes()
.iter_bindings()
.find(|(_scope_id, _symbol_id, name)| name.as_str() == "Fn")
.find(|(_scope_id, _symbol_id, name)| *name == "Fn")
.unwrap();
assert_eq!(semantic.symbols.get_scope_id(top_level_a.1), top_level_a.0);
}
Expand Down
117 changes: 81 additions & 36 deletions crates/oxc_semantic/src/scope.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::mem;

use rustc_hash::FxHashMap;
use rustc_hash::{FxBuildHasher, FxHashMap};

use bumpalo::Bump;
use oxc_index::IndexVec;
use oxc_span::CompactStr;
use oxc_syntax::{
Expand All @@ -11,7 +12,7 @@ use oxc_syntax::{
symbol::SymbolId,
};

pub(crate) type Bindings = FxHashMap<CompactStr, SymbolId>;
pub(crate) type Bindings<'a> = hashbrown::HashMap<&'a str, SymbolId, FxBuildHasher, &'a Bump>;
pub type UnresolvedReferences = FxHashMap<CompactStr, Vec<ReferenceId>>;

/// Scope Tree
Expand All @@ -24,7 +25,6 @@ pub type UnresolvedReferences = FxHashMap<CompactStr, Vec<ReferenceId>>;
/// - Nodes that create a scope store the [`ScopeId`] of the scope they create.
///
/// `SoA` (Struct of Arrays) for memory efficiency.
#[derive(Debug, Default)]
pub struct ScopeTree {
/// Maps a scope to the parent scope it belongs in.
parent_ids: IndexVec<ScopeId, Option<ScopeId>>,
Expand All @@ -40,12 +40,40 @@ pub struct ScopeTree {

flags: IndexVec<ScopeId, ScopeFlags>,

pub(crate) root_unresolved_references: UnresolvedReferences,

pub(crate) cell: ScopeTreeCell,
}

impl Default for ScopeTree {
fn default() -> Self {
Self {
parent_ids: IndexVec::new(),
child_ids: IndexVec::new(),
build_child_ids: false,
node_ids: IndexVec::new(),
flags: IndexVec::new(),
root_unresolved_references: UnresolvedReferences::default(),
cell: ScopeTreeCell::new(Bump::new(), |_bump| ScopeTreeInner {
bindings: IndexVec::new(),
}),
}
}
}

self_cell::self_cell!(
pub(crate) struct ScopeTreeCell {
owner: Bump,
#[covariant]
dependent: ScopeTreeInner,
}
);

pub(crate) struct ScopeTreeInner<'cell> {
/// Symbol bindings in a scope.
///
/// A binding is a mapping from an identifier name to its [`SymbolId`]
bindings: IndexVec<ScopeId, Bindings>,

pub(crate) root_unresolved_references: UnresolvedReferences,
pub(crate) bindings: IndexVec<ScopeId, Bindings<'cell>>,
}

impl ScopeTree {
Expand Down Expand Up @@ -200,7 +228,7 @@ impl ScopeTree {

/// Check if a symbol is declared in a certain scope.
pub fn has_binding(&self, scope_id: ScopeId, name: &str) -> bool {
self.bindings[scope_id].contains_key(name)
self.cell.borrow_dependent().bindings[scope_id].contains_key(name)
}

/// Get the symbol bound to an identifier name in a scope.
Expand All @@ -212,7 +240,7 @@ impl ScopeTree {
///
/// [`find_binding`]: ScopeTree::find_binding
pub fn get_binding(&self, scope_id: ScopeId, name: &str) -> Option<SymbolId> {
self.bindings[scope_id].get(name).copied()
self.cell.borrow_dependent().bindings[scope_id].get(name).copied()
}

/// Find a binding by name in a scope or its ancestors.
Expand All @@ -221,7 +249,7 @@ 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) {
if let Some(symbol_id) = self.get_binding(scope_id, name) {
return Some(symbol_id);
}
}
Expand All @@ -231,7 +259,7 @@ impl ScopeTree {
/// Get all bound identifiers in a scope.
#[inline]
pub fn get_bindings(&self, scope_id: ScopeId) -> &Bindings {
&self.bindings[scope_id]
&self.cell.borrow_dependent().bindings[scope_id]
}

/// Get the ID of the [`AstNode`] that created a scope.
Expand All @@ -247,21 +275,24 @@ impl ScopeTree {
/// If you only want bindings in a specific scope, use [`iter_bindings_in`].
///
/// [`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))
pub fn iter_bindings(&self) -> impl Iterator<Item = (ScopeId, SymbolId, &str)> + '_ {
self.cell.borrow_dependent().bindings.iter_enumerated().flat_map(|(scope_id, bindings)| {
bindings.iter().map(move |(&name, &symbol_id)| (scope_id, symbol_id, name))
})
}

/// Iterate over bindings declared inside a scope.
#[inline]
pub fn iter_bindings_in(&self, scope_id: ScopeId) -> impl Iterator<Item = SymbolId> + '_ {
self.bindings[scope_id].values().copied()
self.cell.borrow_dependent().bindings[scope_id].values().copied()
}

#[inline]
pub(crate) fn get_bindings_mut(&mut self, scope_id: ScopeId) -> &mut Bindings {
&mut self.bindings[scope_id]
pub(crate) fn insert_binding(&mut self, scope_id: ScopeId, name: &str, symbol_id: SymbolId) {
self.cell.with_dependent_mut(|allocator, inner| {
let name = allocator.alloc_str(name);
inner.bindings[scope_id].insert(name, symbol_id);
});
}

/// Return whether this `ScopeTree` has child IDs recorded
Expand Down Expand Up @@ -307,7 +338,9 @@ impl ScopeTree {
) -> ScopeId {
let scope_id = self.parent_ids.push(parent_id);
self.flags.push(flags);
self.bindings.push(Bindings::default());
self.cell.with_dependent_mut(|allocator, inner| {
inner.bindings.push(Bindings::with_hasher_in(FxBuildHasher, allocator));
});
self.node_ids.push(node_id);
if self.build_child_ids {
self.child_ids.push(vec![]);
Expand All @@ -321,21 +354,28 @@ impl ScopeTree {
/// Add a binding to a scope.
///
/// [`binding`]: Bindings
pub fn add_binding(&mut self, scope_id: ScopeId, name: CompactStr, symbol_id: SymbolId) {
self.bindings[scope_id].insert(name, symbol_id);
pub fn add_binding(&mut self, scope_id: ScopeId, name: &str, symbol_id: SymbolId) {
self.cell.with_dependent_mut(|allocator, inner| {
let name = allocator.alloc_str(name);
inner.bindings[scope_id].insert(name, symbol_id);
});
}

/// Remove an existing binding from a scope.
pub fn remove_binding(&mut self, scope_id: ScopeId, name: &CompactStr) {
self.bindings[scope_id].remove(name);
pub fn remove_binding(&mut self, scope_id: ScopeId, name: &str) {
self.cell.with_dependent_mut(|_allocator, inner| {
inner.bindings[scope_id].remove(name);
});
}

/// Move a binding from one scope to another.
pub fn move_binding(&mut self, from: ScopeId, to: ScopeId, name: &str) {
let from_map = &mut self.bindings[from];
if let Some((name, symbol_id)) = from_map.remove_entry(name) {
self.bindings[to].insert(name, symbol_id);
}
self.cell.with_dependent_mut(|_allocator, inner| {
let from_map = &mut inner.bindings[from];
if let Some((name, symbol_id)) = from_map.remove_entry(name) {
inner.bindings[to].insert(name, symbol_id);
}
});
}

/// Rename a binding to a new name.
Expand All @@ -353,24 +393,29 @@ impl ScopeTree {
scope_id: ScopeId,
symbol_id: SymbolId,
old_name: &str,
new_name: CompactStr,
new_name: &str,
) {
let bindings = &mut self.bindings[scope_id];
// Insert on end
let existing_symbol_id = bindings.insert(new_name, symbol_id);
debug_assert!(existing_symbol_id.is_none());
// Remove old entry. `swap_remove` swaps the last entry into the place of removed entry.
// We just inserted the new entry as last, so the new entry takes the place of the old.
// Order of entries is same as before, only the key has changed.
let old_symbol_id = bindings.remove(old_name);
debug_assert_eq!(old_symbol_id, Some(symbol_id));
self.cell.with_dependent_mut(|allocator, inner| {
let new_name = allocator.alloc_str(new_name);
let bindings = &mut inner.bindings[scope_id];
// Insert on end
let existing_symbol_id = bindings.insert(new_name, symbol_id);
debug_assert!(existing_symbol_id.is_none());
// Remove old entry. `swap_remove` swaps the last entry into the place of removed entry.
// We just inserted the new entry as last, so the new entry takes the place of the old.
// Order of entries is same as before, only the key has changed.
let old_symbol_id = bindings.remove(old_name);
debug_assert_eq!(old_symbol_id, Some(symbol_id));
});
}

/// Reserve memory for an `additional` number of scopes.
pub fn reserve(&mut self, additional: usize) {
self.parent_ids.reserve(additional);
self.flags.reserve(additional);
self.bindings.reserve(additional);
self.cell.with_dependent_mut(|_allocator, inner| {
inner.bindings.reserve(additional);
});
self.node_ids.reserve(additional);
if self.build_child_ids {
self.child_ids.reserve(additional);
Expand Down
Loading
Loading