LibJS: Track current shape dictionary generation in PropertyLookupCache

When an object becomes too big (currently 64 properties or more), we
change its shape to a dictionary and don't do any further transitions.

However, this means the Shape of the object no longer changes, so the
cache invalidation check of `current_shape != cache.shape` is no longer
a valid check.

This fixes that by keeping track of a generation number for the Shape
both on the Shape object and in the cache, allowing that to be checked
instead of the Shape identity. The generation is incremented whenever
the dictionary is mutated.

Fixes stale cache lookups on Gmail preventing emails from being
displayed.

I was not able to produce a reproduction for this, plus the generation
count was over the 20k mark on Gmail.

Libraries/LibJS/Bytecode/Executable.h

@@ -43,6 +43,7 @@ struct PropertyLookupCache {
         Optional<u32> property_offset;
         GC::Weak<Object> prototype;
         GC::Weak<PrototypeChainValidity> prototype_chain_validity;
+        Optional<u32> shape_dictionary_generation;
     };
     AK::Array<Entry, max_number_of_shapes_to_remember> entries;
 };

Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -1033,7 +1033,7 @@ inline ThrowCompletionOr<Value> get_global(Interpreter& interpreter, IdentifierT
 
         // OPTIMIZATION: For global var bindings, if the shape of the global object hasn't changed,
         //               we can use the cached property offset.
-        if (&shape == cache.entries[0].shape) {
+        if (&shape == cache.entries[0].shape && (!shape.is_dictionary() || shape.dictionary_generation() == cache.entries[0].shape_dictionary_generation.value())) {
             auto value = binding_object.get_direct(cache.entries[0].property_offset.value());
             if (value.is_accessor())
                 return TRY(call(vm, value.as_accessor().getter(), js_undefined()));
@@ -1085,6 +1085,10 @@ inline ThrowCompletionOr<Value> get_global(Interpreter& interpreter, IdentifierT
         if (cacheable_metadata.type == CacheableGetPropertyMetadata::Type::GetOwnProperty) {
             cache.entries[0].shape = shape;
             cache.entries[0].property_offset = cacheable_metadata.property_offset.value();
+
+            if (shape.is_dictionary()) {
+                cache.entries[0].shape_dictionary_generation = shape.dictionary_generation();
+            }
         }
         return value;
     }
@@ -1140,6 +1144,13 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
                     bool can_use_cache = [&]() -> bool {
                         if (&object->shape() != cache.shape) [[unlikely]]
                             return false;
+
+                        if (cache.shape->is_dictionary()) {
+                            VERIFY(cache.shape_dictionary_generation.has_value());
+                            if (object->shape().dictionary_generation() != cache.shape_dictionary_generation.value()) [[unlikely]]
+                                return false;
+                        }
+
                         auto cached_prototype_chain_validity = cache.prototype_chain_validity.ptr();
                         if (!cached_prototype_chain_validity) [[unlikely]]
                             return false;
@@ -1159,6 +1170,13 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
                 case PropertyLookupCache::Entry::Type::ChangeOwnProperty: {
                     if (cache.shape != &object->shape()) [[unlikely]]
                         break;
+
+                    if (cache.shape->is_dictionary()) {
+                        VERIFY(cache.shape_dictionary_generation.has_value());
+                        if (cache.shape->dictionary_generation() != cache.shape_dictionary_generation.value())
+                            break;
+                    }
+
                     auto value_in_object = object->get_direct(cache.property_offset.value());
                     if (value_in_object.is_accessor()) [[unlikely]] {
                         (void)TRY(call(vm, value_in_object.as_accessor().setter(), this_value, value));
@@ -1175,6 +1193,13 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
                     auto cached_shape = cache.shape.ptr();
                     if (!cached_shape) [[unlikely]]
                         break;
+
+                    if (cache.shape->is_dictionary()) {
+                        VERIFY(cache.shape_dictionary_generation.has_value());
+                        if (object->shape().dictionary_generation() != cache.shape_dictionary_generation.value())
+                            break;
+                    }
+
                     // The cache is invalid if the prototype chain has been mutated, since such a mutation could have added a setter for the property.
                     auto cached_prototype_chain_validity = cache.prototype_chain_validity.ptr();
                     if (cached_prototype_chain_validity && !cached_prototype_chain_validity->is_valid()) [[unlikely]]
@@ -1209,6 +1234,9 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
             if (cacheable_metadata.prototype) {
                 cache.prototype_chain_validity = *cacheable_metadata.prototype->shape().prototype_chain_validity();
             }
+            if (cache.shape->is_dictionary()) {
+                cache.shape_dictionary_generation = cache.shape->dictionary_generation();
+            }
         }
 
         // If internal_set() caused object's shape change, we can no longer be sure
@@ -1225,6 +1253,10 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
                 cache.type = PropertyLookupCache::Entry::Type::ChangeOwnProperty;
                 cache.shape = object->shape();
                 cache.property_offset = cacheable_metadata.property_offset.value();
+
+                if (cache.shape->is_dictionary()) {
+                    cache.shape_dictionary_generation = cache.shape->dictionary_generation();
+                }
                 break;
             case CacheableSetPropertyMetadata::Type::ChangePropertyInPrototypeChain:
                 cache.type = PropertyLookupCache::Entry::Type::ChangePropertyInPrototypeChain;
@@ -1232,6 +1264,10 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
                 cache.property_offset = cacheable_metadata.property_offset.value();
                 cache.prototype = *cacheable_metadata.prototype;
                 cache.prototype_chain_validity = *cacheable_metadata.prototype->shape().prototype_chain_validity();
+
+                if (cache.shape->is_dictionary()) {
+                    cache.shape_dictionary_generation = cache.shape->dictionary_generation();
+                }
                 break;
             case CacheableSetPropertyMetadata::Type::NotCacheable:
                 break;
@@ -2294,7 +2330,7 @@ ThrowCompletionOr<void> SetGlobal::execute_impl(Bytecode::Interpreter& interpret
     if (cache.environment_serial_number == declarative_record.environment_serial_number()) {
         // OPTIMIZATION: For global var bindings, if the shape of the global object hasn't changed,
         //               we can use the cached property offset.
-        if (&shape == cache.entries[0].shape) {
+        if (&shape == cache.entries[0].shape && (!shape.is_dictionary() || shape.dictionary_generation() == cache.entries[0].shape_dictionary_generation.value())) {
             auto value = binding_object.get_direct(cache.entries[0].property_offset.value());
             if (value.is_accessor())
                 TRY(call(vm, value.as_accessor().setter(), &binding_object, src));
@@ -2363,6 +2399,10 @@ ThrowCompletionOr<void> SetGlobal::execute_impl(Bytecode::Interpreter& interpret
         if (cacheable_metadata.type == CacheableSetPropertyMetadata::Type::ChangeOwnProperty) {
             cache.entries[0].shape = shape;
             cache.entries[0].property_offset = cacheable_metadata.property_offset.value();
+
+            if (shape.is_dictionary()) {
+                cache.entries[0].shape_dictionary_generation = shape.dictionary_generation();
+            }
         }
         return {};
     }

Libraries/LibJS/Bytecode/PropertyAccess.h

@@ -98,6 +98,14 @@ ALWAYS_INLINE ThrowCompletionOr<Value> get_by_id(VM& vm, GetBaseIdentifier get_b
             bool can_use_cache = [&]() -> bool {
                 if (&shape != cache_entry.shape) [[unlikely]]
                     return false;
+
+                if (shape.is_dictionary()) {
+                    VERIFY(cache_entry.shape_dictionary_generation.has_value());
+                    if (shape.dictionary_generation() != cache_entry.shape_dictionary_generation.value()) [[unlikely]] {
+                        return false;
+                    }
+                }
+
                 auto cached_prototype_chain_validity = cache_entry.prototype_chain_validity.ptr();
                 if (!cached_prototype_chain_validity) [[unlikely]]
                     return false;
@@ -113,11 +121,21 @@ ALWAYS_INLINE ThrowCompletionOr<Value> get_by_id(VM& vm, GetBaseIdentifier get_b
             }
         } else if (&shape == cache_entry.shape) {
             // OPTIMIZATION: If the shape of the object hasn't changed, we can use the cached property offset.
-            auto value = base_obj->get_direct(cache_entry.property_offset.value());
+            bool can_use_cache = true;
-            if (value.is_accessor()) {
+            if (shape.is_dictionary()) {
-                return TRY(call(vm, value.as_accessor().getter(), this_value));
+                VERIFY(cache_entry.shape_dictionary_generation.has_value());
+                if (shape.dictionary_generation() != cache_entry.shape_dictionary_generation.value()) [[unlikely]] {
+                    can_use_cache = false;
+                }
+            }
+
+            if (can_use_cache) [[likely]] {
+                auto value = base_obj->get_direct(cache_entry.property_offset.value());
+                if (value.is_accessor()) {
+                    return TRY(call(vm, value.as_accessor().getter(), this_value));
+                }
+                return value;
             }
-            return value;
         }
     }
 
@@ -139,12 +157,20 @@ ALWAYS_INLINE ThrowCompletionOr<Value> get_by_id(VM& vm, GetBaseIdentifier get_b
             auto& entry = get_cache_slot();
             entry.shape = shape;
             entry.property_offset = cacheable_metadata.property_offset.value();
+
+            if (shape.is_dictionary()) {
+                entry.shape_dictionary_generation = shape.dictionary_generation();
+            }
         } else if (cacheable_metadata.type == CacheableGetPropertyMetadata::Type::GetPropertyInPrototypeChain) {
             auto& entry = get_cache_slot();
             entry.shape = &base_obj->shape();
             entry.property_offset = cacheable_metadata.property_offset.value();
             entry.prototype = *cacheable_metadata.prototype;
             entry.prototype_chain_validity = *prototype_chain_validity;
+
+            if (shape.is_dictionary()) {
+                entry.shape_dictionary_generation = shape.dictionary_generation();
+            }
         }
     }
 

Libraries/LibJS/Runtime/Shape.cpp

@@ -291,6 +291,7 @@ void Shape::add_property_without_transition(PropertyKey const& property_key, Pro
     if (m_property_table->set(property_key, { m_property_count, attributes }) == AK::HashSetResult::InsertedNewEntry) {
         VERIFY(m_property_count < NumericLimits<u32>::max());
         ++m_property_count;
+        ++m_dictionary_generation;
     }
 }
 
@@ -303,6 +304,7 @@ void Shape::set_property_attributes_without_transition(PropertyKey const& proper
     VERIFY(it != m_property_table->end());
     it->value.attributes = attributes;
     m_property_table->set(property_key, it->value);
+    ++m_dictionary_generation;
 }
 
 void Shape::remove_property_without_transition(PropertyKey const& property_key, u32 offset)
@@ -317,6 +319,7 @@ void Shape::remove_property_without_transition(PropertyKey const& property_key,
         if (it.value.offset > offset)
             --it.value.offset;
     }
+    ++m_dictionary_generation;
 }
 
 GC::Ref<Shape> Shape::clone_for_prototype()

Libraries/LibJS/Runtime/Shape.h

@@ -84,6 +84,8 @@ public:
     [[nodiscard]] bool is_cacheable_dictionary() const { return m_dictionary && m_cacheable; }
     [[nodiscard]] bool is_uncacheable_dictionary() const { return m_dictionary && !m_cacheable; }
 
+    [[nodiscard]] u32 dictionary_generation() const { return m_dictionary_generation; }
+
     [[nodiscard]] bool is_prototype_shape() const { return m_is_prototype_shape; }
     void set_prototype_shape();
 
@@ -137,6 +139,7 @@ private:
     GC::Ptr<PrototypeChainValidity> m_prototype_chain_validity;
 
     u32 m_property_count { 0 };
+    u32 m_dictionary_generation { 0 };
 
     PropertyAttributes m_attributes { 0 };
     TransitionType m_transition_type { TransitionType::Invalid };