From 40eaf81834052bdef7d6ccefbfac9dac7b265bc0 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Tue, 11 Feb 2020 15:45:33 +0000 Subject: [PATCH] [vm/concurrency] Remove redundant isolates list, ensure shutdown procedure waits until the isolates actually got deleted Issue https://github.com/dart-lang/sdk/issues/36097 Change-Id: If24affbb838eff8d80e5d448eac7455b3ffcb3a1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135062 Commit-Queue: Martin Kustermann Reviewed-by: Alexander Aprelev --- runtime/vm/dart.cc | 70 +++++++++------ runtime/vm/dart.h | 4 +- runtime/vm/isolate.cc | 161 ++++++++++++++++------------------ runtime/vm/isolate.h | 27 ++++-- runtime/vm/service_isolate.cc | 4 - 5 files changed, 141 insertions(+), 125 deletions(-) diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc index 1fc4d447393..f962acf06b7 100644 --- a/runtime/vm/dart.cc +++ b/runtime/vm/dart.cc @@ -246,12 +246,12 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot, // really an isolate itself - it acts more as a container for VM-global // objects. std::unique_ptr source( - new IsolateGroupSource(nullptr, "vm-isolate", vm_isolate_snapshot, + new IsolateGroupSource(nullptr, kVmIsolateName, vm_isolate_snapshot, instructions_snapshot, nullptr, -1, api_flags)); auto group = new IsolateGroup(std::move(source), /*embedder_data=*/nullptr); IsolateGroup::RegisterIsolateGroup(group); vm_isolate_ = - Isolate::InitIsolate("vm-isolate", group, api_flags, is_vm_isolate); + Isolate::InitIsolate(kVmIsolateName, group, api_flags, is_vm_isolate); group->set_initial_spawn_successful(); // Verify assumptions about executing in the VM isolate. @@ -405,31 +405,43 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot, return NULL; } -bool Dart::HasApplicationIsolateLocked() { - for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL; - isolate = isolate->next_) { - if (!Isolate::IsVMInternalIsolate(isolate)) return true; - } - return false; +static void DumpAliveIsolates(intptr_t num_attempts, + bool only_aplication_isolates) { + IsolateGroup::ForEach([&](IsolateGroup* group) { + group->ForEachIsolate([&](Isolate* isolate) { + if (!only_aplication_isolates || !Isolate::IsVMInternalIsolate(isolate)) { + OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n", + num_attempts, isolate->name()); + } + }); + }); +} + +static bool OnlyVmIsolateLeft() { + intptr_t count = 0; + bool found_vm_isolate = false; + IsolateGroup::ForEach([&](IsolateGroup* group) { + group->ForEachIsolate([&](Isolate* isolate) { + count++; + if (isolate == Dart::vm_isolate()) { + found_vm_isolate = true; + } + }); + }); + return count == 1 && found_vm_isolate; } // This waits until only the VM, service and kernel isolates are in the list. void Dart::WaitForApplicationIsolateShutdown() { ASSERT(!Isolate::creation_enabled_); - MonitorLocker ml(Isolate::isolates_list_monitor_); + MonitorLocker ml(Isolate::isolate_creation_monitor_); intptr_t num_attempts = 0; - while (HasApplicationIsolateLocked()) { + while (Isolate::application_isolates_count_ > 1) { Monitor::WaitResult retval = ml.Wait(1000); if (retval == Monitor::kTimedOut) { num_attempts += 1; if (num_attempts > 10) { - for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL; - isolate = isolate->next_) { - if (!Isolate::IsVMInternalIsolate(isolate)) { - OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n", - num_attempts, isolate->name_); - } - } + DumpAliveIsolates(num_attempts, /*only_application_isolates=*/true); } } } @@ -438,22 +450,19 @@ void Dart::WaitForApplicationIsolateShutdown() { // This waits until only the VM isolate remains in the list. void Dart::WaitForIsolateShutdown() { ASSERT(!Isolate::creation_enabled_); - MonitorLocker ml(Isolate::isolates_list_monitor_); + MonitorLocker ml(Isolate::isolate_creation_monitor_); intptr_t num_attempts = 0; - while ((Isolate::isolates_list_head_ != NULL) && - (Isolate::isolates_list_head_->next_ != NULL)) { + while (Isolate::total_isolates_count_ > 1) { Monitor::WaitResult retval = ml.Wait(1000); if (retval == Monitor::kTimedOut) { num_attempts += 1; if (num_attempts > 10) { - for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL; - isolate = isolate->next_) - OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n", - num_attempts, isolate->name_); + DumpAliveIsolates(num_attempts, /*only_application_isolates=*/false); } } } - ASSERT(Isolate::isolates_list_head_ == Dart::vm_isolate()); + + ASSERT(OnlyVmIsolateLeft()); } char* Dart::Cleanup() { @@ -940,11 +949,22 @@ void Dart::ShutdownIsolate(Isolate* isolate) { void Dart::ShutdownIsolate() { Isolate* isolate = Isolate::Current(); + const bool is_application_isolate = !Isolate::IsVMInternalIsolate(isolate); isolate->Shutdown(); if (KernelIsolate::IsKernelIsolate(isolate)) { KernelIsolate::SetKernelIsolate(NULL); } delete isolate; + + // Only now do we know for sure that the isolate and all it's resources have + // been deleted. So we can let any potential Dart::Cleanup() know it's safe to + // proceed shutdown of the VM. + Isolate::MarkIsolateDead(is_application_isolate); +} + +bool Dart::VmIsolateNameEquals(const char* name) { + ASSERT(name != NULL); + return (strcmp(name, kVmIsolateName) == 0); } int64_t Dart::UptimeMicros() { diff --git a/runtime/vm/dart.h b/runtime/vm/dart.h index 11e5c43ca98..0c7b6e8e630 100644 --- a/runtime/vm/dart.h +++ b/runtime/vm/dart.h @@ -66,6 +66,7 @@ class Dart : public AllStatic { static Isolate* vm_isolate() { return vm_isolate_; } static ThreadPool* thread_pool() { return thread_pool_; } + static bool VmIsolateNameEquals(const char* name); static int64_t UptimeMicros(); static int64_t UptimeMillis() { @@ -129,9 +130,10 @@ class Dart : public AllStatic { static bool non_nullable_flag() { return true; } private: + static constexpr const char* kVmIsolateName = "vm-isolate"; + static void WaitForIsolateShutdown(); static void WaitForApplicationIsolateShutdown(); - static bool HasApplicationIsolateLocked(); static Isolate* vm_isolate_; static int64_t start_time_micros_; diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 8fc58d9cb95..d933ba54fd5 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -1380,10 +1380,10 @@ void Isolate::InitVM() { shutdown_callback_ = nullptr; cleanup_callback_ = nullptr; cleanup_group_callback_ = nullptr; - if (isolates_list_monitor_ == nullptr) { - isolates_list_monitor_ = new Monitor(); + if (isolate_creation_monitor_ == nullptr) { + isolate_creation_monitor_ = new Monitor(); } - ASSERT(isolates_list_monitor_ != nullptr); + ASSERT(isolate_creation_monitor_ != nullptr); EnableIsolateCreation(); } @@ -1392,6 +1392,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix, const Dart_IsolateFlags& api_flags, bool is_vm_isolate) { Isolate* result = new Isolate(isolate_group, api_flags); + result->BuildName(name_prefix); ASSERT(result != nullptr); #if !defined(PRODUCT) @@ -1458,7 +1459,6 @@ Isolate* Isolate::InitIsolate(const char* name_prefix, result->set_pause_capability(result->random()->NextUInt64()); result->set_terminate_capability(result->random()->NextUInt64()); - result->BuildName(name_prefix); #if !defined(PRODUCT) result->debugger_ = new Debugger(result); #endif @@ -1476,7 +1476,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix, #endif // !PRODUCT // Add to isolate list. Shutdown and delete the isolate on failure. - if (!AddIsolateToList(result)) { + if (!TryMarkIsolateReady(result)) { result->LowLevelShutdown(); Thread::ExitIsolate(); if (KernelIsolate::IsKernelIsolate(result)) { @@ -2224,12 +2224,7 @@ void Isolate::Shutdown() { // Don't allow anymore dart code to execution on this isolate. thread->ClearStackLimit(); - // Remove this isolate from the list *before* we start tearing it down, to - // avoid exposing it in a state of decay. - RemoveIsolateFromList(this); - { - // After removal from isolate list. Before tearing down the heap. StackZone zone(thread); HandleScope handle_scope(thread); ServiceIsolate::SendIsolateShutdownMessage(); @@ -2265,6 +2260,7 @@ void Isolate::Shutdown() { #endif // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME) // Then, proceed with low-level teardown. + Isolate::UnMarkIsolateReady(this); LowLevelShutdown(); #if defined(DEBUG) @@ -2295,8 +2291,9 @@ Dart_IsolateShutdownCallback Isolate::shutdown_callback_ = nullptr; Dart_IsolateCleanupCallback Isolate::cleanup_callback_ = nullptr; Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr; -Monitor* Isolate::isolates_list_monitor_ = nullptr; -Isolate* Isolate::isolates_list_head_ = 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; @@ -3026,113 +3023,99 @@ void Isolate::VisitIsolates(IsolateVisitor* visitor) { if (visitor == nullptr) { return; } - // The visitor could potentially run code that could safepoint so use - // SafepointMonitorLocker to ensure the lock has safepoint checks. - SafepointMonitorLocker ml(isolates_list_monitor_); - Isolate* current = isolates_list_head_; - while (current != nullptr) { - visitor->VisitIsolate(current); - current = current->next_; - } + IsolateGroup::ForEach([&](IsolateGroup* group) { + group->ForEachIsolate( + [&](Isolate* isolate) { visitor->VisitIsolate(isolate); }); + }); } intptr_t Isolate::IsolateListLength() { - MonitorLocker ml(isolates_list_monitor_); - intptr_t count = 0; - Isolate* current = isolates_list_head_; - while (current != nullptr) { - count++; - current = current->next_; - } - return count; + MonitorLocker ml(isolate_creation_monitor_); + return total_isolates_count_; } Isolate* Isolate::LookupIsolateByPort(Dart_Port port) { - MonitorLocker ml(isolates_list_monitor_); - Isolate* current = isolates_list_head_; - while (current != nullptr) { - if (current->main_port() == port) { - return current; - } - current = current->next_; - } - return nullptr; + Isolate* match = nullptr; + IsolateGroup::ForEach([&](IsolateGroup* group) { + group->ForEachIsolate([&](Isolate* isolate) { + if (isolate->main_port() == port) { + match = isolate; + } + }); + }); + return match; } std::unique_ptr Isolate::LookupIsolateNameByPort(Dart_Port port) { - MonitorLocker ml(isolates_list_monitor_); - Isolate* current = isolates_list_head_; - while (current != nullptr) { - if (current->main_port() == port) { - const size_t len = strlen(current->name()) + 1; - auto result = std::unique_ptr(new char[len]); - strncpy(result.get(), current->name(), len); - return result; - } - current = current->next_; - } - return std::unique_ptr(); + MonitorLocker ml(isolate_creation_monitor_); + std::unique_ptr result; + IsolateGroup::ForEach([&](IsolateGroup* group) { + group->ForEachIsolate([&](Isolate* isolate) { + if (isolate->main_port() == port) { + const size_t len = strlen(isolate->name()) + 1; + result = std::unique_ptr(new char[len]); + strncpy(result.get(), isolate->name(), len); + } + }); + }); + return result; } -bool Isolate::AddIsolateToList(Isolate* isolate) { - MonitorLocker ml(isolates_list_monitor_); +bool Isolate::TryMarkIsolateReady(Isolate* isolate) { + MonitorLocker ml(isolate_creation_monitor_); if (!creation_enabled_) { return false; } - ASSERT(isolate != nullptr); - ASSERT(isolate->next_ == nullptr); - isolate->next_ = isolates_list_head_; - isolates_list_head_ = isolate; + total_isolates_count_++; + if (!Isolate::IsVMInternalIsolate(isolate)) { + application_isolates_count_++; + } + isolate->accepts_messages_ = true; return true; } -void Isolate::RemoveIsolateFromList(Isolate* isolate) { - MonitorLocker ml(isolates_list_monitor_); - ASSERT(isolate != nullptr); - if (isolate == isolates_list_head_) { - isolates_list_head_ = isolate->next_; - if (!creation_enabled_) { - ml.Notify(); - } - return; +void Isolate::UnMarkIsolateReady(Isolate* isolate) { + MonitorLocker ml(isolate_creation_monitor_); + ASSERT(total_isolates_count_ > 0); + 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_--; } - Isolate* previous = nullptr; - Isolate* current = isolates_list_head_; - while (current != nullptr) { - if (current == isolate) { - ASSERT(previous != nullptr); - previous->next_ = current->next_; - if (!creation_enabled_) { - ml.Notify(); - } - return; - } - previous = current; - current = current->next_; + if (!creation_enabled_) { + ml.Notify(); } - // If we are shutting down the VM, the isolate may not be in the list. - ASSERT(!creation_enabled_); } void Isolate::DisableIsolateCreation() { - MonitorLocker ml(isolates_list_monitor_); + MonitorLocker ml(isolate_creation_monitor_); creation_enabled_ = false; } void Isolate::EnableIsolateCreation() { - MonitorLocker ml(isolates_list_monitor_); + MonitorLocker ml(isolate_creation_monitor_); creation_enabled_ = true; } bool Isolate::IsolateCreationEnabled() { - MonitorLocker ml(isolates_list_monitor_); + MonitorLocker ml(isolate_creation_monitor_); return creation_enabled_; } bool Isolate::IsVMInternalIsolate(const Isolate* isolate) { - return (isolate == Dart::vm_isolate()) || - ServiceIsolate::IsServiceIsolateDescendant(isolate) || - KernelIsolate::IsKernelIsolate(isolate); + // 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()) || +#if !defined(DART_PRECOMPILED_RUNTIME) + KernelIsolate::NameEquals(isolate->name()) || +#endif + ServiceIsolate::NameEquals(isolate->name()); } void Isolate::KillLocked(LibMsgId msg_id) { @@ -3189,7 +3172,9 @@ class IsolateKillerVisitor : public IsolateVisitor { void VisitIsolate(Isolate* isolate) { ASSERT(isolate != nullptr); if (ShouldKill(isolate)) { - isolate->KillLocked(msg_id_); + if (isolate->AcceptsMessagesLocked()) { + isolate->KillLocked(msg_id_); + } } } @@ -3206,11 +3191,15 @@ class IsolateKillerVisitor : public IsolateVisitor { }; void Isolate::KillAllIsolates(LibMsgId msg_id) { + MonitorLocker ml(isolate_creation_monitor_); + IsolateKillerVisitor visitor(msg_id); VisitIsolates(&visitor); } void Isolate::KillIfExists(Isolate* isolate, LibMsgId msg_id) { + MonitorLocker ml(isolate_creation_monitor_); + IsolateKillerVisitor visitor(isolate, msg_id); VisitIsolates(&visitor); } diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 7a74bccabe1..e1e03029bcd 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -1087,7 +1087,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { const Dart_IsolateFlags& api_flags, bool is_vm_isolate = false); - // The isolates_list_monitor_ should be held when calling Kill(). + // The isolate_creation_monitor_ should be held when calling Kill(). void KillLocked(LibMsgId msg_id); void LowLevelShutdown(); @@ -1277,9 +1277,6 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { RawError* sticky_error_; - // Isolate list next pointer. - Isolate* next_ = nullptr; - // Protect access to boxed_field_list_. Mutex field_list_mutex_; // List of fields that became boxed and that trigger deoptimization. @@ -1303,6 +1300,11 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { std::unique_ptr forward_table_new_; std::unique_ptr forward_table_old_; + // Signals whether the isolate can receive messages (e.g. KillAllIsolates can + // send a kill message). + // This is protected by [isolate_creation_monitor_]. + bool accepts_messages_ = false; + static Dart_IsolateGroupCreateCallback create_group_callback_; static Dart_InitializeIsolateCallback initialize_callback_; static Dart_IsolateShutdownCallback shutdown_callback_; @@ -1314,12 +1316,19 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { #endif // Manage list of existing isolates. - static bool AddIsolateToList(Isolate* isolate); - static void RemoveIsolateFromList(Isolate* isolate); + static bool TryMarkIsolateReady(Isolate* isolate); + static void UnMarkIsolateReady(Isolate* isolate); + static void MarkIsolateDead(bool is_application_isolate); + bool AcceptsMessagesLocked() { + ASSERT(isolate_creation_monitor_->IsOwnedByCurrentThread()); + return accepts_messages_; + } - // This monitor protects isolates_list_head_, and creation_enabled_. - static Monitor* isolates_list_monitor_; - static Isolate* isolates_list_head_; + // This monitor protects application_isolates_count_, total_isolates_count_, + // 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) \ diff --git a/runtime/vm/service_isolate.cc b/runtime/vm/service_isolate.cc index 7198691ac4f..4817ab98efa 100644 --- a/runtime/vm/service_isolate.cc +++ b/runtime/vm/service_isolate.cc @@ -363,7 +363,6 @@ class RunServiceTask : public ThreadPool::Task { free(error); error = nullptr; - ServiceIsolate::SetServiceIsolate(NULL); ServiceIsolate::InitializingFailed(formatted_error); return; } @@ -419,9 +418,6 @@ class RunServiceTask : public ThreadPool::Task { } Dart::RunShutdownCallback(); } - ASSERT(ServiceIsolate::IsServiceIsolate(I)); - ServiceIsolate::SetServiceIsolate(NULL); - ServiceIsolate::SetServicePort(ILLEGAL_PORT); // Shut the isolate down. Dart::ShutdownIsolate(I);