[vm/concurrency] Delay notification of Dart_Cleanup() until isolate group has died

It also avoids acquiring a lock when the isolate shuts down but the
group is not empty yet.

Issue https://github.com/dart-lang/sdk/issues/36097

Fixes b/156067624

Change-Id: I3b58e9ed24ea97c0cd796bd8a386d06075bfc0e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/147960
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
This commit is contained in:
Martin Kustermann 2020-05-14 10:35:11 +00:00 committed by commit-bot@chromium.org
parent 22f09f16b4
commit c4acfb506f
3 changed files with 52 additions and 40 deletions

View file

@ -437,7 +437,7 @@ void Dart::WaitForApplicationIsolateShutdown() {
ASSERT(!Isolate::creation_enabled_);
MonitorLocker ml(Isolate::isolate_creation_monitor_);
intptr_t num_attempts = 0;
while (Isolate::application_isolates_count_ > 0) {
while (IsolateGroup::HasApplicationIsolateGroups()) {
Monitor::WaitResult retval = ml.Wait(1000);
if (retval == Monitor::kTimedOut) {
num_attempts += 1;
@ -453,7 +453,7 @@ void Dart::WaitForIsolateShutdown() {
ASSERT(!Isolate::creation_enabled_);
MonitorLocker ml(Isolate::isolate_creation_monitor_);
intptr_t num_attempts = 0;
while (Isolate::total_isolates_count_ > 1) {
while (!IsolateGroup::HasOnlyVMIsolateGroup()) {
Monitor::WaitResult retval = ml.Wait(1000);
if (retval == Monitor::kTimedOut) {
num_attempts += 1;

View file

@ -423,6 +423,16 @@ void IsolateGroup::Shutdown() {
}
delete this;
// After this isolate group has died we might need to notify a pending
// `Dart_Cleanup()` call.
{
MonitorLocker ml(Isolate::isolate_creation_monitor_);
if (!Isolate::creation_enabled_ &&
!IsolateGroup::HasApplicationIsolateGroups()) {
ml.Notify();
}
}
}
void IsolateGroup::set_heap(std::unique_ptr<Heap> heap) {
@ -665,6 +675,26 @@ void IsolateGroup::UnregisterIsolateGroup(IsolateGroup* isolate_group) {
isolate_groups_->Remove(isolate_group);
}
bool IsolateGroup::HasApplicationIsolateGroups() {
ReadRwLocker wl(ThreadState::Current(), isolate_groups_rwlock_);
for (auto group : *isolate_groups_) {
if (!IsolateGroup::IsVMInternalIsolate(group)) {
return true;
}
}
return false;
}
bool IsolateGroup::HasOnlyVMIsolateGroup() {
ReadRwLocker wl(ThreadState::Current(), isolate_groups_rwlock_);
for (auto group : *isolate_groups_) {
if (!Dart::VmIsolateNameEquals(group->source()->name)) {
return false;
}
}
return true;
}
void IsolateGroup::Init() {
ASSERT(isolate_groups_rwlock_ == nullptr);
isolate_groups_rwlock_ = new RwLock();
@ -2479,7 +2509,6 @@ void Isolate::Shutdown() {
}
void Isolate::LowLevelCleanup(Isolate* isolate) {
const bool is_application_isolate = !Isolate::IsVMInternalIsolate(isolate);
#if !defined(DART_PECOMPILED_RUNTIME)
if (KernelIsolate::IsKernelIsolate(isolate)) {
KernelIsolate::SetKernelIsolate(nullptr);
@ -2540,12 +2569,6 @@ void Isolate::LowLevelCleanup(Isolate* isolate) {
// inform the GC about this situation.
}
}
// After deleting the isolate we know that all it's resources have been freed.
// We still delay the notification to a possible call to `Dart::Cleanup()` to
// after a potential shutdown of the group, which would turn down any pending
// GC tasks as well as the heap.
Isolate::MarkIsolateDead(is_application_isolate);
} // namespace dart
Dart_InitializeIsolateCallback Isolate::initialize_callback_ = nullptr;
@ -2556,8 +2579,6 @@ Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr;
Random* IsolateGroup::isolate_group_random_ = nullptr;
Monitor* Isolate::isolate_creation_monitor_ = nullptr;
intptr_t Isolate::application_isolates_count_ = 0;
intptr_t Isolate::total_isolates_count_ = 0;
bool Isolate::creation_enabled_ = false;
RwLock* IsolateGroup::isolate_groups_rwlock_ = nullptr;
@ -3410,8 +3431,11 @@ void Isolate::VisitIsolates(IsolateVisitor* visitor) {
}
intptr_t Isolate::IsolateListLength() {
MonitorLocker ml(isolate_creation_monitor_);
return total_isolates_count_;
intptr_t count = 0;
IsolateGroup::ForEach([&](IsolateGroup* group) {
group->ForEachIsolate([&](Isolate* isolate) { count++; });
});
return count;
}
Isolate* Isolate::LookupIsolateByPort(Dart_Port port) {
@ -3443,10 +3467,6 @@ std::unique_ptr<char[]> Isolate::LookupIsolateNameByPort(Dart_Port port) {
bool Isolate::TryMarkIsolateReady(Isolate* isolate) {
MonitorLocker ml(isolate_creation_monitor_);
total_isolates_count_++;
if (!Isolate::IsVMInternalIsolate(isolate)) {
application_isolates_count_++;
}
if (!creation_enabled_) {
return false;
}
@ -3459,19 +3479,6 @@ void Isolate::UnMarkIsolateReady(Isolate* isolate) {
isolate->accepts_messages_ = false;
}
void Isolate::MarkIsolateDead(bool is_application_isolate) {
MonitorLocker ml(isolate_creation_monitor_);
ASSERT(total_isolates_count_ > 0);
total_isolates_count_--;
if (is_application_isolate) {
ASSERT(application_isolates_count_ > 0);
application_isolates_count_--;
}
if (!creation_enabled_) {
ml.Notify();
}
}
void Isolate::DisableIsolateCreation() {
MonitorLocker ml(isolate_creation_monitor_);
creation_enabled_ = false;
@ -3487,14 +3494,15 @@ bool Isolate::IsolateCreationEnabled() {
return creation_enabled_;
}
bool Isolate::IsVMInternalIsolate(const Isolate* isolate) {
bool IsolateGroup::IsVMInternalIsolate(const IsolateGroup* group) {
// We use a name comparison here because this method can be called during
// shutdown, where the actual isolate pointers might've already been cleared.
return Dart::VmIsolateNameEquals(isolate->name()) ||
const char* name = group->source()->name;
return Dart::VmIsolateNameEquals(name) ||
#if !defined(DART_PRECOMPILED_RUNTIME)
KernelIsolate::NameEquals(isolate->name()) ||
KernelIsolate::NameEquals(name) ||
#endif
ServiceIsolate::NameEquals(isolate->name());
ServiceIsolate::NameEquals(name);
}
void Isolate::KillLocked(LibMsgId msg_id) {

View file

@ -309,6 +309,7 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
IdleTimeHandler* idle_time_handler() { return &idle_time_handler_; }
// Returns true if this is the first isolate registered.
void RegisterIsolate(Isolate* isolate);
void RegisterIsolateLocked(Isolate* isolate);
void UnregisterIsolate(Isolate* isolate);
@ -489,6 +490,10 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
static void RegisterIsolateGroup(IsolateGroup* isolate_group);
static void UnregisterIsolateGroup(IsolateGroup* isolate_group);
static bool HasApplicationIsolateGroups();
static bool HasOnlyVMIsolateGroup();
static bool IsVMInternalIsolate(const IsolateGroup* group);
int64_t UptimeMicros() const;
ApiState* api_state() const { return api_state_.get(); }
@ -1244,7 +1249,9 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
static void DisableIsolateCreation();
static void EnableIsolateCreation();
static bool IsolateCreationEnabled();
static bool IsVMInternalIsolate(const Isolate* isolate);
static bool IsVMInternalIsolate(const Isolate* isolate) {
return IsolateGroup::IsVMInternalIsolate(isolate->group());
}
#if !defined(PRODUCT)
intptr_t reload_every_n_stack_overflow_checks() const {
@ -1522,17 +1529,14 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
// Manage list of existing isolates.
static bool TryMarkIsolateReady(Isolate* isolate);
static void UnMarkIsolateReady(Isolate* isolate);
static void MarkIsolateDead(bool is_application_isolate);
static void MaybeNotifyVMShutdown();
bool AcceptsMessagesLocked() {
ASSERT(isolate_creation_monitor_->IsOwnedByCurrentThread());
return accepts_messages_;
}
// This monitor protects application_isolates_count_, total_isolates_count_,
// creation_enabled_.
// This monitor protects [creation_enabled_].
static Monitor* isolate_creation_monitor_;
static intptr_t application_isolates_count_;
static intptr_t total_isolates_count_;
static bool creation_enabled_;
#define REUSABLE_FRIEND_DECLARATION(name) \