Skip to content

Commit

Permalink
[vm/concurrency] Make ICData use new type_feeback_mutex to prevent du…
Browse files Browse the repository at this point in the history 
…plicate 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 dart-lang/sdk#36097

TEST=CL is purely a refactoring.

Change-Id: Ic3b2de61e25de0210323edd732424cc7d1178771
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174465
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
  • Loading branch information
@mkustermann
mkustermann authored and commit-bot@chromium.org committed Dec 2, 2020
1 parent 89811d9 commit 83e460a
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 22 deletions.
3 changes: 3 additions & 0 deletions runtime/vm/isolate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,8 @@ IsolateGroup::IsolateGroup(std::shared_ptr<IsolateGroupSource> 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()) {
Expand Down Expand Up @@ -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_(
Expand Down
5 changes: 3 additions & 2 deletions runtime/vm/isolate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
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() {
Expand Down Expand Up @@ -1600,8 +1601,8 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
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_;
Expand Down
47 changes: 44 additions & 3 deletions runtime/vm/object.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15123,8 +15123,8 @@ bool ICData::HasCheck(const GrowableArray<intptr_t>& cids) const {

intptr_t ICData::FindCheck(const GrowableArray<intptr_t>& cids) const {
const intptr_t len = NumberOfChecks();
GrowableArray<intptr_t> class_ids;
for (intptr_t i = 0; i < len; i++) {
GrowableArray<intptr_t> class_ids;
GetClassIdsAt(i, &class_ids);
bool matches = true;
for (intptr_t k = 0; k < class_ids.length(); k++) {
Expand Down Expand Up @@ -15236,9 +15236,27 @@ bool ICData::ValidateInterceptor(const Function& target) const {
return true;
}

void ICData::EnsureHasCheck(const GrowableArray<intptr_t>& 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<intptr_t>& 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<intptr_t>& 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));
Expand Down Expand Up @@ -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<intptr_t> 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<intptr_t> class_ids(1);
class_ids.Add(receiver_class_id);
Expand Down Expand Up @@ -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());
}
}

Expand Down
28 changes: 28 additions & 0 deletions runtime/vm/object.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<intptr_t>& 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<intptr_t>& class_ids,
Expand All @@ -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,
Expand Down Expand Up @@ -2262,6 +2283,13 @@ class ICData : public CallSiteData {
intptr_t data_pos,
intptr_t num_args_tested,
const Function& target);
void AddCheckInternal(const GrowableArray<intptr_t>& 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
Expand Down
30 changes: 13 additions & 17 deletions runtime/vm/runtime_entry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<intptr_t> 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(),
Expand Down Expand Up @@ -1322,8 +1319,7 @@ static FunctionPtr InlineCacheMissHandlerGivenTargetFunction(

static FunctionPtr InlineCacheMissHandler(
const GrowableArray<const Instance*>& args, // Checked arguments only.
const ICData& ic_data,
intptr_t count = 1) {
const ICData& ic_data) {
Thread* thread = Thread::Current();
Zone* zone = thread->zone();

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1444,7 +1440,7 @@ DEFINE_RUNTIME_ENTRY(StaticCallMissHandlerTwoArgs, 3) {
GrowableArray<intptr_t> 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);
Expand Down

0 comments on commit 83e460a

Please sign in to comment.