LibJS: Fast-path own-property enumeration and reduce descriptor lookups

Before this change, PropertyNameIterator (used by for..in) and
`Object::enumerable_own_property_names()` (used by `Object.keys()`,
`Object.values()`, and `Object.entries()`) enumerated an object's own
enumerable properties exactly as the spec prescribes:
- Call `internal_own_property_keys()`, allocating a list of JS::Value
  keys.
- For each key, call internal_get_own_property() to obtain a
  descriptor and check `[[Enumerable]]`.

While that is required in the general case (e.g. for Proxy objects or
platform/exotic objects that override `[[OwnPropertyKeys]]`), it's
overkill for ordinary JS objects that store their own properties in the
shape table and indexed-properties storage.

This change introduces `for_each_own_property_with_enumerability()`,
which, for objects where
`eligible_for_own_property_enumeration_fast_path()` is `true`, lets us
read the enumerability directly from shape metadata (and from
indexed-properties storage) without a per-property descriptor lookup.
When we cannot avoid `internal_get_own_property()`, we still
benefit by skipping the temporary `Vector<Value>` of keys and avoiding
the unnecessary round-trip between PropertyKey and Value.

Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -1782,7 +1782,7 @@ public:
     virtual ~PropertyNameIterator() override = default;
 
     BuiltinIterator* as_builtin_iterator_if_next_is_not_redefined(IteratorRecord const&) override { return this; }
-    ThrowCompletionOr<void> next(VM&, bool& done, Value& value) override
+    ThrowCompletionOr<void> next(VM& vm, bool& done, Value& value) override
     {
         while (true) {
             if (m_iterator == m_properties.end()) {
@@ -1794,17 +1794,17 @@ public:
             ScopeGuard remove_first = [&] { ++m_iterator; };
 
             // If the property is deleted, don't include it (invariant no. 2)
-            if (!TRY(m_object->has_property(entry.key.key)))
+            if (!TRY(m_object->has_property(entry.key)))
                 continue;
 
             done = false;
-            value = entry.value;
+            value = entry.key.to_value(vm);
             return {};
         }
     }
 
 private:
-    PropertyNameIterator(JS::Realm& realm, GC::Ref<Object> object, OrderedHashMap<PropertyKeyAndEnumerableFlag, Value> properties)
+    PropertyNameIterator(JS::Realm& realm, GC::Ref<Object> object, OrderedHashTable<PropertyKeyAndEnumerableFlag> properties)
         : Object(realm, nullptr)
         , m_object(object)
         , m_properties(move(properties))
@@ -1816,11 +1816,10 @@ private:
     {
         Base::visit_edges(visitor);
         visitor.visit(m_object);
-        visitor.visit(m_properties);
     }
 
     GC::Ref<Object> m_object;
-    OrderedHashMap<PropertyKeyAndEnumerableFlag, Value> m_properties;
+    OrderedHashTable<PropertyKeyAndEnumerableFlag> m_properties;
     decltype(m_properties.begin()) m_iterator;
 };
 
@@ -1848,39 +1847,38 @@ inline ThrowCompletionOr<Value> get_object_property_iterator(Interpreter& interp
     // Note: While the spec doesn't explicitly require these to be ordered, it says that the values should be retrieved via OwnPropertyKeys,
     //       so we just keep the order consistent anyway.
 
-    GC::OrderedRootHashMap<PropertyKeyAndEnumerableFlag, Value> properties(vm.heap());
+    size_t estimated_properties_count = 0;
     HashTable<GC::Ref<Object>> seen_objects;
+    for (auto object_to_check = GC::Ptr { object.ptr() }; object_to_check && !seen_objects.contains(*object_to_check); object_to_check = TRY(object_to_check->internal_get_prototype_of())) {
+        seen_objects.set(*object_to_check);
+        estimated_properties_count += object_to_check->own_properties_count();
+    }
+    seen_objects.clear_with_capacity();
+
+    OrderedHashTable<PropertyKeyAndEnumerableFlag> properties;
+    properties.ensure_capacity(estimated_properties_count);
+
     // Collect all keys immediately (invariant no. 5)
     for (auto object_to_check = GC::Ptr { object.ptr() }; object_to_check && !seen_objects.contains(*object_to_check); object_to_check = TRY(object_to_check->internal_get_prototype_of())) {
         seen_objects.set(*object_to_check);
-        auto keys = TRY(object_to_check->internal_own_property_keys());
-        properties.ensure_capacity(properties.size() + keys.size());
-        for (auto& key : keys) {
-            if (key.is_symbol())
-                continue;
-
+        TRY(object_to_check->for_each_own_property_with_enumerability([&](PropertyKey const& property_key, bool enumerable) -> ThrowCompletionOr<void> {
             // NOTE: If there is a non-enumerable property higher up the prototype chain with the same key,
             //       we mustn't include this property even if it's enumerable (invariant no. 5 and 6)
             //       This is achieved with the PropertyKeyAndEnumerableFlag struct, which doesn't consider
             //       the enumerable flag when comparing keys.
             PropertyKeyAndEnumerableFlag new_entry {
-                .key = TRY(PropertyKey::from_value(vm, key)),
+                .key = property_key,
                 .enumerable = false,
             };
-
             if (properties.contains(new_entry))
-                continue;
-
-            auto descriptor = TRY(object_to_check->internal_get_own_property(new_entry.key));
-            if (!descriptor.has_value())
-                continue;
-
-            new_entry.enumerable = *descriptor->enumerable;
-            properties.set(move(new_entry), key, AK::HashSetExistingEntryBehavior::Keep);
-        }
+                return {};
+            new_entry.enumerable = enumerable;
+            properties.set(move(new_entry), AK::HashSetExistingEntryBehavior::Keep);
+            return {};
+        }));
     }
 
-    properties.remove_all_matching([&](auto& key, auto&) { return !key.enumerable; });
+    properties.remove_all_matching([&](auto& key) { return !key.enumerable; });
 
     auto iterator = interpreter.realm().create<PropertyNameIterator>(interpreter.realm(), object, move(properties));
 

Libraries/LibJS/Runtime/IndexedProperties.cpp

@@ -214,6 +214,8 @@ IndexedPropertyIterator::IndexedPropertyIterator(IndexedProperties const& indexe
         m_cached_indices = m_indexed_properties.indices();
         skip_empty_indices();
     }
+    if (auto const* storage = m_indexed_properties.storage())
+        m_all_enumerable = storage->is_simple_storage();
 }
 
 IndexedPropertyIterator& IndexedPropertyIterator::operator++()
@@ -236,6 +238,13 @@ bool IndexedPropertyIterator::operator!=(IndexedPropertyIterator const& other) c
     return m_index != other.m_index;
 }
 
+bool IndexedPropertyIterator::enumerable() const
+{
+    if (m_all_enumerable)
+        return true;
+    return m_indexed_properties.get(m_index)->attributes.is_enumerable();
+}
+
 void IndexedPropertyIterator::skip_empty_indices()
 {
     for (size_t i = m_next_cached_index; i < m_cached_indices.size(); i++) {

Libraries/LibJS/Runtime/IndexedProperties.h

@@ -138,6 +138,7 @@ public:
     bool operator!=(IndexedPropertyIterator const&) const;
 
     u32 index() const { return m_index; }
+    bool enumerable() const;
 
 private:
     void skip_empty_indices();
@@ -147,6 +148,7 @@ private:
     size_t m_next_cached_index { 0 };
     u32 m_index { 0 };
     bool m_skip_empty { false };
+    bool m_all_enumerable { false };
 };
 
 class JS_API IndexedProperties {

Libraries/LibJS/Runtime/ModuleNamespaceObject.h

@@ -32,6 +32,8 @@ public:
     virtual ThrowCompletionOr<GC::RootVector<Value>> internal_own_property_keys() const override;
     virtual void initialize(Realm&) override;
 
+    virtual bool eligible_for_own_property_enumeration_fast_path() const final { return false; }
+
 private:
     ModuleNamespaceObject(Realm&, Module* module, Vector<Utf16FlyString> exports);
 

Libraries/LibJS/Runtime/Object.cpp

@@ -394,50 +394,57 @@ ThrowCompletionOr<GC::RootVector<Value>> Object::enumerable_own_property_names(P
     auto& realm = *vm.current_realm();
 
     // 1. Let ownKeys be ? O.[[OwnPropertyKeys]]().
-    auto own_keys = TRY(internal_own_property_keys());
 
     // 2. Let properties be a new empty List.
     auto properties = GC::RootVector<Value> { heap() };
+    properties.ensure_capacity(own_properties_count());
 
-    // 3. For each element key of ownKeys, do
-    for (auto& key : own_keys) {
+    auto& pre_iteration_shape = shape();
+    TRY(for_each_own_property_with_enumerability([&](PropertyKey const& property_key, bool enumerable) -> ThrowCompletionOr<void> {
         // a. If Type(key) is String, then
-        if (!key.is_string())
-            continue;
-        auto property_key = MUST(PropertyKey::from_value(vm, key));
-
         // i. Let desc be ? O.[[GetOwnProperty]](key).
-        auto descriptor = TRY(internal_get_own_property(property_key));
-
         // ii. If desc is not undefined and desc.[[Enumerable]] is true, then
-        if (descriptor.has_value() && *descriptor->enumerable) {
-            // 1. If kind is key, append key to properties.
-            if (kind == PropertyKind::Key) {
-                properties.append(key);
-                continue;
-            }
-            // 2. Else,
-
-            // a. Let value be ? Get(O, key).
-            auto value = TRY(get(property_key));
-
-            // b. If kind is value, append value to properties.
-            if (kind == PropertyKind::Value) {
-                properties.append(value);
-                continue;
-            }
-            // c. Else,
-
-            // i. Assert: kind is key+value.
-            VERIFY(kind == PropertyKind::KeyAndValue);
-
-            // ii. Let entry be CreateArrayFromList(« key, value »).
-            auto entry = Array::create_from(realm, { key, value });
-
-            // iii. Append entry to properties.
-            properties.append(entry);
+        // NOTE: If the object's shape has been mutated during iteration through own properties
+        //       by executing a getter, we can no longer assume that subsequent properties
+        //       are still present and enumerable.
+        if (shape().is_cacheable() && &shape() == &pre_iteration_shape) {
+            if (!enumerable)
+                return {};
+        } else {
+            auto descriptor = TRY(internal_get_own_property(property_key));
+            if (!descriptor.has_value() || !*descriptor->enumerable)
+                return {};
         }
-    }
+
+        // 1. If kind is key, append key to properties.
+        if (kind == PropertyKind::Key) {
+            // 1. If kind is key, append key to properties.
+            properties.append(property_key.to_value(vm));
+            return {};
+        }
+
+        // 2. Else,
+        // a. Let value be ? Get(O, key).
+        auto value = TRY(get(property_key));
+
+        // b. If kind is value, append value to properties.
+        if (kind == PropertyKind::Value) {
+            properties.append(value);
+            return {};
+        }
+
+        // c. Else,
+        // i. Assert: kind is key+value.
+        VERIFY(kind == PropertyKind::KeyAndValue);
+
+        // ii. Let entry be CreateArrayFromList(« key, value »).
+        auto entry = Array::create_from(realm, { property_key.to_value(vm), value });
+
+        // iii. Append entry to properties.
+        properties.append(entry);
+
+        return {};
+    }));
 
     // 4. Return properties.
     return { move(properties) };
@@ -1338,6 +1345,49 @@ void Object::define_intrinsic_accessor(PropertyKey const& property_key, Property
     intrinsics.set(property_key.as_string(), move(accessor));
 }
 
+ThrowCompletionOr<void> Object::for_each_own_property_with_enumerability(Function<ThrowCompletionOr<void>(PropertyKey const&, bool)>&& callback) const
+{
+    auto& vm = this->vm();
+    if (eligible_for_own_property_enumeration_fast_path()) {
+        struct OwnKey {
+            PropertyKey property_key;
+            bool enumerable;
+        };
+        GC::ConservativeVector<OwnKey> keys { heap() };
+        keys.ensure_capacity(m_indexed_properties.real_size() + shape().property_count());
+
+        for (auto& entry : m_indexed_properties)
+            keys.unchecked_append({ PropertyKey(entry.index()), entry.enumerable() });
+
+        for (auto const& [property_key, metadata] : shape().property_table()) {
+            if (!property_key.is_string())
+                continue;
+            keys.unchecked_append({ property_key, metadata.attributes.is_enumerable() });
+        }
+
+        for (auto& key : keys)
+            TRY(callback(key.property_key, key.enumerable));
+    } else {
+        auto keys = TRY(internal_own_property_keys());
+        for (auto& key : keys) {
+            auto property_key = TRY(PropertyKey::from_value(vm, key));
+            if (property_key.is_symbol())
+                continue;
+            auto descriptor = TRY(internal_get_own_property(property_key));
+            bool enumerable = false;
+            if (descriptor.has_value())
+                enumerable = *descriptor->enumerable;
+            TRY(callback(property_key, enumerable));
+        }
+    }
+    return {};
+}
+
+size_t Object::own_properties_count() const
+{
+    return m_indexed_properties.real_size() + shape().property_table().size();
+}
+
 // Simple side-effect free property lookup, following the prototype chain. Non-standard.
 Value Object::get_without_side_effects(PropertyKey const& property_key) const
 {

Libraries/LibJS/Runtime/Object.h

@@ -191,6 +191,9 @@ public:
 
     // Non-standard methods
 
+    ThrowCompletionOr<void> for_each_own_property_with_enumerability(Function<ThrowCompletionOr<void>(PropertyKey const&, bool)>&&) const;
+    size_t own_properties_count() const;
+
     Value get_without_side_effects(PropertyKey const&) const;
 
     void define_direct_property(PropertyKey const& property_key, Value value, PropertyAttributes attributes) { (void)storage_set(property_key, { value, attributes }); }
@@ -225,6 +228,8 @@ public:
     virtual bool is_array_iterator() const { return false; }
     virtual bool is_raw_json_object() const { return false; }
 
+    virtual bool eligible_for_own_property_enumeration_fast_path() const { return true; }
+
     virtual BuiltinIterator* as_builtin_iterator_if_next_is_not_redefined(IteratorRecord const&) { return nullptr; }
 
     virtual bool is_array_iterator_prototype() const { return false; }

Libraries/LibJS/Runtime/ProxyObject.h

@@ -53,6 +53,7 @@ private:
 
     virtual bool is_function() const override { return m_target->is_function(); }
     virtual bool is_proxy_object() const final { return true; }
+    virtual bool eligible_for_own_property_enumeration_fast_path() const override final { return false; }
 
     virtual ThrowCompletionOr<void> get_stack_frame_size(size_t& registers_and_constants_and_locals_count, size_t& argument_count) override;
 

Libraries/LibJS/Runtime/StringObject.h

@@ -33,6 +33,7 @@ private:
     virtual ThrowCompletionOr<GC::RootVector<Value>> internal_own_property_keys() const override;
 
     virtual bool is_string_object() const final { return true; }
+    virtual bool eligible_for_own_property_enumeration_fast_path() const override final { return false; }
     virtual void visit_edges(Visitor&) override;
 
     GC::Ref<PrimitiveString> m_string;

Libraries/LibJS/Runtime/TypedArray.h

@@ -84,6 +84,7 @@ protected:
 
 private:
     virtual void visit_edges(Visitor&) override;
+    virtual bool eligible_for_own_property_enumeration_fast_path() const final override { return false; }
 };
 
 // 10.4.5.9 TypedArray With Buffer Witness Records, https://tc39.es/ecma262/#sec-typedarray-with-buffer-witness-records

Libraries/LibJS/Tests/builtins/Object/Object.entries.js

@@ -47,6 +47,18 @@ describe("basic functionality", () => {
         let entries = Object.entries(obj);
         expect(entries).toEqual([["foo", 1]]);
     });
+
+    test("delete key from getter", () => {
+        const obj = {
+            get a() {
+                delete this.b;
+                return 1;
+            },
+            b: 2,
+        };
+
+        expect(Object.entries(obj)).toEqual([["a", 1]]);
+    });
 });
 
 describe("errors", () => {

Libraries/LibWeb/Bindings/PlatformObject.h

@@ -105,6 +105,8 @@ protected:
     // NOTE: This will crash if you make has_named_property_deleter return true but do not override this method.
     virtual WebIDL::ExceptionOr<DidDeletionFail> delete_value(String const&);
 
+    virtual bool eligible_for_own_property_enumeration_fast_path() const override final { return false; }
+
 private:
     WebIDL::ExceptionOr<void> invoke_indexed_property_setter(JS::PropertyKey const&, JS::Value);
     WebIDL::ExceptionOr<void> invoke_named_property_setter(FlyString const&, JS::Value);

Libraries/LibWeb/HTML/WindowProxy.h

@@ -33,6 +33,8 @@ public:
     virtual JS::ThrowCompletionOr<bool> internal_delete(JS::PropertyKey const&) override;
     virtual JS::ThrowCompletionOr<GC::RootVector<JS::Value>> internal_own_property_keys() const override;
 
+    virtual bool eligible_for_own_property_enumeration_fast_path() const override final { return false; }
+
     GC::Ptr<Window> window() const { return m_window; }
     void set_window(GC::Ref<Window>);
 

Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp

@@ -3383,6 +3383,8 @@ private:
     virtual JS::ThrowCompletionOr<bool> internal_set_prototype_of(JS::Object* prototype) override;
     virtual JS::ThrowCompletionOr<bool> internal_prevent_extensions() override;
 
+    virtual bool eligible_for_own_property_enumeration_fast_path() const override final { return false; }
+
     virtual void visit_edges(Visitor&) override;
 
     GC::Ref<JS::Realm> m_realm; // [[Realm]]