From bed78eb3cc00a029e4434f3b1f5d3fcd10004abb Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 21 Jan 2024 16:06:18 +0100 Subject: [PATCH] LibJS: Don't add uncaptured parameter bindings to environment For parameters that exist strictly as "locals", we can save time and space by not adding them to the function environment. This is a speed-up across the board on basically every test. For example, ~11% on Octane/typescript.js :^) --- .../Runtime/ECMAScriptFunctionObject.cpp | 24 +++++++++++-------- .../LibJS/Runtime/ECMAScriptFunctionObject.h | 6 ++++- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp index 0ebcaaa135..e01827198f 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp @@ -125,7 +125,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt parameter.binding.visit( [&](Identifier const& identifier) { - if (m_parameter_names.set(identifier.string()) != AK::HashSetResult::InsertedNewEntry) + if (m_parameter_names.set(identifier.string(), identifier.is_local() ? ParameterIsLocal::Yes : ParameterIsLocal::No) != AK::HashSetResult::InsertedNewEntry) m_has_duplicates = true; }, [&](NonnullRefPtr const& pattern) { @@ -134,7 +134,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt // NOTE: Nothing in the callback throws an exception. MUST(pattern->for_each_bound_identifier([&](auto& identifier) { - if (m_parameter_names.set(identifier.string()) != AK::HashSetResult::InsertedNewEntry) + if (m_parameter_names.set(identifier.string(), identifier.is_local() ? ParameterIsLocal::Yes : ParameterIsLocal::No) != AK::HashSetResult::InsertedNewEntry) m_has_duplicates = true; })); }); @@ -204,13 +204,13 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt *environment_size += m_parameter_names.size(); - HashTable parameter_bindings; + HashMap parameter_bindings; // 22. If argumentsObjectNeeded is true, then if (m_arguments_object_needed) { // f. Let parameterBindings be the list-concatenation of parameterNames and « "arguments" ». parameter_bindings = m_parameter_names; - parameter_bindings.set(vm().names.arguments.as_string()); + parameter_bindings.set(vm().names.arguments.as_string(), ParameterIsLocal::No); (*environment_size)++; } else { @@ -218,7 +218,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt // a. Let parameterBindings be parameterNames. } - HashTable instantiated_var_names; + HashMap instantiated_var_names; size_t* var_environment_size = nullptr; @@ -231,7 +231,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt // c. For each element n of varNames, do MUST(scope_body->for_each_var_declared_identifier([&](auto const& id) { // i. If instantiatedVarNames does not contain n, then - if (instantiated_var_names.set(id.string()) == AK::HashSetResult::InsertedNewEntry) { + if (instantiated_var_names.set(id.string(), id.is_local() ? ParameterIsLocal::Yes : ParameterIsLocal::No) == AK::HashSetResult::InsertedNewEntry) { // 1. Append n to instantiatedVarNames. // Following steps will be executed in function_declaration_instantiation: // 2. Perform ! env.CreateMutableBinding(n, false). @@ -266,7 +266,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt // Following steps will be executed in function_declaration_instantiation: // 2. Perform ! env.CreateMutableBinding(n, false). // 3. Perform ! env.InitializeBinding(n, undefined). - if (instantiated_var_names.set(id.string()) == AK::HashSetResult::InsertedNewEntry) { + if (instantiated_var_names.set(id.string(), id.is_local() ? ParameterIsLocal::Yes : ParameterIsLocal::No) == AK::HashSetResult::InsertedNewEntry) { m_var_names_to_initialize_binding.append({ .identifier = id, .parameter_binding = parameter_bindings.contains(id.string()), @@ -290,7 +290,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Byt if (!instantiated_var_names.contains(function_name) && function_name != vm().names.arguments.as_string()) { m_function_names_to_initialize_binding.append(function_name); - instantiated_var_names.set(function_name); + instantiated_var_names.set(function_name, ParameterIsLocal::No); (*var_environment_size)++; } @@ -620,6 +620,10 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia // 21. For each String paramName of parameterNames, do for (auto const& parameter_name : m_parameter_names) { + // OPTIMIZATION: Parameters that are locals don't need to be added to the environment. + if (parameter_name.value == ParameterIsLocal::Yes) + continue; + // a. Let alreadyDeclared be ! env.HasBinding(paramName). // b. NOTE: Early errors ensure that duplicate parameter names can only occur in non-strict functions that do not have parameter default values or rest parameters. @@ -627,12 +631,12 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia // c. If alreadyDeclared is false, then // NOTE: alreadyDeclared is always false because we use hash table for parameterNames // i. Perform ! env.CreateMutableBinding(paramName, false). - MUST(environment->create_mutable_binding(vm, parameter_name, false)); + MUST(environment->create_mutable_binding(vm, parameter_name.key, false)); // ii. If hasDuplicates is true, then if (m_has_duplicates) { // 1. Perform ! env.InitializeBinding(paramName, undefined). - MUST(environment->initialize_binding(vm, parameter_name, js_undefined(), Environment::InitializeBindingHint::Normal)); + MUST(environment->initialize_binding(vm, parameter_name.key, js_undefined(), Environment::InitializeBindingHint::Normal)); } } diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h index fe1108bde5..eec843fb6a 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h @@ -153,7 +153,11 @@ private: bool m_has_parameter_expressions { false }; bool m_has_duplicates { false }; - HashTable m_parameter_names; + enum class ParameterIsLocal { + No, + Yes, + }; + HashMap m_parameter_names; Vector m_functions_to_initialize; bool m_arguments_object_needed { false }; bool m_is_module_wrapper { false };