From 83e460ab6ddf68874a39ffe9fbb6ebcaaeeaf7e0 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Wed, 2 Dec 2020 14:51:07 +0000 Subject: [PATCH] [vm/concurrency] Make ICData use new type_feeback_mutex to prevent duplicate entries Dart mutator threads cause update to runtime type feedback information (e.g. ICData, MegamorphicCache). With multiple lightweight isolates sharing JITed code, they will share the type feedback as well. That also means multiple mutators can try to update type feedback at the same time with the same (or different) feedback. This means we can run into situations where we're trying to update type feedback with information that's already there. => At a minimum we want to avoid adding duplicate entries to the ICData, so we have to guard against that. Since the actual backing store to ICData is immutable, we have two choices: * Let each thread read the current backing store, make a copy-on-write, add the new entry and update ICData with new backing store. => This can lead to an successful update being visible for a short period and then disappearing. * We can can ensure mutual exclusion when updating the backing store of ICData, thereby making read+grow+write appear atomic. => This requires taking a lock but ensures we never hit the same IC miss multiple times. => We take the latter approach for now. Issue https://github.com/dart-lang/sdk/issues/36097 TEST=CL is purely a refactoring. Change-Id: Ic3b2de61e25de0210323edd732424cc7d1178771 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174465 Commit-Queue: Martin Kustermann Reviewed-by: Vyacheslav Egorov --- runtime/vm/isolate.cc | 3 +++ runtime/vm/isolate.h | 5 ++-- runtime/vm/object.cc | 47 ++++++++++++++++++++++++++++++++++--- runtime/vm/object.h | 28 ++++++++++++++++++++++ runtime/vm/runtime_entry.cc | 30 ++++++++++------------- 5 files changed, 91 insertions(+), 22 deletions(-) diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 2fe5ead25d093..f8fa4cb19897b 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -359,6 +359,8 @@ IsolateGroup::IsolateGroup(std::shared_ptr source, NOT_IN_PRODUCT("IsolateGroup::type_canonicalization_mutex_")), type_arguments_canonicalization_mutex_(NOT_IN_PRODUCT( "IsolateGroup::type_arguments_canonicalization_mutex_")), + subtype_test_cache_mutex_( + NOT_IN_PRODUCT("IsolateGroup::subtype_test_cache_mutex_")), program_lock_(new SafepointRwLock()), active_mutators_monitor_(new Monitor()), max_active_mutators_(Scavenger::MaxMutatorThreadCount()) { @@ -1647,6 +1649,7 @@ Isolate::Isolate(IsolateGroup* isolate_group, constant_canonicalization_mutex_( NOT_IN_PRODUCT("Isolate::constant_canonicalization_mutex_")), megamorphic_mutex_(NOT_IN_PRODUCT("Isolate::megamorphic_mutex_")), + type_feedback_mutex_(NOT_IN_PRODUCT("Isolate::type_feedback_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 8407f4f50ce00..93eb508edcc92 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -965,6 +965,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { return &constant_canonicalization_mutex_; } Mutex* megamorphic_mutex() { return &megamorphic_mutex_; } + Mutex* type_feedback_mutex() { return &type_feedback_mutex_; } Mutex* kernel_data_lib_cache_mutex() { return &kernel_data_lib_cache_mutex_; } Mutex* kernel_data_class_cache_mutex() { @@ -1600,8 +1601,8 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { Simulator* simulator_ = nullptr; Mutex mutex_; // Protects compiler stats. Mutex constant_canonicalization_mutex_; // Protects const canonicalization. - Mutex megamorphic_mutex_; // Protects the table of megamorphic caches and - // their entries. + Mutex megamorphic_mutex_; + Mutex type_feedback_mutex_; Mutex kernel_data_lib_cache_mutex_; Mutex kernel_data_class_cache_mutex_; Mutex kernel_constants_mutex_; diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 160aa54f98397..125767591a458 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -15123,8 +15123,8 @@ bool ICData::HasCheck(const GrowableArray& cids) const { intptr_t ICData::FindCheck(const GrowableArray& cids) const { const intptr_t len = NumberOfChecks(); + GrowableArray class_ids; for (intptr_t i = 0; i < len; i++) { - GrowableArray class_ids; GetClassIdsAt(i, &class_ids); bool matches = true; for (intptr_t k = 0; k < class_ids.length(); k++) { @@ -15236,9 +15236,27 @@ bool ICData::ValidateInterceptor(const Function& target) const { return true; } +void ICData::EnsureHasCheck(const GrowableArray& class_ids, + const Function& target, + intptr_t count) const { + SafepointMutexLocker ml(Isolate::Current()->type_feedback_mutex()); + + if (FindCheck(class_ids) != -1) return; + AddCheckInternal(class_ids, target, count); +} + void ICData::AddCheck(const GrowableArray& class_ids, const Function& target, intptr_t count) const { + SafepointMutexLocker ml(Isolate::Current()->type_feedback_mutex()); + AddCheckInternal(class_ids, target, count); +} + +void ICData::AddCheckInternal(const GrowableArray& class_ids, + const Function& target, + intptr_t count) const { + ASSERT(Isolate::Current()->type_feedback_mutex()->IsOwnedByCurrentThread()); + ASSERT(!is_tracking_exactness()); ASSERT(!target.IsNull()); ASSERT((target.name() == target_name()) || ValidateInterceptor(target)); @@ -15321,10 +15339,32 @@ void ICData::DebugDump() const { } } +void ICData::EnsureHasReceiverCheck(intptr_t receiver_class_id, + const Function& target, + intptr_t count, + StaticTypeExactnessState exactness) const { + SafepointMutexLocker ml(Isolate::Current()->type_feedback_mutex()); + + GrowableArray class_ids(1); + class_ids.Add(receiver_class_id); + if (FindCheck(class_ids) != -1) return; + + AddReceiverCheckInternal(receiver_class_id, target, count, exactness); +} + void ICData::AddReceiverCheck(intptr_t receiver_class_id, const Function& target, intptr_t count, StaticTypeExactnessState exactness) const { + SafepointMutexLocker ml(Isolate::Current()->type_feedback_mutex()); + AddReceiverCheckInternal(receiver_class_id, target, count, exactness); +} + +void ICData::AddReceiverCheckInternal( + intptr_t receiver_class_id, + const Function& target, + intptr_t count, + StaticTypeExactnessState exactness) const { #if defined(DEBUG) GrowableArray class_ids(1); class_ids.Add(receiver_class_id); @@ -15566,8 +15606,9 @@ ICDataPtr ICData::AsUnaryClassChecksForArgNr(intptr_t arg_nr) const { result.IncrementCountAt(duplicate_class_id, count); } else { // This will make sure that Smi is first if it exists. - result.AddReceiverCheck(class_id, Function::Handle(GetTargetAt(i)), - count); + result.AddReceiverCheckInternal(class_id, + Function::Handle(GetTargetAt(i)), count, + StaticTypeExactnessState::NotTracking()); } } diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 3c70ead99c02b..5ca1a8c9abb4b 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -2088,6 +2088,15 @@ class ICData : public CallSiteData { // Adding checks. + // Ensures there is a check for [class_ids]. + // + // Calls [AddCheck] iff there is no existing check. Ensures test (and + // potential update) will be performed under exclusive lock to guard against + // multiple threads trying to add the same check. + void EnsureHasCheck(const GrowableArray& class_ids, + const Function& target, + intptr_t count = 1) const; + // Adds one more class test to ICData. Length of 'classes' must be equal to // the number of arguments tested. Use only for num_args_tested > 1. void AddCheck(const GrowableArray& class_ids, @@ -2096,6 +2105,18 @@ class ICData : public CallSiteData { StaticTypeExactnessState GetExactnessAt(intptr_t count) const; + // Ensures there is a receiver check for [receiver_class_id]. + // + // Calls [AddCheckReceiverCheck] iff there is no existing check. Ensures + // test (and potential update) will be performed under exclusive lock to + // guard against multiple threads trying to add the same check. + void EnsureHasReceiverCheck( + intptr_t receiver_class_id, + const Function& target, + intptr_t count = 1, + StaticTypeExactnessState exactness = + StaticTypeExactnessState::NotTracking()) const; + // Adds sorted so that Smi is the first class-id. Use only for // num_args_tested == 1. void AddReceiverCheck(intptr_t receiver_class_id, @@ -2262,6 +2283,13 @@ class ICData : public CallSiteData { intptr_t data_pos, intptr_t num_args_tested, const Function& target); + void AddCheckInternal(const GrowableArray& class_ids, + const Function& target, + intptr_t count) const; + void AddReceiverCheckInternal(intptr_t receiver_class_id, + const Function& target, + intptr_t count, + StaticTypeExactnessState exactness) const; // This bit is set when a call site becomes megamorphic and starts using a // MegamorphicCache instead of ICData. It means that the entries in the diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc index 8ef9463e40a11..134fa30cc8bef 100644 --- a/runtime/vm/runtime_entry.cc +++ b/runtime/vm/runtime_entry.cc @@ -1267,30 +1267,27 @@ static FunctionPtr InlineCacheMissHandlerGivenTargetFunction( const Instance& receiver = *args[0]; if (args.length() == 1) { + auto exactness = StaticTypeExactnessState::NotTracking(); if (ic_data.is_tracking_exactness()) { #if !defined(DART_PRECOMPILED_RUNTIME) - const auto state = receiver.IsNull() - ? StaticTypeExactnessState::NotExact() - : StaticTypeExactnessState::Compute( - Type::Cast(AbstractType::Handle( - ic_data.receivers_static_type())), - receiver); - ic_data.AddReceiverCheck( - receiver.GetClassId(), target_function, count, - /*exactness=*/state.CollapseSuperTypeExactness()); + exactness = receiver.IsNull() ? StaticTypeExactnessState::NotExact() + : StaticTypeExactnessState::Compute( + Type::Cast(AbstractType::Handle( + ic_data.receivers_static_type())), + receiver); #else UNREACHABLE(); #endif - } else { - ic_data.AddReceiverCheck(args[0]->GetClassId(), target_function, count); } + ic_data.EnsureHasReceiverCheck(receiver.GetClassId(), target_function, + count, exactness); } else { GrowableArray class_ids(args.length()); ASSERT(ic_data.NumArgsTested() == args.length()); for (intptr_t i = 0; i < args.length(); i++) { class_ids.Add(args[i]->GetClassId()); } - ic_data.AddCheck(class_ids, target_function, count); + ic_data.EnsureHasCheck(class_ids, target_function, count); } if (FLAG_trace_ic_miss_in_optimized || FLAG_trace_ic) { DartFrameIterator iterator(Thread::Current(), @@ -1322,8 +1319,7 @@ static FunctionPtr InlineCacheMissHandlerGivenTargetFunction( static FunctionPtr InlineCacheMissHandler( const GrowableArray& args, // Checked arguments only. - const ICData& ic_data, - intptr_t count = 1) { + const ICData& ic_data) { Thread* thread = Thread::Current(); Zone* zone = thread->zone(); @@ -1365,7 +1361,7 @@ static FunctionPtr InlineCacheMissHandler( return target_function.raw(); } - return InlineCacheMissHandlerGivenTargetFunction(args, ic_data, count, + return InlineCacheMissHandlerGivenTargetFunction(args, ic_data, 1, target_function); } @@ -1416,7 +1412,7 @@ DEFINE_RUNTIME_ENTRY(StaticCallMissHandlerOneArg, 2) { const Function& target = Function::Handle(zone, ic_data.GetTargetAt(0)); target.EnsureHasCode(); ASSERT(!target.IsNull() && target.HasCode()); - ic_data.AddReceiverCheck(arg.GetClassId(), target, 1); + ic_data.EnsureHasReceiverCheck(arg.GetClassId(), target, 1); if (FLAG_trace_ic) { DartFrameIterator iterator(thread, StackFrameIterator::kNoCrossThreadIteration); @@ -1444,7 +1440,7 @@ DEFINE_RUNTIME_ENTRY(StaticCallMissHandlerTwoArgs, 3) { GrowableArray cids(2); cids.Add(arg0.GetClassId()); cids.Add(arg1.GetClassId()); - ic_data.AddCheck(cids, target); + ic_data.EnsureHasCheck(cids, target); if (FLAG_trace_ic) { DartFrameIterator iterator(thread, StackFrameIterator::kNoCrossThreadIteration);