[vm] Don't stop mutators when inserting into a megamorphic cache.

rmacnak-google · Commit Queue · commit 9e3998623a77 · 2025-07-14T15:07:21.000-07:00
TEST=rr --chaos Change-Id: I4501d10b09691907e88f92aacbd3e4071d05e107 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/440065 Reviewed-by: Alexander Aprelev <aam@google.com> Commit-Queue: Ryan Macnak <rmacnak@google.com>
diff --git a/runtime/tests/vm/dart/isolates/concurrent_megamorphic_call_test.dart b/runtime/tests/vm/dart/isolates/concurrent_megamorphic_call_test.dart
diff --git a/runtime/vm/compiler/stub_code_compiler_arm.cc b/runtime/vm/compiler/stub_code_compiler_arm.cc
@@ -3288,12 +3288,12 @@ void StubCodeCompiler::GenerateOptimizedIdenticalWithNumberCheckStub() {
 void StubCodeCompiler::GenerateMegamorphicCallStub() {
   __ LoadTaggedClassIdMayBeSmi(R8, R0);
   // R8: receiver cid as Smi.
-  __ ldr(R2,
-         FieldAddress(IC_DATA_REG, target::MegamorphicCache::buckets_offset()));
   __ ldr(R1,
          FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
+  __ ldr(R2,
+         FieldAddress(IC_DATA_REG, target::MegamorphicCache::buckets_offset()));
+  // R1: mask as a smi - load first to support insertion w/o stopping Dart code.
   // R2: cache buckets array.
-  // R1: mask as a smi.
 
   // Compute the table index.
   ASSERT(target::MegamorphicCache::kSpreadFactor == 7);
diff --git a/runtime/vm/compiler/stub_code_compiler_arm64.cc b/runtime/vm/compiler/stub_code_compiler_arm64.cc
@@ -3683,12 +3683,12 @@ void StubCodeCompiler::GenerateMegamorphicCallStub() {
 
   Label cid_loaded;
   __ Bind(&cid_loaded);
-  __ ldr(R2,
-         FieldAddress(IC_DATA_REG, target::MegamorphicCache::buckets_offset()));
   __ ldr(R1,
          FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
+  __ ldr(R2,
+         FieldAddress(IC_DATA_REG, target::MegamorphicCache::buckets_offset()));
+  // R1: mask as a smi - load first to support insertion w/o stopping Dart code.
   // R2: cache buckets array.
-  // R1: mask as a smi.
 
   // Make the cid into a smi.
   __ SmiTag(R8);
diff --git a/runtime/vm/compiler/stub_code_compiler_ia32.cc b/runtime/vm/compiler/stub_code_compiler_ia32.cc
@@ -3177,8 +3177,8 @@ void StubCodeCompiler::GenerateMegamorphicCallStub() {
           FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
   __ movl(EDI, FieldAddress(IC_DATA_REG,
                             target::MegamorphicCache::buckets_offset()));
+  // EBX: mask as a smi - load first to support insert w/o stopping Dart code.
   // EDI: cache buckets array.
-  // EBX: mask as a smi.
 
   // Tag cid as a smi.
   __ addl(EAX, EAX);
diff --git a/runtime/vm/compiler/stub_code_compiler_riscv.cc b/runtime/vm/compiler/stub_code_compiler_riscv.cc
@@ -3194,11 +3194,11 @@ void StubCodeCompiler::GenerateMegamorphicCallStub() {
 
   Label cid_loaded;
   __ Bind(&cid_loaded);
+  __ lx(T1, FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
   __ lx(T2,
         FieldAddress(IC_DATA_REG, target::MegamorphicCache::buckets_offset()));
-  __ lx(T1, FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
+  // T1: mask as a smi - load first to support insertion w/o stopping Dart code.
   // T2: cache buckets array.
-  // T1: mask as a smi.
 
   // Make the cid into a smi.
   __ SmiTag(T5);
diff --git a/runtime/vm/compiler/stub_code_compiler_x64.cc b/runtime/vm/compiler/stub_code_compiler_x64.cc
@@ -3587,7 +3587,7 @@ void StubCodeCompiler::GenerateMegamorphicCallStub() {
           FieldAddress(IC_DATA_REG, target::MegamorphicCache::mask_offset()));
   __ movq(RDI, FieldAddress(IC_DATA_REG,
                             target::MegamorphicCache::buckets_offset()));
-  // R9: mask as a smi.
+  // R9: mask as a smi - load first to support insertion w/o stopping Dart code.
   // RDI: cache buckets array.
 
   // Tag cid as a smi.
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
@@ -19759,24 +19759,9 @@ ObjectPtr MegamorphicCache::LookupLocked(const Smi& class_id) const {
   UNREACHABLE();
 }
 
+// Note this may run with Dart execution reading this cache.
 void MegamorphicCache::InsertLocked(const Smi& class_id,
                                     const Object& target) const {
-  auto isolate_group = IsolateGroup::Current();
-  ASSERT(isolate_group->type_feedback_mutex()->IsOwnedByCurrentThread());
-
-  // As opposed to ICData we are stopping mutator threads from other isolates
-  // while modifying the megamorphic cache, since updates are not atomic.
-  //
-  // NOTE: In the future we might change the megamorphic cache insertions to
-  // carefully use store-release barriers on the writer as well as
-  // load-acquire barriers on the reader, ...
-  isolate_group->RunWithStoppedMutators([&]() {
-    EnsureCapacityLocked();
-    InsertEntryLocked(class_id, target);
-  });
-}
-
-void MegamorphicCache::EnsureCapacityLocked() const {
   auto thread = Thread::Current();
   auto zone = thread->zone();
   auto isolate_group = thread->isolate_group();
@@ -19787,50 +19772,54 @@ void MegamorphicCache::EnsureCapacityLocked() const {
   if (static_cast<double>(filled_entry_count() + 1) > load_limit) {
     const Array& old_buckets = Array::Handle(zone, buckets());
     intptr_t new_capacity = old_capacity * 2;
+    intptr_t new_mask = new_capacity - 1;
     const Array& new_buckets =
         Array::Handle(zone, Array::New(kEntryLength * new_capacity));
 
     auto& target = Object::Handle(zone);
     for (intptr_t i = 0; i < new_capacity; ++i) {
       SetEntry(new_buckets, i, smi_illegal_cid(), target);
     }
-    set_buckets(new_buckets);
-    set_mask(new_capacity - 1);
-    set_filled_entry_count(0);
 
     // Rehash the valid entries.
     Smi& class_id = Smi::Handle(zone);
     for (intptr_t i = 0; i < old_capacity; ++i) {
       class_id ^= GetClassId(old_buckets, i);
       if (class_id.Value() != kIllegalCid) {
         target = GetTargetFunction(old_buckets, i);
-        InsertEntryLocked(class_id, target);
+        InsertEntryLocked<std::memory_order_relaxed>(new_buckets, new_mask,
+                                                     class_id, target);
       }
     }
+
+    // Publish buckets first. Old mask with new buckets is just a spurious miss.
+    untag()->set_buckets<std::memory_order_release>(new_buckets.ptr());
+    untag()->set_mask<std::memory_order_release>(Smi::New(new_mask));
   }
-}
 
-void MegamorphicCache::InsertEntryLocked(const Smi& class_id,
-                                         const Object& target) const {
-  auto thread = Thread::Current();
-  auto isolate_group = thread->isolate_group();
-  ASSERT(isolate_group->type_feedback_mutex()->IsOwnedByCurrentThread());
+  const Array& new_buckets = Array::Handle(zone, buckets());
+  InsertEntryLocked<std::memory_order_release>(new_buckets, mask(), class_id,
+                                               target);
+  set_filled_entry_count(filled_entry_count() + 1);
+}
 
-  ASSERT(Thread::Current()->IsDartMutatorThread());
-  ASSERT(static_cast<double>(filled_entry_count() + 1) <=
-         (kLoadFactor * static_cast<double>(mask() + 1)));
-  const Array& backing_array = Array::Handle(buckets());
-  intptr_t id_mask = mask();
-  intptr_t index = (class_id.Value() * kSpreadFactor) & id_mask;
-  intptr_t i = index;
+template <std::memory_order order>
+void MegamorphicCache::InsertEntryLocked(const Array& backing_array,
+                                         intptr_t mask,
+                                         const Smi& class_id,
+                                         const Object& target) {
+  const intptr_t start = (class_id.Value() * kSpreadFactor) & mask;
+  intptr_t i = start;
   do {
     if (Smi::Value(Smi::RawCast(GetClassId(backing_array, i))) == kIllegalCid) {
-      SetEntry(backing_array, i, class_id, target);
-      set_filled_entry_count(filled_entry_count() + 1);
+      // Publish target first. Old class id with new target is just a spurious
+      // miss.
+      SetTargetFunction<order>(backing_array, i, target);
+      SetClassId<order>(backing_array, i, class_id);
       return;
     }
-    i = (i + 1) & id_mask;
-  } while (i != index);
+    i = (i + 1) & mask;
+  } while (i != start);
   UNREACHABLE();
 }
 
diff --git a/runtime/vm/object.h b/runtime/vm/object.h
@@ -7838,18 +7838,29 @@ class MegamorphicCache : public CallSiteData {
 
   // The caller must hold IsolateGroup::type_feedback_mutex().
   void InsertLocked(const Smi& class_id, const Object& target) const;
-  void EnsureCapacityLocked() const;
   ObjectPtr LookupLocked(const Smi& class_id) const;
 
-  void InsertEntryLocked(const Smi& class_id, const Object& target) const;
+  template <std::memory_order order>
+  static void InsertEntryLocked(const Array& array,
+                                intptr_t mask,
+                                const Smi& class_id,
+                                const Object& target);
 
   static inline void SetEntry(const Array& array,
                               intptr_t index,
                               const Smi& class_id,
                               const Object& target);
 
   static inline ObjectPtr GetClassId(const Array& array, intptr_t index);
+  template <std::memory_order order>
+  static inline void SetClassId(const Array& array,
+                                intptr_t index,
+                                const Smi& class_id);
   static inline ObjectPtr GetTargetFunction(const Array& array, intptr_t index);
+  template <std::memory_order order>
+  static inline void SetTargetFunction(const Array& array,
+                                       intptr_t index,
+                                       const Object& target);
 
   FINAL_HEAP_OBJECT_IMPLEMENTATION(MegamorphicCache, CallSiteData);
 };
@@ -13549,11 +13560,25 @@ ObjectPtr MegamorphicCache::GetClassId(const Array& array, intptr_t index) {
   return array.At((index * kEntryLength) + kClassIdIndex);
 }
 
+template <std::memory_order order>
+void MegamorphicCache::SetClassId(const Array& array,
+                                  intptr_t index,
+                                  const Smi& class_id) {
+  array.SetAt<order>((index * kEntryLength) + kClassIdIndex, class_id);
+}
+
 ObjectPtr MegamorphicCache::GetTargetFunction(const Array& array,
                                               intptr_t index) {
   return array.At((index * kEntryLength) + kTargetFunctionIndex);
 }
 
+template <std::memory_order order>
+void MegamorphicCache::SetTargetFunction(const Array& array,
+                                         intptr_t index,
+                                         const Object& target) {
+  array.SetAt<order>((index * kEntryLength) + kTargetFunctionIndex, target);
+}
+
 inline uword AbstractType::Hash() const {
   ASSERT(IsFinalized());
   intptr_t result = Smi::Value(untag()->hash());
