[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 #36097

Change-Id: Id27dff886d79ca76f6e05320151aeb72c8ba5140
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108720
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: mkustermann

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);

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;

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) {

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();
   }
 }

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);

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_;