From 112dbabc105e85133abe127ebcd4fe86fdca5c7c Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Thu, 13 Jun 2019 21:13:15 +0000 Subject: [PATCH] [vm] Update Megamorphic::filled_entry_count_ under the megamorphic lock. Otherwise the background compiler may see 0 when the mutator grows the megamorphic cache. Bug: https://github.com/dart-lang/sdk/issues/37257 Change-Id: I64a31937391ad6c0f086f8f175501ca4ef06c305 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105969 Commit-Queue: Ryan Macnak Reviewed-by: Alexander Markov --- runtime/vm/compiler/backend/il.cc | 6 ++++-- runtime/vm/isolate.cc | 3 +-- runtime/vm/isolate.h | 5 +++-- runtime/vm/megamorphic_cache_table.cc | 2 +- runtime/vm/object.cc | 19 ++++++++++++++----- runtime/vm/object.h | 6 ++++-- runtime/vm/runtime_entry.cc | 1 - 7 files changed, 27 insertions(+), 15 deletions(-) diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index 932b94e6d6c..17e1b33b3df 100644 --- 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(); + 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 index e68d40c36cc..544424a55c1 100644 --- 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 index 4813d27ba33..6e8cb9e9a82 100644 --- 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 index c07452fd000..8b9bf6a2d6e 100644 --- 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 index ffea8e7938a..4294af99914 100644 --- 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(old_capacity); if (static_cast(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(filled_entry_count() + 1) <= (kLoadFactor * static_cast(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 index c1cc1a46780..a5a57315139 100644 --- 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 index c2257d03812..f13950a06c9 100644 --- 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); }