diff --git a/runtime/vm/base_isolate.h b/runtime/vm/base_isolate.h index 58a0cbbb8e1..be59b1667c8 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 dc10a412687..c9d3300fcdb 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 52119b78dec..21ba0679279 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 71a9b2d3825..f9bcb736c39 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 b13b1490b49..6795aa3744e 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 2dcabf8234a..6ff47500ea1 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 b6c0ec50e75..80f05ca805e 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;