[vm] Update Megamorphic::filled_entry_count_ under the megamorphic lock.

rmacnak-google · commit-bot@chromium.org · commit 112dbabc105e · 2019-06-13T21:13:15.000Z
Otherwise the background compiler may see 0 when the mutator grows the megamorphic cache. Bug: #37257 Change-Id: I64a31937391ad6c0f086f8f175501ca4ef06c305 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105969 Commit-Queue: Ryan Macnak <rmacnak@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
@@ -776,7 +776,7 @@ void Cids::CreateHelper(Zone* zone,
   if (ic_data.is_megamorphic()) {
     const MegamorphicCache& cache =
         MegamorphicCache::Handle(zone, ic_data.AsMegamorphicCache());
-    SafepointMutexLocker ml(Isolate::Current()->megamorphic_lookup_mutex());
+    SafepointMutexLocker ml(Isolate::Current()->megamorphic_mutex());
     MegamorphicCacheEntries entries(Array::Handle(zone, cache.buckets()));
     for (intptr_t i = 0; i < entries.Length(); i++) {
       const intptr_t id =
@@ -787,8 +787,10 @@ void Cids::CreateHelper(Zone* zone,
       if (include_targets) {
         Function& function = Function::ZoneHandle(zone);
         function ^= entries[i].Get<MegamorphicCache::kTargetFunctionIndex>();
+        const intptr_t filled_entry_count = cache.filled_entry_count();
+        ASSERT(filled_entry_count > 0);
         cid_ranges_.Add(new (zone) TargetInfo(
-            id, id, &function, Usage(function) / cache.filled_entry_count(),
+            id, id, &function, Usage(function) / filled_entry_count,
             StaticTypeExactnessState::NotTracking()));
       } else {
         cid_ranges_.Add(new (zone) CidRange(id, id));
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
@@ -892,8 +892,7 @@ Isolate::Isolate(const Dart_IsolateFlags& api_flags)
           NOT_IN_PRODUCT("Isolate::type_canonicalization_mutex_")),
       constant_canonicalization_mutex_(
           NOT_IN_PRODUCT("Isolate::constant_canonicalization_mutex_")),
-      megamorphic_lookup_mutex_(
-          NOT_IN_PRODUCT("Isolate::megamorphic_lookup_mutex_")),
+      megamorphic_mutex_(NOT_IN_PRODUCT("Isolate::megamorphic_mutex_")),
       kernel_data_lib_cache_mutex_(
           NOT_IN_PRODUCT("Isolate::kernel_data_lib_cache_mutex_")),
       kernel_data_class_cache_mutex_(
diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
@@ -355,7 +355,7 @@ class Isolate : public BaseIsolate {
   Mutex* constant_canonicalization_mutex() {
     return &constant_canonicalization_mutex_;
   }
-  Mutex* megamorphic_lookup_mutex() { return &megamorphic_lookup_mutex_; }
+  Mutex* megamorphic_mutex() { return &megamorphic_mutex_; }
 
   Mutex* kernel_data_lib_cache_mutex() { return &kernel_data_lib_cache_mutex_; }
   Mutex* kernel_data_class_cache_mutex() {
@@ -1027,7 +1027,8 @@ class Isolate : public BaseIsolate {
   Mutex symbols_mutex_;  // Protects concurrent access to the symbol table.
   Mutex type_canonicalization_mutex_;      // Protects type canonicalization.
   Mutex constant_canonicalization_mutex_;  // Protects const canonicalization.
-  Mutex megamorphic_lookup_mutex_;         // Protects megamorphic table lookup.
+  Mutex megamorphic_mutex_;  // Protects the table of megamorphic caches and
+                             // their entries.
   Mutex kernel_data_lib_cache_mutex_;
   Mutex kernel_data_class_cache_mutex_;
   Mutex kernel_constants_mutex_;
diff --git a/runtime/vm/megamorphic_cache_table.cc b/runtime/vm/megamorphic_cache_table.cc
@@ -16,7 +16,7 @@ RawMegamorphicCache* MegamorphicCacheTable::Lookup(Isolate* isolate,
                                                    const String& name,
                                                    const Array& descriptor) {
   // Multiple compilation threads could access this lookup.
-  SafepointMutexLocker ml(isolate->megamorphic_lookup_mutex());
+  SafepointMutexLocker ml(isolate->megamorphic_mutex());
   ASSERT(name.IsSymbol());
   // TODO(rmacnak): ASSERT(descriptor.IsCanonical());
 
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
@@ -14175,7 +14175,7 @@ bool ICData::HasOneTarget() const {
   if (is_megamorphic()) {
     const MegamorphicCache& cache =
         MegamorphicCache::Handle(AsMegamorphicCache());
-    SafepointMutexLocker ml(Isolate::Current()->megamorphic_lookup_mutex());
+    SafepointMutexLocker ml(Isolate::Current()->megamorphic_mutex());
     MegamorphicCacheEntries entries(Array::Handle(cache.buckets()));
     for (intptr_t i = 0; i < entries.Length(); i++) {
       const intptr_t id =
@@ -15672,7 +15672,14 @@ RawMegamorphicCache* MegamorphicCache::New(const String& target_name,
   return result.raw();
 }
 
-void MegamorphicCache::EnsureCapacity() const {
+void MegamorphicCache::Insert(const Smi& class_id, const Object& target) const {
+  SafepointMutexLocker ml(Isolate::Current()->megamorphic_mutex());
+  EnsureCapacityLocked();
+  InsertLocked(class_id, target);
+}
+
+void MegamorphicCache::EnsureCapacityLocked() const {
+  ASSERT(Isolate::Current()->megamorphic_mutex()->IsOwnedByCurrentThread());
   intptr_t old_capacity = mask() + 1;
   double load_limit = kLoadFactor * static_cast<double>(old_capacity);
   if (static_cast<double>(filled_entry_count() + 1) > load_limit) {
@@ -15696,16 +15703,18 @@ void MegamorphicCache::EnsureCapacity() const {
       class_id ^= GetClassId(old_buckets, i);
       if (class_id.Value() != kIllegalCid) {
         target = GetTargetFunction(old_buckets, i);
-        Insert(class_id, target);
+        InsertLocked(class_id, target);
       }
     }
   }
 }
 
-void MegamorphicCache::Insert(const Smi& class_id, const Object& target) const {
+void MegamorphicCache::InsertLocked(const Smi& class_id,
+                                    const Object& target) const {
+  ASSERT(Isolate::Current()->megamorphic_mutex()->IsOwnedByCurrentThread());
+  ASSERT(Thread::Current()->IsMutatorThread());
   ASSERT(static_cast<double>(filled_entry_count() + 1) <=
          (kLoadFactor * static_cast<double>(mask() + 1)));
-  SafepointMutexLocker ml(Isolate::Current()->megamorphic_lookup_mutex());
   const Array& backing_array = Array::Handle(buckets());
   intptr_t id_mask = mask();
   intptr_t index = (class_id.Value() * kSpreadFactor) & id_mask;
diff --git a/runtime/vm/object.h b/runtime/vm/object.h
@@ -5836,8 +5836,6 @@ class MegamorphicCache : public Object {
   static RawMegamorphicCache* New(const String& target_name,
                                   const Array& arguments_descriptor);
 
-  void EnsureCapacity() const;
-
   void Insert(const Smi& class_id, const Object& target) const;
 
   void SwitchToBareInstructions();
@@ -5856,6 +5854,10 @@ class MegamorphicCache : public Object {
   void set_target_name(const String& value) const;
   void set_arguments_descriptor(const Array& value) const;
 
+  // The caller must hold Isolate::megamorphic_mutex().
+  void EnsureCapacityLocked() const;
+  void InsertLocked(const Smi& class_id, const Object& target) const;
+
   static inline void SetEntry(const Array& array,
                               intptr_t index,
                               const Smi& class_id,
diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc
@@ -1786,7 +1786,6 @@ DEFINE_RUNTIME_ENTRY(MegamorphicCacheMissHandler, 3) {
   } else {
     const MegamorphicCache& cache = MegamorphicCache::Cast(ic_data_or_cache);
     // Insert function found into cache and return it.
-    cache.EnsureCapacity();
     const Smi& class_id = Smi::Handle(zone, Smi::New(cls.id()));
     cache.Insert(class_id, target_function);
   }

Original file line number	Diff line number	Diff line change
`@@ -1786,7 +1786,6 @@ DEFINE_RUNTIME_ENTRY(MegamorphicCacheMissHandler, 3) {`
`1786`	`1786`	`} else {`
`1787`	`1787`	`const MegamorphicCache& cache = MegamorphicCache::Cast(ic_data_or_cache);`
`1788`	`1788`	`// Insert function found into cache and return it.`
`1789`		`- cache.EnsureCapacity();`
`1790`	`1789`	`const Smi& class_id = Smi::Handle(zone, Smi::New(cls.id()));`
`1791`	`1790`	`cache.Insert(class_id, target_function);`
`1792`	`1791`	`}`