Skip to content

perf: use FxHashSet for O(1) name lookups in declaration instantiation #5024

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.

Sign up for GitHub

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

Open
tkshsbcue wants to merge 5 commits into
base: main
Choose a base branch
from
+36 −61
Open

perf: use FxHashSet for O(1) name lookups in declaration instantiation #5024

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f37cb06
perf: use FxHashSet for O(1) name lookups in declaration instantiation
tkshsbcue Mar 13, 2026
38c6905
Merge branch 'main' into perf/hashset-declaration-lookups
jedel1043 Apr 6, 2026
3f38608
style: fix formatting in declarations.rs
tkshsbcue Apr 7, 2026
fd75f4a
refactor: drop parallel Vec for declared_var_names, use FxHashSet only
tkshsbcue Apr 7, 2026
cf628f7
style: drop empty if blocks left over from Vec->FxHashSet conversion
tkshsbcue Apr 17, 2026
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
97 changes: 36 additions & 61 deletions core/engine/src/bytecompiler/declarations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use boa_ast::{
visitor::NodeRef,
};
use boa_interner::{JStrRef, Sym};
use rustc_hash::FxHashSet;

#[cfg(feature = "annex-b")]
use boa_ast::operations::annex_b_function_declarations_names;
Expand Down Expand Up @@ -71,7 +72,7 @@ pub(crate) fn global_declaration_instantiation_context(
// SKIP: 6. Let functionsToInitialize be a new empty List.

// 7. Let declaredFunctionNames be a new empty List.
let mut declared_function_names = Vec::new();
let mut declared_function_names = FxHashSet::default();

// 8. For each element d of varDeclarations, in reverse List order, do
for declaration in var_declarations.iter().rev() {
Expand All @@ -88,18 +89,15 @@ pub(crate) fn global_declaration_instantiation_context(
};

// a.iv. If declaredFunctionNames does not contain fn, then
if !declared_function_names.contains(&name.sym()) {
// SKIP: 1. Let fnDefinable be ? env.CanDeclareGlobalFunction(fn).
// SKIP: 2. If fnDefinable is false, throw a TypeError exception.
// 3. Append fn to declaredFunctionNames.
declared_function_names.push(name.sym());

// SKIP: 4. Insert d as the first element of functionsToInitialize.
}
// SKIP: 1. Let fnDefinable be ? env.CanDeclareGlobalFunction(fn).
// SKIP: 2. If fnDefinable is false, throw a TypeError exception.
// 3. Append fn to declaredFunctionNames.
// SKIP: 4. Insert d as the first element of functionsToInitialize.
declared_function_names.insert(name.sym());
}

// // 9. Let declaredVarNames be a new empty List.
let mut declared_var_names = Vec::new();
let mut declared_var_names = FxHashSet::default();

// 10. For each element d of varDeclarations, do
// a. If d is either a VariableDeclaration, a ForBinding, or a BindingIdentifier, then
Expand All @@ -115,10 +113,8 @@ pub(crate) fn global_declaration_instantiation_context(
// SKIP: a. Let vnDefinable be ? env.CanDeclareGlobalVar(vn).
// SKIP: b. If vnDefinable is false, throw a TypeError exception.
// c. If declaredVarNames does not contain vn, then
if !declared_var_names.contains(&name) {
// i. Append vn to declaredVarNames.
declared_var_names.push(name);
}
// i. Append vn to declaredVarNames.
declared_var_names.insert(name);
}
}
}
Expand Down Expand Up @@ -160,7 +156,7 @@ pub(crate) fn global_declaration_instantiation_context(
context.create_global_var_binding(f_string, false)?;

// ii. Append F to declaredFunctionOrVarNames.
declared_function_names.push(f);
declared_function_names.insert(f);
}
// iii. When the FunctionDeclaration f is evaluated, perform the following
// steps in place of the FunctionDeclaration Evaluation algorithm provided in 15.2.6:
Expand Down Expand Up @@ -240,7 +236,7 @@ pub(crate) fn prepare_eval_declaration_instantiation(

// 9. Let declaredFunctionNames be a new empty List.
#[cfg(feature = "annex-b")]
let mut declared_function_names = Vec::new();
let mut declared_function_names = FxHashSet::default();

// 10. For each element d of varDeclarations, in reverse List order, do
#[cfg(feature = "annex-b")]
Expand All @@ -258,21 +254,18 @@ pub(crate) fn prepare_eval_declaration_instantiation(
};

// a.iv. If declaredFunctionNames does not contain fn, then
if !declared_function_names.contains(&name.sym()) {
// SKIP: 1. If varEnv is a Global Environment Record, then

// 2. Append fn to declaredFunctionNames.
declared_function_names.push(name.sym());

// SKIP: 3. Insert d as the first element of functionsToInitialize.
}
// SKIP: 1. If varEnv is a Global Environment Record, then
// 2. Append fn to declaredFunctionNames.
// SKIP: 3. Insert d as the first element of functionsToInitialize.
declared_function_names.insert(name.sym());
}

// 11. NOTE: Annex B.3.2.3 adds additional steps at this point.
// 11. If strict is false, then
#[cfg(feature = "annex-b")]
if !strict {
let lexically_declared_names = lexically_declared_names(body);
let lexically_declared_names: FxHashSet<Sym> =
lexically_declared_names(body).into_iter().collect();
Copy link
Copy Markdown
Contributor

@zhuzhu81998 zhuzhu81998 Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the doc of lexically_declared_names says:

/// Returns a list with the lexical bindings of a node, which may contain duplicates.
///
/// This is equivalent to the [`LexicallyDeclaredNames`][spec] syntax operation in the spec.
///
/// [spec]: https://tc39.es/ecma262/#sec-static-semantics-lexicallydeclarednames

what cases will result in duplicates here? 🤔

Copy link
Copy Markdown
Contributor Author

@tkshsbcue tkshsbcue Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicates can occur in the !strict branch via Annex B e.g. sibling blocks each declaring the same function name, or repeated function bindings across nested CaseClauses. That said, dedup isn't really the goal of the change here; the loop below does .contains(&f) for every Annex B function name, so the win is O(1) lookup vs O(n) scan. I'll update the commit message / PR description to make that the stated motivation rather than "duplicates"

Copy link
Copy Markdown
Contributor

@zhuzhu81998 zhuzhu81998 Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern is whether or not the preservation of duplicates is necessary here for correctness/spec-compliance

Copy link
Copy Markdown
Contributor Author

@tkshsbcue tkshsbcue Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No dedup is safe here. The only use of lexically_declared_names in this block is the .contains(&f) membership check in the Annex B loop below:

  for f in annex_b_function_declarations_names(body) {                                                                                                                       
      if !lexically_declared_names.contains(&f) {                                                                                                                            
          ...                                                                                                                                                                
      }                                                                                                                                                                      
  }


// a. Let declaredFunctionOrVarNames be the list-concatenation of declaredFunctionNames and declaredVarNames.
// b. For each FunctionDeclaration f that is directly contained in the StatementList
Expand Down Expand Up @@ -405,7 +398,7 @@ impl ByteCompiler<'_> {
let mut functions_to_initialize = Vec::new();

// 7. Let declaredFunctionNames be a new empty List.
let mut declared_function_names = Vec::new();
let mut declared_function_names = FxHashSet::default();

// 8. For each element d of varDeclarations, in reverse List order, do
for declaration in var_declarations.iter().rev() {
Expand All @@ -422,17 +415,14 @@ impl ByteCompiler<'_> {
};

// a.iv. If declaredFunctionNames does not contain fn, then
if !declared_function_names.contains(&name.sym()) {
if declared_function_names.insert(name.sym()) {
// 1. Let fnDefinable be ? env.CanDeclareGlobalFunction(fn).
// 2. If fnDefinable is false, throw a TypeError exception.
// Done in `Context::global_declaration_instantiation`.
// The names checked here are the same names from the functions
// in `functions_to_initialize`, but in reverse order, so we can
// reuse `global_fns` for this check.

// 3. Append fn to declaredFunctionNames.
declared_function_names.push(name.sym());

// 4. Insert d as the first element of functionsToInitialize.
functions_to_initialize.push(declaration.clone());
}
Expand All @@ -441,7 +431,7 @@ impl ByteCompiler<'_> {
functions_to_initialize.reverse();

// 9. Let declaredVarNames be a new empty List.
let mut declared_var_names = Vec::new();
let mut declared_var_names = FxHashSet::default();

// 10. For each element d of varDeclarations, do
// a. If d is either a VariableDeclaration, a ForBinding, or a BindingIdentifier, then
Expand All @@ -462,10 +452,8 @@ impl ByteCompiler<'_> {
// for this check.

// c. If declaredVarNames does not contain vn, then
if !declared_var_names.contains(&name) {
// i. Append vn to declaredVarNames.
declared_var_names.push(name);
}
// i. Append vn to declaredVarNames.
declared_var_names.insert(name);
}
}
}
Expand Down Expand Up @@ -657,7 +645,7 @@ impl ByteCompiler<'_> {
let mut functions_to_initialize = Vec::new();

// 9. Let declaredFunctionNames be a new empty List.
let mut declared_function_names = Vec::new();
let mut declared_function_names = FxHashSet::default();

// 10. For each element d of varDeclarations, in reverse List order, do
for declaration in var_declarations.iter().rev() {
Expand All @@ -673,7 +661,7 @@ impl ByteCompiler<'_> {
VarScopedDeclaration::VariableDeclaration(_) => continue,
};
// a.iv. If declaredFunctionNames does not contain fn, then
if !declared_function_names.contains(&name.sym()) {
if declared_function_names.insert(name.sym()) {
// 1. If varEnv is a Global Environment Record, then
// a. Let fnDefinable be ? varEnv.CanDeclareGlobalFunction(fn).
// b. If fnDefinable is false, throw a TypeError exception.
Expand All @@ -682,9 +670,6 @@ impl ByteCompiler<'_> {
// in `functions_to_initialize`, but in reverse order, so we can
// reuse `global_fns` for this check.

// 2. Append fn to declaredFunctionNames.
declared_function_names.push(name.sym());

// 3. Insert d as the first element of functionsToInitialize.
functions_to_initialize.push(declaration.clone());
}
Expand Down Expand Up @@ -717,7 +702,7 @@ impl ByteCompiler<'_> {
}

// 12. Let declaredVarNames be a new empty List.
let mut declared_var_names = Vec::new();
let mut declared_var_names = FxHashSet::default();

// 13. For each element d of varDeclarations, do
for declaration in var_declarations {
Expand All @@ -739,10 +724,8 @@ impl ByteCompiler<'_> {
// for this check.

// b. If declaredVarNames does not contain vn, then
if !declared_var_names.contains(&name) {
// i. Append vn to declaredVarNames.
declared_var_names.push(name);
}
// i. Append vn to declaredVarNames.
declared_var_names.insert(name);
}
}
}
Expand Down Expand Up @@ -931,7 +914,7 @@ impl ByteCompiler<'_> {
let lexical_names = lexically_declared_names(body);

// 12. Let functionNames be a new empty List.
let mut function_names = Vec::new();
let mut function_names = FxHashSet::default();

// 13. Let functionsToInitialize be a new empty List.
let mut functions_to_initialize = Vec::new();
Expand All @@ -954,17 +937,15 @@ impl ByteCompiler<'_> {
};

// a.iii. If functionNames does not contain fn, then
if !function_names.contains(&name.sym()) {
if function_names.insert(name.sym()) {
// 1. Insert fn as the first element of functionNames.
function_names.push(name.sym());

// 2. NOTE: If there are multiple function declarations for the same name, the last declaration is used.
// 3. Insert d as the first element of functionsToInitialize.
functions_to_initialize.push(function);
}
}

function_names.reverse();
functions_to_initialize.reverse();

// 15. Let argumentsObjectNeeded be true.
Expand Down Expand Up @@ -1040,7 +1021,7 @@ impl ByteCompiler<'_> {

// 23. Else,
// a. Let parameterBindings be parameterNames.
let parameter_bindings = parameter_names.clone();
let parameter_bindings: FxHashSet<Sym> = parameter_names.iter().copied().collect();

// 24. Let iteratorRecord be CreateListIteratorRecord(argumentsList).
// 25. If hasDuplicates is true, then
Expand Down Expand Up @@ -1102,15 +1083,12 @@ impl ByteCompiler<'_> {
let mut variable_scope = self.lexical_scope.clone();

// d. Let instantiatedVarNames be a new empty List.
let mut instantiated_var_names = Vec::new();
let mut instantiated_var_names = FxHashSet::default();

// e. For each element n of varNames, do
for n in var_names {
// i. If instantiatedVarNames does not contain n, then
if !instantiated_var_names.contains(&n) {
// 1. Append n to instantiatedVarNames.
instantiated_var_names.push(n);

if instantiated_var_names.insert(n) {
let n_string = n.to_js_string(self.interner());

// 2. Perform ! varEnv.CreateMutableBinding(n, false).
Expand Down Expand Up @@ -1151,15 +1129,12 @@ impl ByteCompiler<'_> {
} else {
// a. NOTE: Only a single Environment Record is needed for the parameters and top-level vars.
// b. Let instantiatedVarNames be a copy of the List parameterBindings.
let mut instantiated_var_names = parameter_bindings;
let mut instantiated_var_names = parameter_bindings.clone();

// c. For each element n of varNames, do
for n in var_names {
// i. If instantiatedVarNames does not contain n, then
if !instantiated_var_names.contains(&n) {
// 1. Append n to instantiatedVarNames.
instantiated_var_names.push(n);

if instantiated_var_names.insert(n) {
let n = n.to_js_string(self.interner());

// 2. Perform ! env.CreateMutableBinding(n, false).
Expand Down Expand Up @@ -1210,7 +1185,7 @@ impl ByteCompiler<'_> {
);

// c. Append F to instantiatedVarNames.
instantiated_var_names.push(f);
instantiated_var_names.insert(f);
}

// 3. When the FunctionDeclaration f is evaluated, perform the following steps
Expand Down
Loading