Reland "[heap] Attach to shared isolate after setting up main thread"

This is a reland of 929b83fb

This version of the CL also fixes initialization of the
marking_barrier_ in the LocalHeap constructor.

This CL also got rebased on Victor's CL in https://crrev.com/c/3229361.
It added a code_space_allocator_ in LocalHeap which needs to be
initialized a bit later on the main thread as well.

Original change's description:
> [heap] Attach to shared isolate after setting up main thread
>
> Attach to the shared isolate after the main thread was set up. Otherwise
> it could happen that a shared GC initiated from another isolate might
> see no threads are running and performs the safepoint operation in the
> middle of isolate deserialization.
>
> We use DisallowSafepoints to check that the isolate doesn't join a
> global safepoint before deserialization is complete. DisallowSafepoints
> used to prevent only invocations of Safepoint() but was updated to
> also prevent Park() and Unpark() invocations. Each state change could
> cause the thread to reach a safepoint, which would allow a shared GC
> to run.
>
> We now also DCHECK that every isolate has at least one local heap and
> that shared collections aren't started before deserialization is
> complete.
>
> Bug: v8:11708
> Change-Id: Iba3fb59dd951d5ee4fc9934158062287302fc279
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3221157
> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Shu-yu Guo <syg@chromium.org>
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#77424}

Bug: v8:11708
Change-Id: I7d44e4a5f76cc09092c2444cede10e9331222c1d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3229361Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77448}

src/api/api.cc

@@ -8481,7 +8481,7 @@ void Isolate::Initialize(Isolate* isolate,
   }
 
   if (params.experimental_attach_to_shared_isolate != nullptr) {
-    i_isolate->AttachToSharedIsolate(reinterpret_cast<i::Isolate*>(
+    i_isolate->set_shared_isolate(reinterpret_cast<i::Isolate*>(
         params.experimental_attach_to_shared_isolate));
   }
 

src/execution/isolate.cc

@@ -3125,6 +3125,7 @@ bool Isolate::LogObjectRelocation() {
 
 void Isolate::Deinit() {
   TRACE_ISOLATE(deinit);
+  DisallowHeapAllocation no_allocation;
 
   tracing_cpu_profiler_.reset();
   if (FLAG_stress_sampling_allocation_profiler > 0) {
@@ -3210,9 +3211,7 @@ void Isolate::Deinit() {
 
   main_thread_local_isolate_->heap()->FreeLinearAllocationArea();
 
-  if (shared_isolate_) {
-    DetachFromSharedIsolate();
-  }
+  DetachFromSharedIsolate();
 
   heap_.TearDown();
 
@@ -3659,12 +3658,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
   date_cache_ = new DateCache();
   heap_profiler_ = new HeapProfiler(heap());
   interpreter_ = new interpreter::Interpreter(this);
-  if (OwnsStringTable()) {
-    string_table_ = std::make_shared<StringTable>(this);
-  } else {
-    DCHECK_NOT_NULL(shared_isolate_);
-    string_table_ = shared_isolate_->string_table_;
-  }
   bigint_processor_ = bigint::Processor::New(new BigIntPlatform(this));
 
   compiler_dispatcher_ = new LazyCompileDispatcher(
@@ -3684,12 +3677,33 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     stack_guard()->InitThread(lock);
   }
 
+  // Create LocalIsolate/LocalHeap for the main thread and set state to Running.
+  main_thread_local_isolate_.reset(new LocalIsolate(this, ThreadKind::kMain));
+  main_thread_local_heap()->Unpark();
+
+  // The main thread LocalHeap needs to be set up when attaching to the shared
+  // isolate. Otherwise a global safepoint would find an isolate without
+  // LocalHeaps and not wait until this thread is ready for a GC.
+  AttachToSharedIsolate();
+
+  // We need to ensure that we do not let a shared GC run before this isolate is
+  // fully set up.
+  DisallowSafepoints no_shared_gc;
+
   // SetUp the object heap.
   DCHECK(!heap_.HasBeenSetUp());
-  heap_.SetUp();
+  heap_.SetUp(main_thread_local_heap());
   ReadOnlyHeap::SetUp(this, read_only_snapshot_data, can_rehash);
   heap_.SetUpSpaces();
 
+  if (OwnsStringTable()) {
+    string_table_ = std::make_shared<StringTable>(this);
+  } else {
+    // Only refer to shared string table after attaching to the shared isolate.
+    DCHECK_NOT_NULL(shared_isolate());
+    string_table_ = shared_isolate()->string_table_;
+  }
+
   if (V8_SHORT_BUILTIN_CALLS_BOOL && FLAG_short_builtin_calls) {
     // Check if the system has more than 4GB of physical memory by comparing the
     // old space size with respective threshold value.
@@ -3714,14 +3728,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     }
   }
 
-  // Create LocalIsolate/LocalHeap for the main thread and set state to Running.
-  main_thread_local_isolate_.reset(new LocalIsolate(this, ThreadKind::kMain));
-  main_thread_local_heap()->Unpark();
-
-  heap_.InitializeMainThreadLocalHeap(main_thread_local_heap());
-
-  if (shared_isolate_) heap()->InitSharedSpaces();
-
   isolate_data_.external_reference_table()->Init(this);
 
 #if V8_ENABLE_WEBASSEMBLY
@@ -5163,19 +5169,30 @@ Address Isolate::store_to_stack_count_address(const char* function_name) {
   return reinterpret_cast<Address>(&map[name].second);
 }
 
-void Isolate::AttachToSharedIsolate(Isolate* shared) {
-  DCHECK(shared->is_shared());
-  DCHECK_NULL(shared_isolate_);
-  DCHECK(!heap_.HasBeenSetUp());
-  shared->AppendAsClientIsolate(this);
-  shared_isolate_ = shared;
+void Isolate::AttachToSharedIsolate() {
+  DCHECK(!attached_to_shared_isolate_);
+
+  if (shared_isolate_) {
+    DCHECK(shared_isolate_->is_shared());
+    shared_isolate_->AppendAsClientIsolate(this);
+  }
+
+#if DEBUG
+  attached_to_shared_isolate_ = true;
+#endif  // DEBUG
 }
 
 void Isolate::DetachFromSharedIsolate() {
-  DCHECK_NOT_NULL(shared_isolate_);
-  shared_isolate_->RemoveAsClientIsolate(this);
-  shared_isolate_ = nullptr;
-  heap()->DeinitSharedSpaces();
+  DCHECK(attached_to_shared_isolate_);
+
+  if (shared_isolate_) {
+    shared_isolate_->RemoveAsClientIsolate(this);
+    shared_isolate_ = nullptr;
+  }
+
+#if DEBUG
+  attached_to_shared_isolate_ = false;
+#endif  // DEBUG
 }
 
 void Isolate::AppendAsClientIsolate(Isolate* client) {

src/execution/isolate.h

@@ -1827,11 +1827,18 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
     using IsDebugActive = HasAsyncEventDelegate::Next<bool, 1>;
   };
 
-  bool is_shared() { return is_shared_; }
-  Isolate* shared_isolate() { return shared_isolate_; }
+  bool is_shared() const { return is_shared_; }
+  Isolate* shared_isolate() const {
+    DCHECK(attached_to_shared_isolate_);
+    return shared_isolate_;
+  }
 
-  void AttachToSharedIsolate(Isolate* shared);
-  void DetachFromSharedIsolate();
+  void set_shared_isolate(Isolate* shared_isolate) {
+    DCHECK(shared_isolate->is_shared());
+    DCHECK_NULL(shared_isolate_);
+    DCHECK(!attached_to_shared_isolate_);
+    shared_isolate_ = shared_isolate;
+  }
 
   bool HasClientIsolates() const { return client_isolate_head_; }
 
@@ -1967,6 +1974,12 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   // Returns the Exception sentinel.
   Object ThrowInternal(Object exception, MessageLocation* location);
 
+  // These methods add/remove the isolate to/from the list of clients in the
+  // shared isolate. Isolates in the client list need to participate in a global
+  // safepoint.
+  void AttachToSharedIsolate();
+  void DetachFromSharedIsolate();
+
   // Methods for appending and removing to/from client isolates list.
   void AppendAsClientIsolate(Isolate* client);
   void RemoveAsClientIsolate(Isolate* client);
@@ -2262,6 +2275,13 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   // isolates or when no shared isolate is used.
   Isolate* shared_isolate_ = nullptr;
 
+#if DEBUG
+  // Set to true once during isolate initialization right when attaching to the
+  // shared isolate. If there was no shared isolate given it will still be set
+  // to true. After this point invocations of shared_isolate() are valid.
+  bool attached_to_shared_isolate_ = false;
+#endif  // DEBUG
+
   // A shared isolate will use these two fields to track all its client
   // isolates.
   base::Mutex client_isolate_mutex_;

src/heap/heap.cc

@@ -2240,6 +2240,7 @@ size_t Heap::PerformGarbageCollection(
 }
 
 void Heap::CollectSharedGarbage(GarbageCollectionReason gc_reason) {
+  CHECK(deserialization_complete());
   DCHECK(!IsShared());
   DCHECK_NOT_NULL(isolate()->shared_isolate());
 
@@ -2266,6 +2267,7 @@ void Heap::PerformSharedGarbageCollection(Isolate* initiator,
                             : IsolateSafepoint::StopMainThread::kYes;
 
     client_heap->safepoint()->EnterSafepointScope(stop_main_thread);
+    DCHECK(client_heap->deserialization_complete());
 
     client_heap->shared_old_allocator_->FreeLinearAllocationArea();
     client_heap->shared_map_allocator_->FreeLinearAllocationArea();
@@ -5512,7 +5514,10 @@ HeapObject Heap::AllocateRawWithRetryOrFailSlowPath(
   FatalProcessOutOfMemory("CALL_AND_RETRY_LAST");
 }
 
-void Heap::SetUp() {
+void Heap::SetUp(LocalHeap* main_thread_local_heap) {
+  DCHECK_NULL(main_thread_local_heap_);
+  main_thread_local_heap_ = main_thread_local_heap;
+
 #ifdef V8_ENABLE_ALLOCATION_TIMEOUT
   allocation_timeout_ = NextAllocationTimeout();
 #endif
@@ -5705,11 +5710,18 @@ void Heap::SetUpSpaces() {
   }
 
   write_protect_code_memory_ = FLAG_write_protect_code_memory;
-}
 
-void Heap::InitializeMainThreadLocalHeap(LocalHeap* main_thread_local_heap) {
-  DCHECK_NULL(main_thread_local_heap_);
-  main_thread_local_heap_ = main_thread_local_heap;
+  if (isolate()->shared_isolate()) {
+    shared_old_space_ = isolate()->shared_isolate()->heap()->old_space();
+    shared_old_allocator_.reset(
+        new ConcurrentAllocator(main_thread_local_heap(), shared_old_space_));
+
+    shared_map_space_ = isolate()->shared_isolate()->heap()->map_space();
+    shared_map_allocator_.reset(
+        new ConcurrentAllocator(main_thread_local_heap(), shared_map_space_));
+  }
+
+  main_thread_local_heap()->SetUpMainThread();
 }
 
 void Heap::InitializeHashSeed() {
@@ -5992,6 +6004,12 @@ void Heap::TearDown() {
 
   allocation_sites_to_pretenure_.reset();
 
+  shared_old_space_ = nullptr;
+  shared_old_allocator_.reset();
+
+  shared_map_space_ = nullptr;
+  shared_map_allocator_.reset();
+
   for (int i = FIRST_MUTABLE_SPACE; i <= LAST_MUTABLE_SPACE; i++) {
     delete space_[i];
     space_[i] = nullptr;
@@ -6013,24 +6031,6 @@ void Heap::TearDown() {
   memory_allocator_.reset();
 }
 
-void Heap::InitSharedSpaces() {
-  shared_old_space_ = isolate()->shared_isolate()->heap()->old_space();
-  shared_old_allocator_.reset(
-      new ConcurrentAllocator(main_thread_local_heap(), shared_old_space_));
-
-  shared_map_space_ = isolate()->shared_isolate()->heap()->map_space();
-  shared_map_allocator_.reset(
-      new ConcurrentAllocator(main_thread_local_heap(), shared_map_space_));
-}
-
-void Heap::DeinitSharedSpaces() {
-  shared_old_space_ = nullptr;
-  shared_old_allocator_.reset();
-
-  shared_map_space_ = nullptr;
-  shared_map_allocator_.reset();
-}
-
 void Heap::AddGCPrologueCallback(v8::Isolate::GCCallbackWithData callback,
                                  GCType gc_type, void* data) {
   DCHECK_NOT_NULL(callback);

src/heap/heap.h

@@ -840,7 +840,7 @@ class Heap {
   void ConfigureHeapDefault();
 
   // Prepares the heap, setting up for deserialization.
-  void SetUp();
+  void SetUp(LocalHeap* main_thread_local_heap);
 
   // Sets read-only heap and space.
   void SetUpFromReadOnlyHeap(ReadOnlyHeap* ro_heap);
@@ -872,12 +872,6 @@ class Heap {
   // Returns whether SetUp has been called.
   bool HasBeenSetUp() const;
 
-  // Initialializes shared spaces.
-  void InitSharedSpaces();
-
-  // Removes shared spaces again.
-  void DeinitSharedSpaces();
-
   // ===========================================================================
   // Getters for spaces. =======================================================
   // ===========================================================================

src/heap/local-heap.cc

@@ -13,6 +13,7 @@
 #include "src/execution/isolate.h"
 #include "src/handles/local-handles.h"
 #include "src/heap/collection-barrier.h"
+#include "src/heap/concurrent-allocator.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/heap-write-barrier.h"
@@ -51,10 +52,8 @@ LocalHeap::LocalHeap(Heap* heap, ThreadKind kind,
       prev_(nullptr),
       next_(nullptr),
       handles_(new LocalHandles),
-      persistent_handles_(std::move(persistent_handles)),
-      marking_barrier_(new MarkingBarrier(this)),
-      old_space_allocator_(this, heap->old_space()),
-      code_space_allocator_(this, heap->code_space()) {
+      persistent_handles_(std::move(persistent_handles)) {
+  if (!is_main_thread()) SetUp();
   heap_->safepoint()->AddLocalHeap(this, [this] {
     if (!is_main_thread()) {
       WriteBarrier::SetForThread(marking_barrier_.get());
@@ -77,8 +76,7 @@ LocalHeap::~LocalHeap() {
   EnsureParkedBeforeDestruction();
 
   heap_->safepoint()->RemoveLocalHeap(this, [this] {
-    old_space_allocator_.FreeLinearAllocationArea();
-    code_space_allocator_.FreeLinearAllocationArea();
+    FreeLinearAllocationArea();
 
     if (!is_main_thread()) {
       marking_barrier_->Publish();
@@ -94,6 +92,26 @@ LocalHeap::~LocalHeap() {
   DCHECK(gc_epilogue_callbacks_.empty());
 }
 
+void LocalHeap::SetUpMainThreadForTesting() { SetUpMainThread(); }
+
+void LocalHeap::SetUpMainThread() {
+  DCHECK(is_main_thread());
+  SetUp();
+}
+
+void LocalHeap::SetUp() {
+  DCHECK_NULL(old_space_allocator_);
+  old_space_allocator_ =
+      std::make_unique<ConcurrentAllocator>(this, heap_->old_space());
+
+  DCHECK_NULL(code_space_allocator_);
+  code_space_allocator_ =
+      std::make_unique<ConcurrentAllocator>(this, heap_->code_space());
+
+  DCHECK_NULL(marking_barrier_);
+  marking_barrier_ = std::make_unique<MarkingBarrier>(this);
+}
+
 void LocalHeap::EnsurePersistentHandles() {
   if (!persistent_handles_) {
     persistent_handles_.reset(
@@ -227,23 +245,23 @@ void LocalHeap::SafepointSlowPath() {
 }
 
 void LocalHeap::FreeLinearAllocationArea() {
-  old_space_allocator_.FreeLinearAllocationArea();
-  code_space_allocator_.FreeLinearAllocationArea();
+  old_space_allocator_->FreeLinearAllocationArea();
+  code_space_allocator_->FreeLinearAllocationArea();
 }
 
 void LocalHeap::MakeLinearAllocationAreaIterable() {
-  old_space_allocator_.MakeLinearAllocationAreaIterable();
-  code_space_allocator_.MakeLinearAllocationAreaIterable();
+  old_space_allocator_->MakeLinearAllocationAreaIterable();
+  code_space_allocator_->MakeLinearAllocationAreaIterable();
 }
 
 void LocalHeap::MarkLinearAllocationAreaBlack() {
-  old_space_allocator_.MarkLinearAllocationAreaBlack();
-  code_space_allocator_.MarkLinearAllocationAreaBlack();
+  old_space_allocator_->MarkLinearAllocationAreaBlack();
+  code_space_allocator_->MarkLinearAllocationAreaBlack();
 }
 
 void LocalHeap::UnmarkLinearAllocationArea() {
-  old_space_allocator_.UnmarkLinearAllocationArea();
-  code_space_allocator_.UnmarkLinearAllocationArea();
+  old_space_allocator_->UnmarkLinearAllocationArea();
+  code_space_allocator_->UnmarkLinearAllocationArea();
 }
 
 bool LocalHeap::TryPerformCollection() {

src/heap/local-heap.h

@@ -93,8 +93,12 @@ class V8_EXPORT_PRIVATE LocalHeap {
   Heap* AsHeap() { return heap(); }
 
   MarkingBarrier* marking_barrier() { return marking_barrier_.get(); }
-  ConcurrentAllocator* old_space_allocator() { return &old_space_allocator_; }
-  ConcurrentAllocator* code_space_allocator() { return &code_space_allocator_; }
+  ConcurrentAllocator* old_space_allocator() {
+    return old_space_allocator_.get();
+  }
+  ConcurrentAllocator* code_space_allocator() {
+    return code_space_allocator_.get();
+  }
 
   // Mark/Unmark linear allocation areas black. Used for black allocation.
   void MarkLinearAllocationAreaBlack();
@@ -150,6 +154,9 @@ class V8_EXPORT_PRIVATE LocalHeap {
   void AddGCEpilogueCallback(GCEpilogueCallback* callback, void* data);
   void RemoveGCEpilogueCallback(GCEpilogueCallback* callback, void* data);
 
+  // Used to make SetupMainThread() available to unit tests.
+  void SetUpMainThreadForTesting();
+
  private:
   using ParkedBit = base::BitField8<bool, 0, 1>;
   using SafepointRequestedBit = ParkedBit::Next<bool, 1>;
@@ -248,7 +255,7 @@ class V8_EXPORT_PRIVATE LocalHeap {
                                             AllocationAlignment alignment);
 
   void Park() {
-    DCHECK(AllowGarbageCollection::IsAllowed());
+    DCHECK(AllowSafepoints::IsAllowed());
     ThreadState expected = ThreadState::Running();
     if (!state_.CompareExchangeWeak(expected, ThreadState::Parked())) {
       ParkSlowPath();
@@ -256,7 +263,7 @@ class V8_EXPORT_PRIVATE LocalHeap {
   }
 
   void Unpark() {
-    DCHECK(AllowGarbageCollection::IsAllowed());
+    DCHECK(AllowSafepoints::IsAllowed());
     ThreadState expected = ThreadState::Parked();
     if (!state_.CompareExchangeWeak(expected, ThreadState::Running())) {
       UnparkSlowPath();
@@ -272,6 +279,9 @@ class V8_EXPORT_PRIVATE LocalHeap {
 
   void InvokeGCEpilogueCallbacksInSafepoint();
 
+  void SetUpMainThread();
+  void SetUp();
+
   Heap* heap_;
   bool is_main_thread_;
 
@@ -289,8 +299,8 @@ class V8_EXPORT_PRIVATE LocalHeap {
 
   std::vector<std::pair<GCEpilogueCallback*, void*>> gc_epilogue_callbacks_;
 
-  ConcurrentAllocator old_space_allocator_;
-  ConcurrentAllocator code_space_allocator_;
+  std::unique_ptr<ConcurrentAllocator> old_space_allocator_;
+  std::unique_ptr<ConcurrentAllocator> code_space_allocator_;
 
   friend class CollectionBarrier;
   friend class ConcurrentAllocator;

src/heap/safepoint.cc

@@ -39,6 +39,9 @@ void IsolateSafepoint::EnterSafepointScope(StopMainThread stop_main_thread) {
 
   int running = 0;
 
+  // There needs to be at least one LocalHeap for the main thread.
+  DCHECK_NOT_NULL(local_heaps_head_);
+
   for (LocalHeap* local_heap = local_heaps_head_; local_heap;
        local_heap = local_heap->next_) {
     if (local_heap->is_main_thread() &&

test/cctest/test-serialize.cc

@@ -159,15 +159,14 @@ class TestSerializer {
     SnapshotData shared_space_snapshot(blobs.shared_space);
     const bool kEnableSerializer = false;
     const bool kGenerateHeap = false;
-    if (is_shared) CHECK_NULL(shared_isolate);
+    CHECK_IMPLIES(is_shared, !shared_isolate);
     v8::Isolate* v8_isolate =
         NewIsolate(kEnableSerializer, kGenerateHeap, is_shared);
     v8::Isolate::Scope isolate_scope(v8_isolate);
     i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
     if (shared_isolate) {
       CHECK(!is_shared);
-      isolate->AttachToSharedIsolate(
-          reinterpret_cast<Isolate*>(shared_isolate));
+      isolate->set_shared_isolate(reinterpret_cast<Isolate*>(shared_isolate));
     }
     isolate->Init(&startup_snapshot, &read_only_snapshot,
                   &shared_space_snapshot, false);

test/unittests/heap/local-heap-unittest.cc

@@ -29,6 +29,7 @@ TEST_F(LocalHeapTest, Current) {
 
   {
     LocalHeap lh(heap, ThreadKind::kMain);
+    lh.SetUpMainThreadForTesting();
     CHECK_NULL(LocalHeap::Current());
   }
 
@@ -36,6 +37,7 @@ TEST_F(LocalHeapTest, Current) {
 
   {
     LocalHeap lh(heap, ThreadKind::kMain);
+    lh.SetUpMainThreadForTesting();
     CHECK_NULL(LocalHeap::Current());
   }
 
@@ -67,6 +69,7 @@ TEST_F(LocalHeapTest, CurrentBackground) {
   CHECK_NULL(LocalHeap::Current());
   {
     LocalHeap lh(heap, ThreadKind::kMain);
+    lh.SetUpMainThreadForTesting();
     auto thread = std::make_unique<BackgroundThread>(heap);
     CHECK(thread->Start());
     CHECK_NULL(LocalHeap::Current());
@@ -157,6 +160,7 @@ class BackgroundThreadForGCEpilogue final : public v8::base::Thread {
 TEST_F(LocalHeapTest, GCEpilogue) {
   Heap* heap = i_isolate()->heap();
   LocalHeap lh(heap, ThreadKind::kMain);
+  lh.SetUpMainThreadForTesting();
   std::array<GCEpilogue, 3> epilogue;
   {
     UnparkedScope unparked(&lh);