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. :^)

Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020-2021, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2020-2022, Andreas Kling <kling@serenityos.org>
  *
  * 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<bool> DeclarativeEnvironment::has_binding(FlyString const& name, Optional<size_t>* 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<void> 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<void> 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<void> 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<void> 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<void> DeclarativeEnvironment::initialize_binding(GlobalObject&
 ThrowCompletionOr<void> 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<ReferenceError>(global_object, ErrorType::UnknownIdentifier, name);
@@ -126,7 +126,7 @@ ThrowCompletionOr<void> 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<void> DeclarativeEnvironment::set_mutable_binding_direct(Globa
         strict = true;
 
     if (!binding.initialized)
-        return vm().throw_completion<ReferenceError>(global_object, ErrorType::BindingNotInitialized, name_from_index(index));
+        return vm().throw_completion<ReferenceError>(global_object, ErrorType::BindingNotInitialized, m_names[index]);
 
     if (binding.mutable_) {
         binding.value = value;
@@ -155,11 +155,11 @@ ThrowCompletionOr<void> DeclarativeEnvironment::set_mutable_binding_direct(Globa
 ThrowCompletionOr<Value> 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<Value> DeclarativeEnvironment::get_binding_value_direct(GlobalObject& global_object, size_t index, bool)
@@ -168,7 +168,7 @@ ThrowCompletionOr<Value> 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<ReferenceError>(global_object, ErrorType::BindingNotInitialized, name_from_index(index));
+        return vm().throw_completion<ReferenceError>(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<Value> DeclarativeEnvironment::get_binding_value_direct(Global
 ThrowCompletionOr<bool> 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<bool> DeclarativeEnvironment::delete_binding(GlobalObject&, Fl
 
 ThrowCompletionOr<void> 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<ScopeNode>,
     MUST(initialize_or_set_mutable_binding(global_object, name, value));
 }
 
-Vector<String> DeclarativeEnvironment::bindings() const
-{
-    Vector<String> 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();
-}
-
 }

Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.h

@@ -34,7 +34,7 @@ public:
     ThrowCompletionOr<void> 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<String> bindings() const;
+    [[nodiscard]] Vector<FlyString> const& bindings() const { return m_names; }
 
     ThrowCompletionOr<Value> get_binding_value_direct(GlobalObject&, size_t index, bool strict);
     ThrowCompletionOr<void> 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<FlyString, size_t> m_names;
+    Vector<FlyString> m_names;
     Vector<Binding> m_bindings;
 };
 

Userland/Utilities/js.cpp

@@ -1612,7 +1612,7 @@ ErrorOr<int> 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();