From ff60e8ffc6e940bcf71cc1a5a697d1b2c25c6caa Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 7 Mar 2022 14:02:37 +0100 Subject: [PATCH] LibJS: Use Vector instead of HashMap in DeclarativeEnvironment Constructing the HashMap in DeclarativeEnvironment was by far the most expensive thing when making JavaScript function calls. As it turns out, we don't really need this to be a HashMap in the first place, as lookups are cached (by EnvironmentCoordinate) after the first access, so after that we were not even looking in the HashMap, going directly to the bindings Vector instead. This reduces function_declaration_instantiation() from 16% to 9% when idling in "Biolab Disaster". It also reduces has_binding() from 3% to 1% on the same content. With these changes, we now actually get to idle a little bit between game frames on my machine. :^) --- .../LibJS/Runtime/DeclarativeEnvironment.cpp | 72 +++++++------------ .../LibJS/Runtime/DeclarativeEnvironment.h | 6 +- Userland/Utilities/js.cpp | 2 +- 3 files changed, 30 insertions(+), 50 deletions(-) diff --git a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp index 17e126ffd02..4099785a4c2 100644 --- a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp +++ b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020-2021, Andreas Kling + * Copyright (c) 2020-2022, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -37,11 +37,11 @@ void DeclarativeEnvironment::visit_edges(Visitor& visitor) // 9.1.1.1.1 HasBinding ( N ), https://tc39.es/ecma262/#sec-declarative-environment-records-hasbinding-n ThrowCompletionOr DeclarativeEnvironment::has_binding(FlyString const& name, Optional* out_index) const { - auto it = m_names.find(name); - if (it == m_names.end()) + auto index = m_names.find_first_index(name); + if (!index.has_value()) return false; if (!is_permanently_screwed_by_eval() && out_index) - *out_index = it->value; + *out_index = *index; return true; } @@ -56,10 +56,10 @@ ThrowCompletionOr DeclarativeEnvironment::create_mutable_binding(GlobalObj .can_be_deleted = can_be_deleted, .initialized = false, }); - auto result = m_names.set(name, m_bindings.size() - 1); + m_names.append(name); // 1. Assert: envRec does not already have a binding for N. - VERIFY(result == AK::HashSetResult::InsertedNewEntry); + // NOTE: We skip this to avoid O(n) traversal of m_names. // 3. Return NormalCompletion(empty). return {}; @@ -76,10 +76,10 @@ ThrowCompletionOr DeclarativeEnvironment::create_immutable_binding(GlobalO .can_be_deleted = false, .initialized = false, }); - auto result = m_names.set(name, m_bindings.size() - 1); + m_names.append(name); // 1. Assert: envRec does not already have a binding for N. - VERIFY(result == AK::HashSetResult::InsertedNewEntry); + // NOTE: We skip this to avoid O(n) traversal of m_names. // 3. Return NormalCompletion(empty). return {}; @@ -88,9 +88,9 @@ ThrowCompletionOr DeclarativeEnvironment::create_immutable_binding(GlobalO // 9.1.1.1.4 InitializeBinding ( N, V ), https://tc39.es/ecma262/#sec-declarative-environment-records-initializebinding-n-v ThrowCompletionOr DeclarativeEnvironment::initialize_binding(GlobalObject&, FlyString const& name, Value value) { - auto it = m_names.find(name); - VERIFY(it != m_names.end()); - auto& binding = m_bindings[it->value]; + auto index = m_names.find_first_index(name); + VERIFY(index.has_value()); + auto& binding = m_bindings[*index]; // 1. Assert: envRec must have an uninitialized binding for N. VERIFY(binding.initialized == false); @@ -109,8 +109,8 @@ ThrowCompletionOr DeclarativeEnvironment::initialize_binding(GlobalObject& ThrowCompletionOr DeclarativeEnvironment::set_mutable_binding(GlobalObject& global_object, FlyString const& name, Value value, bool strict) { // 1. If envRec does not have a binding for N, then - auto it = m_names.find(name); - if (it == m_names.end()) { + auto index = m_names.find_first_index(name); + if (!index.has_value()) { // a. If S is true, throw a ReferenceError exception. if (strict) return vm().throw_completion(global_object, ErrorType::UnknownIdentifier, name); @@ -126,7 +126,7 @@ ThrowCompletionOr DeclarativeEnvironment::set_mutable_binding(GlobalObject } // 2-5. (extracted into a non-standard function below) - TRY(set_mutable_binding_direct(global_object, it->value, value, strict)); + TRY(set_mutable_binding_direct(global_object, *index, value, strict)); // 6. Return NormalCompletion(empty). return {}; @@ -139,7 +139,7 @@ ThrowCompletionOr DeclarativeEnvironment::set_mutable_binding_direct(Globa strict = true; if (!binding.initialized) - return vm().throw_completion(global_object, ErrorType::BindingNotInitialized, name_from_index(index)); + return vm().throw_completion(global_object, ErrorType::BindingNotInitialized, m_names[index]); if (binding.mutable_) { binding.value = value; @@ -155,11 +155,11 @@ ThrowCompletionOr DeclarativeEnvironment::set_mutable_binding_direct(Globa ThrowCompletionOr DeclarativeEnvironment::get_binding_value(GlobalObject& global_object, FlyString const& name, bool strict) { // 1. Assert: envRec has a binding for N. - auto it = m_names.find(name); - VERIFY(it != m_names.end()); + auto index = m_names.find_first_index(name); + VERIFY(index.has_value()); // 2-3. (extracted into a non-standard function below) - return get_binding_value_direct(global_object, it->value, strict); + return get_binding_value_direct(global_object, *index, strict); } ThrowCompletionOr DeclarativeEnvironment::get_binding_value_direct(GlobalObject& global_object, size_t index, bool) @@ -168,7 +168,7 @@ ThrowCompletionOr DeclarativeEnvironment::get_binding_value_direct(Global // 2. If the binding for N in envRec is an uninitialized binding, throw a ReferenceError exception. if (!binding.initialized) - return vm().throw_completion(global_object, ErrorType::BindingNotInitialized, name_from_index(index)); + return vm().throw_completion(global_object, ErrorType::BindingNotInitialized, m_names[index]); // 3. Return the value currently bound to N in envRec. return binding.value; @@ -178,19 +178,19 @@ ThrowCompletionOr DeclarativeEnvironment::get_binding_value_direct(Global ThrowCompletionOr DeclarativeEnvironment::delete_binding(GlobalObject&, FlyString const& name) { // 1. Assert: envRec has a binding for the name that is the value of N. - auto it = m_names.find(name); - VERIFY(it != m_names.end()); + auto index = m_names.find_first_index(name); + VERIFY(index.has_value()); - auto& binding = m_bindings[it->value]; + auto& binding = m_bindings[*index]; // 2. If the binding for N in envRec cannot be deleted, return false. if (!binding.can_be_deleted) return false; // 3. Remove the binding for N from envRec. - // NOTE: We keep the entry in m_bindings to avoid disturbing indices. + // NOTE: We keep the entries in m_bindings and m_names to avoid disturbing indices. binding = {}; - m_names.remove(it); + m_names[*index] = {}; // 4. Return true. return true; @@ -198,9 +198,9 @@ ThrowCompletionOr DeclarativeEnvironment::delete_binding(GlobalObject&, Fl ThrowCompletionOr DeclarativeEnvironment::initialize_or_set_mutable_binding(GlobalObject& global_object, FlyString const& name, Value value) { - auto it = m_names.find(name); - VERIFY(it != m_names.end()); - auto& binding = m_bindings[it->value]; + auto index = m_names.find_first_index(name); + VERIFY(index.has_value()); + auto& binding = m_bindings[*index]; if (!binding.initialized) TRY(initialize_binding(global_object, name, value)); else @@ -213,22 +213,4 @@ void DeclarativeEnvironment::initialize_or_set_mutable_binding(Badge, MUST(initialize_or_set_mutable_binding(global_object, name, value)); } -Vector DeclarativeEnvironment::bindings() const -{ - Vector names; - for (auto& it : m_names) { - names.append(it.key); - } - return names; -} - -FlyString const& DeclarativeEnvironment::name_from_index(size_t index) const -{ - for (auto& it : m_names) { - if (it.value == index) - return it.key; - } - VERIFY_NOT_REACHED(); -} - } diff --git a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.h b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.h index 4da324a0d82..036620bb42d 100644 --- a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.h +++ b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.h @@ -34,7 +34,7 @@ public: ThrowCompletionOr initialize_or_set_mutable_binding(GlobalObject& global_object, FlyString const& name, Value value); // This is not a method defined in the spec! Do not use this in any LibJS (or other spec related) code. - [[nodiscard]] Vector bindings() const; + [[nodiscard]] Vector const& bindings() const { return m_names; } ThrowCompletionOr get_binding_value_direct(GlobalObject&, size_t index, bool strict); ThrowCompletionOr set_mutable_binding_direct(GlobalObject&, size_t index, Value, bool strict); @@ -43,8 +43,6 @@ protected: virtual void visit_edges(Visitor&) override; private: - FlyString const& name_from_index(size_t) const; - virtual bool is_declarative_environment() const override { return true; } struct Binding { @@ -55,7 +53,7 @@ private: bool initialized { false }; }; - HashMap m_names; + Vector m_names; Vector m_bindings; }; diff --git a/Userland/Utilities/js.cpp b/Userland/Utilities/js.cpp index 6672cd01faf..c9ec2e29b5a 100644 --- a/Userland/Utilities/js.cpp +++ b/Userland/Utilities/js.cpp @@ -1612,7 +1612,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto const& variable = interpreter->global_object(); list_all_properties(variable.shape(), variable_name); - for (String& name : global_environment.declarative_record().bindings()) { + for (auto const& name : global_environment.declarative_record().bindings()) { if (name.starts_with(variable_name)) { results.empend(name); results.last().invariant_offset = variable_name.length();