LibJS: Reduce number of proxy traps called during for..in iteration

Before this change, we would enumerate all the keys with
[[OwnPropertyKeys]], and then do [[GetOwnPropertyDescriptor]] twice for
each key as we went through them.

We now only do one [[GetOwnPropertyDescriptor]] per key, which
drastically reduces the number of proxy traps when those are involved.
The new trap sequence matches what you get with V8, so I don't think
anyone will be unpleasantly surprised here.

Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -37,30 +37,6 @@
 #include <LibJS/Runtime/ValueInlines.h>
 #include <LibJS/SourceTextModule.h>
 
-namespace JS {
-
-struct PropertyKeyAndEnumerableFlag {
-    JS::PropertyKey key;
-    bool enumerable { false };
-};
-
-}
-
-namespace AK {
-template<>
-struct Traits<JS::PropertyKeyAndEnumerableFlag> : public DefaultTraits<JS::PropertyKeyAndEnumerableFlag> {
-    static unsigned hash(JS::PropertyKeyAndEnumerableFlag const& entry)
-    {
-        return Traits<JS::PropertyKey>::hash(entry.key);
-    }
-
-    static bool equals(JS::PropertyKeyAndEnumerableFlag const& a, JS::PropertyKeyAndEnumerableFlag const& b)
-    {
-        return Traits<JS::PropertyKey>::equals(a.key, b.key);
-    }
-};
-}
-
 namespace JS::Bytecode {
 
 bool g_dump_bytecode = false;
@@ -1718,6 +1694,16 @@ inline ThrowCompletionOr<Value> delete_by_value_with_this(Bytecode::Interpreter&
     return Value(TRY(reference.delete_(vm)));
 }
 
+class PropertyNameIteratorState final : public GC::Cell {
+    GC_CELL(PropertyNameIteratorState, GC::Cell);
+    GC_DECLARE_ALLOCATOR(PropertyNameIteratorState);
+
+public:
+    HashTable<PropertyKey> non_enumerable_property_keys;
+};
+
+GC_DEFINE_ALLOCATOR(PropertyNameIteratorState);
+
 // 14.7.5.9 EnumerateObjectProperties ( O ), https://tc39.es/ecma262/#sec-enumerate-object-properties
 inline ThrowCompletionOr<Object*> get_object_property_iterator(VM& vm, Value value)
 {
@@ -1738,60 +1724,74 @@ inline ThrowCompletionOr<Object*> get_object_property_iterator(VM& vm, Value val
     // 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.
 
-    OrderedHashTable<PropertyKeyAndEnumerableFlag> properties;
+    struct ObjectWithProperties {
+        WeakPtr<Object> object;
+        OrderedHashTable<PropertyKey> property_names;
+    };
+    Vector<ObjectWithProperties> objects_with_properties;
+
     HashTable<GC::Ref<Object>> seen_objects;
+    HashTable<PropertyKey> seen_property_keys;
     // 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);
+        objects_with_properties.prepend(ObjectWithProperties {
+            .object = *object_to_check,
+            .property_names = {},
+        });
+        auto& properties = objects_with_properties.first().property_names;
         for (auto& key : TRY(object_to_check->internal_own_property_keys())) {
             if (key.is_symbol())
                 continue;
 
-            // NOTE: If there is a non-enumerable property higher up the prototype chain with the same key,
+            auto property_key = TRY(PropertyKey::from_value(vm, 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)),
-                .enumerable = false,
-            };
 
-            if (properties.contains(new_entry))
+            if (seen_property_keys.set(property_key, AK::HashSetExistingEntryBehavior::Keep) == HashSetResult::KeptExistingEntry)
                 continue;
 
-            auto descriptor = TRY(object_to_check->internal_get_own_property(new_entry.key));
+            properties.set(property_key);
-            if (!descriptor.has_value())
-                continue;
-
-            new_entry.enumerable = *descriptor->enumerable;
-            properties.set(move(new_entry), AK::HashSetExistingEntryBehavior::Keep);
         }
     }
 
-    properties.remove_all_matching([&](auto& entry) { return !entry.enumerable; });
-
     auto& realm = *vm.current_realm();
 
     auto result_object = Object::create_with_premade_shape(realm.intrinsics().iterator_result_object_shape());
     auto value_offset = realm.intrinsics().iterator_result_object_value_offset();
     auto done_offset = realm.intrinsics().iterator_result_object_done_offset();
 
+    auto state = realm.heap().allocate<PropertyNameIteratorState>();
+
     auto callback = NativeFunction::create(
-        *vm.current_realm(), [items = move(properties), result_object, value_offset, done_offset](VM& vm) mutable -> ThrowCompletionOr<Value> {
+        *vm.current_realm(), [objects_with_properties = move(objects_with_properties), result_object, value_offset, done_offset, state](VM& vm) mutable -> ThrowCompletionOr<Value> {
-            auto& iterated_object = vm.this_value().as_object();
             while (true) {
-                if (items.is_empty()) {
+                if (objects_with_properties.is_empty()) {
                     result_object->put_direct(done_offset, JS::Value(true));
                     return result_object;
                 }
 
-                auto key = move(items.take_first().key);
+                auto& object = objects_with_properties.last();
 
-                // If the property is deleted, don't include it (invariant no. 2)
+                if (object.property_names.is_empty()
-                if (!TRY(iterated_object.has_property(key)))
+                    || !object.object) {
+                    objects_with_properties.take_last();
+                    continue;
+                }
+
+                auto key = object.property_names.take_first();
+
+                // 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)
+                if (state->non_enumerable_property_keys.contains(key))
                     continue;
 
-                result_object->put_direct(done_offset, JS::Value(false));
+                auto descriptor = TRY(object.object->internal_get_own_property(key));
+                if (!descriptor.has_value())
+                    continue;
+
+                if (!*descriptor->enumerable) {
+                    state->non_enumerable_property_keys.set(move(key));
+                    continue;
+                }
 
                 if (key.is_number())
                     result_object->put_direct(value_offset, PrimitiveString::create(vm, String::number(key.as_number())));
@@ -1800,6 +1800,7 @@ inline ThrowCompletionOr<Object*> get_object_property_iterator(VM& vm, Value val
                 else
                     VERIFY_NOT_REACHED(); // We should not have non-string/number keys.
 
+                result_object->put_direct(done_offset, JS::Value(false));
                 return result_object;
             }
         },

Libraries/LibJS/Tests/builtins/Proxy/for-in-iteration-traps.js

@@ -0,0 +1,40 @@
+test("for..in iteration Proxy traps", () => {
+    let traps = [];
+    let array = [1, 2, 3];
+    let from = new Proxy(array, {
+        getPrototypeOf: function (t) {
+            traps.push("getPrototypeOf");
+            return Reflect.getPrototypeOf(t);
+        },
+        ownKeys: function (t) {
+            traps.push("ownKeys");
+            return Reflect.ownKeys(t);
+        },
+        has: function (t, p) {
+            traps.push("has");
+            return Reflect.has(t, p);
+        },
+        getOwnPropertyDescriptor: function (t, p) {
+            traps.push("getOwnPropertyDescriptor");
+            return Reflect.getOwnPropertyDescriptor(t, p);
+        },
+    });
+    const to = [];
+    for (const prop in from) {
+        to.push(prop);
+        from.pop();
+    }
+
+    expect(to).toEqual(["0", "1"]);
+
+    expect(traps).toEqual([
+        "ownKeys",
+        "getPrototypeOf",
+        "getOwnPropertyDescriptor",
+        "getOwnPropertyDescriptor",
+        "getOwnPropertyDescriptor",
+        "getOwnPropertyDescriptor",
+        "getOwnPropertyDescriptor",
+        "getOwnPropertyDescriptor",
+    ]);
+});