From 14ea27e65b5be85efe8b01622316640c2625d445 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Thu, 11 Jul 2019 13:36:02 +0000 Subject: [PATCH] [vm/concurrency] Make thread_registry not depend on [Isolate], move mutator thread from thread registry to isolate The thread registry is creating [Thread] objects when threads enter an isolate (as mutator or helper). Once threads exit an isolate, the [Thread] structure is returned and the thread registry will put it into a cache for later re-use. The mutator [Thread] object is an exception: Exiting and entering the isolate as mutator will always use the same cached [Thread] object. We want to eventually use one thread registry for an entire isolate group. There will therefore be multiple mutator threads per thread registry. We therefore move the cached mutator thread from the thread registry to the [Isolate] object. Issue https://github.com/dart-lang/sdk/issues/36097 Change-Id: Id27dff886d79ca76f6e05320151aeb72c8ba5140 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108720 Commit-Queue: Martin Kustermann Reviewed-by: Ryan Macnak --- runtime/vm/base_isolate.h | 20 ++++- .../vm/compiler/runtime_offsets_extracted.h | 84 +++++++++---------- runtime/vm/isolate.cc | 52 ++++++++++-- runtime/vm/thread.cc | 4 +- runtime/vm/thread.h | 2 +- runtime/vm/thread_registry.cc | 46 +++------- runtime/vm/thread_registry.h | 30 +------ 7 files changed, 122 insertions(+), 116 deletions(-) diff --git a/runtime/vm/base_isolate.h b/runtime/vm/base_isolate.h index 58a0cbbb8e12..be59b1667c89 100644 --- a/runtime/vm/base_isolate.h +++ b/runtime/vm/base_isolate.h @@ -31,13 +31,29 @@ class BaseIsolate { #endif protected: - BaseIsolate() : scheduled_mutator_thread_(NULL) {} + BaseIsolate() {} ~BaseIsolate() { // Do not delete stack resources: top_resource_ and current_zone_. } - Thread* scheduled_mutator_thread_; + Thread* scheduled_mutator_thread_ = nullptr; + + // TODO(asiva): Currently we treat a mutator thread as a special thread + // and always schedule execution of Dart code on the same mutator thread + // object. The ApiLocalScope has been made thread specific but we still + // have scenarios where we do a temporary exit of an Isolate with live + // zones/handles in the API scope : + // - Dart_RunLoop() + // - IsolateSaver in Dart_NewNativePort + // Similarly, tracking async_stack_trace requires that we always reschedule + // on the same thread. + // We probably need a mechanism to return to the specific thread only + // for these specific cases. We should also determine if the embedder + // should allow exiting an isolate with live state in zones/handles in + // which case a new API for returning to the specific thread needs to be + // added. + Thread* mutator_thread_ = nullptr; private: DISALLOW_COPY_AND_ASSIGN(BaseIsolate); diff --git a/runtime/vm/compiler/runtime_offsets_extracted.h b/runtime/vm/compiler/runtime_offsets_extracted.h index dc10a4126878..c9d3300fcdb3 100644 --- a/runtime/vm/compiler/runtime_offsets_extracted.h +++ b/runtime/vm/compiler/runtime_offsets_extracted.h @@ -138,13 +138,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 20; static constexpr dart::compiler::target::word ICData_state_bits_offset = 28; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 16; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 32; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 16; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 20; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 24; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 28; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 52; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 12; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 36; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 20; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 28; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 32; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 56; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 24; @@ -491,13 +491,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 40; static constexpr dart::compiler::target::word ICData_state_bits_offset = 52; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 32; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 64; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 32; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 40; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 48; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 56; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 104; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 72; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 40; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 48; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 56; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 64; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 112; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 48; @@ -845,13 +845,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 20; static constexpr dart::compiler::target::word ICData_state_bits_offset = 28; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 16; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 32; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 16; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 20; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 24; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 28; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 52; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 12; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 36; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 20; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 28; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 32; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 56; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 24; @@ -1194,13 +1194,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 40; static constexpr dart::compiler::target::word ICData_state_bits_offset = 52; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 32; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 64; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 32; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 40; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 48; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 56; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 104; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 72; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 40; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 48; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 56; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 64; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 112; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 48; @@ -1551,13 +1551,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 40; static constexpr dart::compiler::target::word ICData_state_bits_offset = 52; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 32; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 64; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 32; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 40; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 48; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 56; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 104; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 72; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 40; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 48; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 56; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 64; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 112; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 32; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 48; @@ -1835,13 +1835,13 @@ static constexpr dart::compiler::target::word ICData_owner_offset = 20; static constexpr dart::compiler::target::word ICData_state_bits_offset = 28; static constexpr dart::compiler::target::word ICData_receivers_static_type_offset = 16; -static constexpr dart::compiler::target::word Isolate_class_table_offset = 32; -static constexpr dart::compiler::target::word Isolate_current_tag_offset = 16; -static constexpr dart::compiler::target::word Isolate_default_tag_offset = 20; -static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 24; -static constexpr dart::compiler::target::word Isolate_object_store_offset = 28; -static constexpr dart::compiler::target::word Isolate_single_step_offset = 52; -static constexpr dart::compiler::target::word Isolate_user_tag_offset = 12; +static constexpr dart::compiler::target::word Isolate_class_table_offset = 36; +static constexpr dart::compiler::target::word Isolate_current_tag_offset = 20; +static constexpr dart::compiler::target::word Isolate_default_tag_offset = 24; +static constexpr dart::compiler::target::word Isolate_ic_miss_code_offset = 28; +static constexpr dart::compiler::target::word Isolate_object_store_offset = 32; +static constexpr dart::compiler::target::word Isolate_single_step_offset = 56; +static constexpr dart::compiler::target::word Isolate_user_tag_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_data_offset = 16; static constexpr dart::compiler::target::word LinkedHashMap_deleted_keys_offset = 24; diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 52119b78decc..21ba06792792 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -1021,6 +1021,13 @@ Isolate::~Isolate() { nullptr); // No deopt in progress when isolate deleted. ASSERT(spawn_count_ == 0); delete safepoint_handler_; + + // We have cached the mutator thread, delete it. + ASSERT(scheduled_mutator_thread_ == nullptr); + mutator_thread_->isolate_ = nullptr; + delete mutator_thread_; + mutator_thread_ = nullptr; + delete thread_registry_; if (obfuscation_map_ != nullptr) { @@ -1177,7 +1184,7 @@ void Isolate::RetainKernelBlob(const ExternalTypedData& kernel_blob) { Thread* Isolate::mutator_thread() const { ASSERT(thread_registry() != nullptr); - return thread_registry()->mutator_thread(); + return mutator_thread_; } RawObject* Isolate::CallTagHandler(Dart_LibraryTag tag, @@ -2051,6 +2058,12 @@ void Isolate::VisitStackPointers(ObjectPointerVisitor* visitor, ValidationPolicy validate_frames) { // Visit objects in all threads (e.g., Dart stack, handles in zones). thread_registry()->VisitObjectPointers(visitor, validate_frames); + + // Visit mutator thread, even if the isolate isn't entered/scheduled (there + // might be live API handles to visit). + if (mutator_thread_ != nullptr) { + mutator_thread_->VisitObjectPointers(visitor, validate_frames); + } } void Isolate::VisitWeakPersistentHandles(HandleVisitor* visitor) { @@ -2887,8 +2900,25 @@ Thread* Isolate::ScheduleThread(bool is_mutator, bool bypass_safepoint) { ml.Wait(); } - // Now get a free Thread structure. - thread = thread_registry()->GetFreeThreadLocked(this, is_mutator); + // NOTE: We cannot just use `Dart::vm_isolate() == this` here, since during + // VM startup it might not have been set at this point. + const bool is_vm_isolate = + Dart::vm_isolate() == nullptr || Dart::vm_isolate() == this; + + if (is_mutator) { + if (mutator_thread_ == nullptr) { + // Allocate a new [Thread] structure for the mutator thread. + thread = thread_registry()->GetFreeThreadLocked(is_vm_isolate); + mutator_thread_ = thread; + } else { + // Reuse the existing cached [Thread] structure for the mutator thread., + // see comment in 'base_isolate.h'. + thread_registry()->AddToActiveListLocked(mutator_thread_); + thread = mutator_thread_; + } + } else { + thread = thread_registry()->GetFreeThreadLocked(is_vm_isolate); + } ASSERT(thread != nullptr); thread->ResetHighWatermark(); @@ -2945,9 +2975,7 @@ void Isolate::UnscheduleThread(Thread* thread, os_thread->DisableThreadInterrupts(); os_thread->set_thread(nullptr); OSThread::SetCurrent(os_thread); - if (is_mutator) { - scheduled_mutator_thread_ = nullptr; - } + // Even if we unschedule the mutator thread, e.g. via calling // `Dart_ExitIsolate()` inside a native, we might still have one or more Dart // stacks active, which e.g. GC marker threads want to visit. So we don't @@ -2967,8 +2995,16 @@ void Isolate::UnscheduleThread(Thread* thread, thread->set_safepoint_state(Thread::SetAtSafepoint(true, 0)); thread->clear_pending_functions(); ASSERT(thread->no_safepoint_scope_depth() == 0); - // Return thread structure. - thread_registry()->ReturnThreadLocked(is_mutator, thread); + + if (is_mutator) { + ASSERT(mutator_thread_ == thread); + ASSERT(mutator_thread_ == scheduled_mutator_thread_); + thread_registry()->RemoveFromActiveListLocked(thread); + scheduled_mutator_thread_ = nullptr; + } else { + // Return thread structure. + thread_registry()->ReturnThreadLocked(thread); + } } static const char* NewConstChar(const char* chars) { diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index 71a9b2d38259..f9bcb736c39f 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -56,7 +56,7 @@ Thread::~Thread() { #define REUSABLE_HANDLE_INITIALIZERS(object) object##_handle_(NULL), -Thread::Thread(Isolate* isolate) +Thread::Thread(bool is_vm_isolate) : ThreadState(false), stack_limit_(0), stack_overflow_flags_(0), @@ -133,7 +133,7 @@ Thread::Thread(Isolate* isolate) // We cannot initialize the VM constants here for the vm isolate thread // due to boot strapping issues. - if ((Dart::vm_isolate() != NULL) && (isolate != Dart::vm_isolate())) { + if (!is_vm_isolate) { InitVMConstants(); } } diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index b13b1490b491..6795aa3744e1 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -938,7 +938,7 @@ class Thread : public ThreadState { Thread* next_; // Used to chain the thread structures in an isolate. - explicit Thread(Isolate* isolate); + explicit Thread(bool is_vm_isolate); void StoreBufferRelease( StoreBuffer::ThresholdPolicy policy = StoreBuffer::kCheckThreshold); diff --git a/runtime/vm/thread_registry.cc b/runtime/vm/thread_registry.cc index 2dcabf8234ac..6ff47500ea19 100644 --- a/runtime/vm/thread_registry.cc +++ b/runtime/vm/thread_registry.cc @@ -4,7 +4,6 @@ #include "vm/thread_registry.h" -#include "vm/isolate.h" #include "vm/json_stream.h" #include "vm/lockers.h" @@ -17,12 +16,6 @@ ThreadRegistry::~ThreadRegistry() { // At this point the active list should be empty. ASSERT(active_list_ == NULL); - // The [mutator_thread_] is kept alive until the destruction of the isolate. - mutator_thread_->isolate_ = nullptr; - - // We have cached the mutator thread, delete it. - delete mutator_thread_; - mutator_thread_ = NULL; // Now delete all the threads in the free list. while (free_list_ != NULL) { Thread* thread = free_list_; @@ -32,51 +25,34 @@ ThreadRegistry::~ThreadRegistry() { } } -// Gets a free Thread structure, we special case the mutator thread -// by reusing the cached structure, see comment in 'thread_registry.h'. -Thread* ThreadRegistry::GetFreeThreadLocked(Isolate* isolate, bool is_mutator) { +Thread* ThreadRegistry::GetFreeThreadLocked(bool is_vm_isolate) { ASSERT(threads_lock()->IsOwnedByCurrentThread()); - Thread* thread; - if (is_mutator) { - if (mutator_thread_ == NULL) { - mutator_thread_ = GetFromFreelistLocked(isolate); - } - thread = mutator_thread_; - } else { - thread = GetFromFreelistLocked(isolate); - ASSERT(thread->api_top_scope() == NULL); - } + Thread* thread = GetFromFreelistLocked(is_vm_isolate); + ASSERT(thread->api_top_scope() == NULL); // Now add this Thread to the active list for the isolate. AddToActiveListLocked(thread); return thread; } -void ThreadRegistry::ReturnThreadLocked(bool is_mutator, Thread* thread) { +void ThreadRegistry::ReturnThreadLocked(Thread* thread) { ASSERT(threads_lock()->IsOwnedByCurrentThread()); // Remove thread from the active list for the isolate. RemoveFromActiveListLocked(thread); - if (!is_mutator) { - ReturnToFreelistLocked(thread); - } + ReturnToFreelistLocked(thread); } void ThreadRegistry::VisitObjectPointers(ObjectPointerVisitor* visitor, ValidationPolicy validate_frames) { MonitorLocker ml(threads_lock()); - bool mutator_thread_visited = false; Thread* thread = active_list_; while (thread != NULL) { - thread->VisitObjectPointers(visitor, validate_frames); - if (mutator_thread_ == thread) { - mutator_thread_visited = true; + // The mutator thread is visited by the isolate itself (see + // [Isolate::VisitStackPointers]). + if (!thread->IsMutatorThread()) { + thread->VisitObjectPointers(visitor, validate_frames); } thread = thread->next_; } - // Visit mutator thread even if it is not in the active list because of - // api handles. - if (!mutator_thread_visited && (mutator_thread_ != NULL)) { - mutator_thread_->VisitObjectPointers(visitor, validate_frames); - } } void ThreadRegistry::ReleaseStoreBuffers() { @@ -174,12 +150,12 @@ void ThreadRegistry::RemoveFromActiveListLocked(Thread* thread) { } } -Thread* ThreadRegistry::GetFromFreelistLocked(Isolate* isolate) { +Thread* ThreadRegistry::GetFromFreelistLocked(bool is_vm_isolate) { ASSERT(threads_lock()->IsOwnedByCurrentThread()); Thread* thread = NULL; // Get thread structure from free list or create a new one. if (free_list_ == NULL) { - thread = new Thread(isolate); + thread = new Thread(is_vm_isolate); } else { thread = free_list_; free_list_ = thread->next_; diff --git a/runtime/vm/thread_registry.h b/runtime/vm/thread_registry.h index b6c0ec50e75f..80f05ca805e1 100644 --- a/runtime/vm/thread_registry.h +++ b/runtime/vm/thread_registry.h @@ -22,11 +22,7 @@ class JSONArray; // Unordered collection of threads relating to a particular isolate. class ThreadRegistry { public: - ThreadRegistry() - : threads_lock_(), - active_list_(NULL), - free_list_(NULL), - mutator_thread_(NULL) {} + ThreadRegistry() : threads_lock_(), active_list_(NULL), free_list_(NULL) {} ~ThreadRegistry(); void VisitObjectPointers(ObjectPointerVisitor* visitor, @@ -36,8 +32,6 @@ class ThreadRegistry { void AcquireMarkingStacks(); void ReleaseMarkingStacks(); - Thread* mutator_thread() const { return mutator_thread_; } - #ifndef PRODUCT void PrintJSON(JSONStream* stream) const; #endif @@ -52,11 +46,11 @@ class ThreadRegistry { Thread* active_list() const { return active_list_; } Monitor* threads_lock() const { return &threads_lock_; } - Thread* GetFreeThreadLocked(Isolate* isolate, bool is_mutator); - void ReturnThreadLocked(bool is_mutator, Thread* thread); + Thread* GetFreeThreadLocked(bool is_vm_isolate); + void ReturnThreadLocked(Thread* thread); void AddToActiveListLocked(Thread* thread); void RemoveFromActiveListLocked(Thread* thread); - Thread* GetFromFreelistLocked(Isolate* isolate); + Thread* GetFromFreelistLocked(bool is_vm_isolate); void ReturnToFreelistLocked(Thread* thread); // This monitor protects the threads list for an isolate, it is used whenever @@ -65,22 +59,6 @@ class ThreadRegistry { Thread* active_list_; // List of active threads in the isolate. Thread* free_list_; // Free list of Thread objects that can be reused. - // TODO(asiva): Currently we treat a mutator thread as a special thread - // and always schedule execution of Dart code on the same mutator thread - // object. The ApiLocalScope has been made thread specific but we still - // have scenarios where we do a temporary exit of an Isolate with live - // zones/handles in the API scope : - // - Dart_RunLoop() - // - IsolateSaver in Dart_NewNativePort - // Similarly, tracking async_stack_trace requires that we always reschedule - // on the same thread. - // We probably need a mechanism to return to the specific thread only - // for these specific cases. We should also determine if the embedder - // should allow exiting an isolate with live state in zones/handles in - // which case a new API for returning to the specific thread needs to be - // added. - Thread* mutator_thread_; - friend class Isolate; friend class SafepointHandler; friend class Scavenger;