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): use Atom instead of CompactStr for UnresolvedReferencesStack #4401

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
26 changes: 15 additions & 11 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use oxc_cfg::{
IterationInstructionKind, ReturnInstructionKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_span::{CompactStr, SourceType, Span};
use oxc_span::{Atom, CompactStr, SourceType, Span};
use oxc_syntax::{module_record::ModuleRecord, operator::AssignmentOperator};

use crate::{
Expand Down Expand Up @@ -72,7 +72,7 @@ pub struct SemanticBuilder<'a> {
pub scope: ScopeTree,
pub symbols: SymbolTable,

unresolved_references: UnresolvedReferencesStack,
unresolved_references: UnresolvedReferencesStack<'a>,

pub(crate) module_record: Arc<ModuleRecord>,

Expand Down Expand Up @@ -218,7 +218,12 @@ impl<'a> SemanticBuilder<'a> {
}

debug_assert_eq!(self.unresolved_references.scope_depth(), 1);
self.scope.root_unresolved_references = self.unresolved_references.into_root();
self.scope.root_unresolved_references = self
.unresolved_references
.into_root()
.into_iter()
.map(|(k, v)| (k.into(), v))
.collect();

let jsdoc = if self.build_jsdoc { self.jsdoc.build() } else { JSDocFinder::default() };

Expand Down Expand Up @@ -367,14 +372,13 @@ impl<'a> SemanticBuilder<'a> {
/// Declare an unresolved reference in the current scope.
///
/// # Panics
pub fn declare_reference(&mut self, reference: Reference) -> ReferenceId {
let reference_name = reference.name().clone();
pub fn declare_reference(&mut self, name: Atom<'a>, reference: Reference) -> ReferenceId {
let reference_flag = *reference.flag();
let reference_id = self.symbols.create_reference(reference);

self.unresolved_references
.current_mut()
.entry(reference_name)
.entry(name)
.or_default()
.push((reference_id, reference_flag));
reference_id
Expand Down Expand Up @@ -404,7 +408,7 @@ impl<'a> SemanticBuilder<'a> {
// Try to resolve a reference.
// If unresolved, transfer it to parent scope's unresolved references.
let bindings = self.scope.get_bindings(self.current_scope_id);
if let Some(symbol_id) = bindings.get(&name).copied() {
if let Some(symbol_id) = bindings.get(name.as_str()).copied() {
let symbol_flag = self.symbols.get_flag(symbol_id);

let resolved_references: &mut Vec<_> =
Expand Down Expand Up @@ -1908,11 +1912,11 @@ impl<'a> SemanticBuilder<'a> {
}
}

fn reference_identifier(&mut self, ident: &IdentifierReference) {
fn reference_identifier(&mut self, ident: &IdentifierReference<'a>) {
let flag = self.resolve_reference_usages();
let name = ident.name.to_compact_str();
let reference = Reference::new(ident.span, name, self.current_node_id, flag);
let reference_id = self.declare_reference(reference);
let reference_id = self.declare_reference(ident.name.clone(), reference);
ident.reference_id.set(Some(reference_id));
}

Expand All @@ -1925,7 +1929,7 @@ impl<'a> SemanticBuilder<'a> {
}
}

fn reference_jsx_identifier(&mut self, ident: &JSXIdentifier) {
fn reference_jsx_identifier(&mut self, ident: &JSXIdentifier<'a>) {
match self.nodes.parent_kind(self.current_node_id) {
Some(AstKind::JSXElementName(_)) => {
if !ident.name.chars().next().is_some_and(char::is_uppercase) {
Expand All @@ -1941,7 +1945,7 @@ impl<'a> SemanticBuilder<'a> {
self.current_node_id,
ReferenceFlag::read(),
);
self.declare_reference(reference);
self.declare_reference(ident.name.clone(), reference);
}

fn is_not_expression_statement_parent(&self) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/src/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type FxIndexMap<K, V> = IndexMap<K, V, BuildHasherDefault<FxHasher>>;

type Bindings = FxIndexMap<CompactStr, SymbolId>;
pub(crate) type UnresolvedReference = (ReferenceId, ReferenceFlag);
pub(crate) type UnresolvedReferences = FxHashMap<CompactStr, Vec<UnresolvedReference>>;
pub type UnresolvedReferences = FxHashMap<CompactStr, Vec<UnresolvedReference>>;

/// Scope Tree
///
Expand Down
23 changes: 14 additions & 9 deletions crates/oxc_semantic/src/unresolved_stack.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,26 @@
use assert_unchecked::assert_unchecked;
use oxc_span::Atom;
use rustc_hash::FxHashMap;

use crate::scope::UnresolvedReferences;
use crate::scope::UnresolvedReference;

/// The difference with Scope's `UnresolvedReferences` is that this type uses Atom as the key. its clone is very cheap!
type TempUnresolvedReferences<'a> = FxHashMap<Atom<'a>, Vec<UnresolvedReference>>;

// Stack used to accumulate unresolved refs while traversing scopes.
// Indexed by scope depth. We recycle `UnresolvedReferences` instances during traversal
// to reduce allocations, so the stack grows to maximum scope depth, but never shrinks.
// See: <https://github.com/oxc-project/oxc/issues/4169>
// This stack abstraction uses the invariant that stack only grows to avoid bounds checks.
pub(crate) struct UnresolvedReferencesStack {
stack: Vec<UnresolvedReferences>,
pub(crate) struct UnresolvedReferencesStack<'a> {
stack: Vec<TempUnresolvedReferences<'a>>,
/// Current scope depth.
/// 0 is global scope. 1 is `Program`.
/// Incremented on entering a scope, and decremented on exit.
current_scope_depth: usize,
}

impl UnresolvedReferencesStack {
impl<'a> UnresolvedReferencesStack<'a> {
// Most programs will have at least 1 place where scope depth reaches 16,
// so initialize `stack` with this length, to reduce reallocations as it grows.
// This is just an estimate of a good initial size, but certainly better than
Expand All @@ -31,7 +36,7 @@ impl UnresolvedReferencesStack {

pub(crate) fn new() -> Self {
let mut stack = vec![];
stack.resize_with(Self::INITIAL_SIZE, UnresolvedReferences::default);
stack.resize_with(Self::INITIAL_SIZE, TempUnresolvedReferences::default);
Self { stack, current_scope_depth: Self::INITIAL_DEPTH }
}

Expand All @@ -40,7 +45,7 @@ impl UnresolvedReferencesStack {

// Grow stack if required to ensure `self.stack[self.current_scope_depth]` is in bounds
if self.stack.len() <= self.current_scope_depth {
self.stack.push(UnresolvedReferences::default());
self.stack.push(TempUnresolvedReferences::default());
}
}

Expand All @@ -57,7 +62,7 @@ impl UnresolvedReferencesStack {
}

/// Get unresolved references hash map for current scope
pub(crate) fn current_mut(&mut self) -> &mut UnresolvedReferences {
pub(crate) fn current_mut(&mut self) -> &mut TempUnresolvedReferences<'a> {
// SAFETY: `stack.len() > current_scope_depth` initially.
// Thereafter, `stack` never shrinks, only grows.
// `current_scope_depth` is only increased in `increment_scope_depth`,
Expand All @@ -69,7 +74,7 @@ impl UnresolvedReferencesStack {
/// Get unresolved references hash maps for current scope, and parent scope
pub(crate) fn current_and_parent_mut(
&mut self,
) -> (&mut UnresolvedReferences, &mut UnresolvedReferences) {
) -> (&mut TempUnresolvedReferences<'a>, &mut TempUnresolvedReferences<'a>) {
// Assert invariants to remove bounds checks in code below.
// https://godbolt.org/z/vv5Wo5csv
// SAFETY: `current_scope_depth` starts at 1, and is only decremented
Expand All @@ -87,7 +92,7 @@ impl UnresolvedReferencesStack {
(current, parent)
}

pub(crate) fn into_root(self) -> UnresolvedReferences {
pub(crate) fn into_root(self) -> TempUnresolvedReferences<'a> {
// SAFETY: Stack starts with a non-zero size and never shrinks.
// This assertion removes bounds check in `.next()`.
unsafe { assert_unchecked!(!self.stack.is_empty()) };
Expand Down