cppgc: Allow allocations in prefinalziers

Prefinalizers have long been forbidden to allocate.
This restriction often proved problematic and has caused several
issues in the past.

This CL adds support for allowing allocations in prefinalizers.
At the start of prefinalizer invocations we clear the linear
allocation buffers, such that all allocations go through the slow
path for allocation. The slow path checks whether prefinalizers
are currently being invoked and marks the newly allocated object
if they are (i.e. black allocation during prefinalizers).

The new behavior is disabled by default and can be enabled by
setting the cppgc_allow_allocations_in_prefinalizers gn arg to true.

Bug: chromium:1056170
Change-Id: Ib86e780dcff88fa7b0f762ac2ab83c42393d33af
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3097877
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76606}

BUILD.gn

@@ -297,6 +297,9 @@ declare_args() {
   # TODO(v8:11749): Enable by default after fixing any existing issues in Blink.
   cppgc_enable_check_assignments_in_prefinalizers = false
 
+  # Enable allocations during prefinalizer invocations.
+  cppgc_allow_allocations_in_prefinalizers = false
+
   # Enable young generation in cppgc.
   cppgc_enable_young_generation = false
 
@@ -784,6 +787,10 @@ config("features") {
     defines += [ "CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS" ]
   }
 
+  if (cppgc_allow_allocations_in_prefinalizers) {
+    defines += [ "CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS" ]
+  }
+
   if (v8_embedder_string != "") {
     defines += [ "V8_EMBEDDER_STRING=\"$v8_embedder_string\"" ]
   }

src/heap/cppgc-js/cpp-heap.cc

@@ -493,12 +493,14 @@ void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {
   // The allocated bytes counter in v8 was reset to the current marked bytes, so
   // any pending allocated bytes updates should be discarded.
   buffered_allocated_bytes_ = 0;
-  ExecutePreFinalizers();
+  const size_t bytes_allocated_in_prefinalizers = ExecutePreFinalizers();
 #if CPPGC_VERIFY_HEAP
   UnifiedHeapMarkingVerifier verifier(*this);
-  verifier.Run(stack_state_of_prev_gc(), stack_end_of_current_gc(),
-               stats_collector()->marked_bytes());
+  verifier.Run(
+      stack_state_of_prev_gc(), stack_end_of_current_gc(),
+      stats_collector()->marked_bytes() + bytes_allocated_in_prefinalizers);
 #endif  // CPPGC_VERIFY_HEAP
+  USE(bytes_allocated_in_prefinalizers);
 
   {
     cppgc::subtle::NoGarbageCollectionScope no_gc(*this);

src/heap/cppgc/heap-base.cc

@@ -75,8 +75,8 @@ HeapBase::HeapBase(
           v8::base::Stack::GetStackStart())),
       prefinalizer_handler_(std::make_unique<PreFinalizerHandler>(*this)),
       compactor_(raw_heap_),
-      object_allocator_(&raw_heap_, page_backend_.get(),
-                        stats_collector_.get()),
+      object_allocator_(&raw_heap_, page_backend_.get(), stats_collector_.get(),
+                        prefinalizer_handler_.get()),
       sweeper_(*this),
       stack_support_(stack_support) {
   stats_collector_->RegisterObserver(
@@ -100,10 +100,17 @@ size_t HeapBase::ObjectPayloadSize() const {
 void HeapBase::AdvanceIncrementalGarbageCollectionOnAllocationIfNeeded() {
   if (marker_) marker_->AdvanceMarkingOnAllocation();
 }
-void HeapBase::ExecutePreFinalizers() {
+
+size_t HeapBase::ExecutePreFinalizers() {
+#ifdef CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
+  // Allocations in pre finalizers should not trigger another GC.
+  cppgc::subtle::NoGarbageCollectionScope no_gc_scope(*this);
+#else
   // Pre finalizers are forbidden from allocating objects.
   cppgc::subtle::DisallowGarbageCollectionScope no_gc_scope(*this);
+#endif  // CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
   prefinalizer_handler_->InvokePreFinalizers();
+  return prefinalizer_handler_->ExtractBytesAllocatedInPrefinalizers();
 }
 
 void HeapBase::Terminate() {

src/heap/cppgc/heap-base.h

@@ -215,7 +215,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   bool IsMarking() const { return marker_.get(); }
 
-  void ExecutePreFinalizers();
+  // Returns amount of bytes allocated while executing prefinalizers.
+  size_t ExecutePreFinalizers();
 
   PageAllocator* page_allocator() const;
 

src/heap/cppgc/heap-object-header.h

@@ -91,6 +91,8 @@ class HeapObjectHeader {
   void Unmark();
   inline bool TryMarkAtomic();
 
+  inline void MarkNonAtomic();
+
   template <AccessMode = AccessMode::kNonAtomic>
   bool IsYoung() const;
 
@@ -266,6 +268,11 @@ bool HeapObjectHeader::TryMarkAtomic() {
                                                  std::memory_order_relaxed);
 }
 
+void HeapObjectHeader::MarkNonAtomic() {
+  DCHECK(!IsMarked<AccessMode::kNonAtomic>());
+  encoded_low_ |= MarkBitField::encode(true);
+}
+
 template <AccessMode mode>
 bool HeapObjectHeader::IsYoung() const {
   return !IsMarked<mode>();

src/heap/cppgc/heap.cc

@@ -187,12 +187,17 @@ void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
     marker_->FinishMarking(config_.stack_state);
   }
   marker_.reset();
-  ExecutePreFinalizers();
+  const size_t bytes_allocated_in_prefinalizers = ExecutePreFinalizers();
 #if CPPGC_VERIFY_HEAP
   MarkingVerifier verifier(*this);
-  verifier.Run(config_.stack_state, stack_end_of_current_gc(),
-               stats_collector()->marked_bytes());
+  verifier.Run(
+      config_.stack_state, stack_end_of_current_gc(),
+      stats_collector()->marked_bytes() + bytes_allocated_in_prefinalizers);
 #endif  // CPPGC_VERIFY_HEAP
+#ifndef CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
+  DCHECK_EQ(0u, bytes_allocated_in_prefinalizers);
+#endif
+  USE(bytes_allocated_in_prefinalizers);
 
   subtle::NoGarbageCollectionScope no_gc(*this);
   const Sweeper::SweepingConfig sweeping_config{

src/heap/cppgc/object-allocator.cc

@@ -16,6 +16,7 @@
 #include "src/heap/cppgc/memory.h"
 #include "src/heap/cppgc/object-start-bitmap.h"
 #include "src/heap/cppgc/page-memory.h"
+#include "src/heap/cppgc/prefinalizer-handler.h"
 #include "src/heap/cppgc/stats-collector.h"
 #include "src/heap/cppgc/sweeper.h"
 
@@ -102,16 +103,28 @@ void* AllocateLargeObject(PageBackend* page_backend, LargePageSpace* space,
 constexpr size_t ObjectAllocator::kSmallestSpaceSize;
 
 ObjectAllocator::ObjectAllocator(RawHeap* heap, PageBackend* page_backend,
-                                 StatsCollector* stats_collector)
+                                 StatsCollector* stats_collector,
+                                 PreFinalizerHandler* prefinalizer_handler)
     : raw_heap_(heap),
       page_backend_(page_backend),
-      stats_collector_(stats_collector) {}
+      stats_collector_(stats_collector),
+      prefinalizer_handler_(prefinalizer_handler) {}
 
 void* ObjectAllocator::OutOfLineAllocate(NormalPageSpace& space, size_t size,
                                          GCInfoIndex gcinfo) {
   void* memory = OutOfLineAllocateImpl(space, size, gcinfo);
   stats_collector_->NotifySafePointForConservativeCollection();
   raw_heap_->heap()->AdvanceIncrementalGarbageCollectionOnAllocationIfNeeded();
+  if (prefinalizer_handler_->IsInvokingPreFinalizers()) {
+    // Objects allocated during pre finalizers should be allocated as black
+    // since marking is already done. Atomics are not needed because there is
+    // no concurrent marking in the background.
+    HeapObjectHeader::FromObject(memory).MarkNonAtomic();
+    // Resetting the allocation buffer forces all further allocations in pre
+    // finalizers to go through this slow path.
+    ReplaceLinearAllocationBuffer(space, *stats_collector_, nullptr, 0);
+    prefinalizer_handler_->NotifyAllocationInPrefinalizer(size);
+  }
   return memory;
 }
 

src/heap/cppgc/object-allocator.h

@@ -20,6 +20,7 @@ namespace cppgc {
 
 namespace internal {
 class ObjectAllocator;
+class PreFinalizerHandler;
 }  // namespace internal
 
 class V8_EXPORT AllocationHandle {
@@ -38,7 +39,8 @@ class V8_EXPORT_PRIVATE ObjectAllocator final : public cppgc::AllocationHandle {
   static constexpr size_t kSmallestSpaceSize = 32;
 
   ObjectAllocator(RawHeap* heap, PageBackend* page_backend,
-                  StatsCollector* stats_collector);
+                  StatsCollector* stats_collector,
+                  PreFinalizerHandler* prefinalizer_handler);
 
   inline void* AllocateObject(size_t size, GCInfoIndex gcinfo);
   inline void* AllocateObject(size_t size, GCInfoIndex gcinfo,
@@ -66,6 +68,7 @@ class V8_EXPORT_PRIVATE ObjectAllocator final : public cppgc::AllocationHandle {
   RawHeap* raw_heap_;
   PageBackend* page_backend_;
   StatsCollector* stats_collector_;
+  PreFinalizerHandler* prefinalizer_handler_;
 };
 
 void* ObjectAllocator::AllocateObject(size_t size, GCInfoIndex gcinfo) {

src/heap/cppgc/prefinalizer-handler.cc

@@ -31,7 +31,8 @@ bool PreFinalizerRegistrationDispatcher::PreFinalizer::operator==(
 }
 
 PreFinalizerHandler::PreFinalizerHandler(HeapBase& heap)
-    : heap_(heap)
+    : current_ordered_pre_finalizers_(&ordered_pre_finalizers_),
+      heap_(heap)
 #ifdef DEBUG
       ,
       creation_thread_id_(v8::base::OS::GetCurrentThreadId())
@@ -44,7 +45,10 @@ void PreFinalizerHandler::RegisterPrefinalizer(PreFinalizer pre_finalizer) {
   DCHECK_EQ(ordered_pre_finalizers_.end(),
             std::find(ordered_pre_finalizers_.begin(),
                       ordered_pre_finalizers_.end(), pre_finalizer));
-  ordered_pre_finalizers_.push_back(pre_finalizer);
+  DCHECK_EQ(current_ordered_pre_finalizers_->end(),
+            std::find(current_ordered_pre_finalizers_->begin(),
+                      current_ordered_pre_finalizers_->end(), pre_finalizer));
+  current_ordered_pre_finalizers_->push_back(pre_finalizer);
 }
 
 void PreFinalizerHandler::InvokePreFinalizers() {
@@ -54,6 +58,13 @@ void PreFinalizerHandler::InvokePreFinalizers() {
   DCHECK(CurrentThreadIsCreationThread());
   LivenessBroker liveness_broker = LivenessBrokerFactory::Create();
   is_invoking_ = true;
+  DCHECK_EQ(0u, bytes_allocated_in_prefinalizers);
+  // Reset all LABs to force allocations to the slow path for black allocation.
+  heap_.object_allocator().ResetLinearAllocationBuffers();
+  // Prefinalizers can allocate other objects with prefinalizers, which will
+  // modify ordered_pre_finalizers_ and break iterators.
+  std::vector<PreFinalizer> new_ordered_pre_finalizers;
+  current_ordered_pre_finalizers_ = &new_ordered_pre_finalizers;
   ordered_pre_finalizers_.erase(
       ordered_pre_finalizers_.begin(),
       std::remove_if(ordered_pre_finalizers_.rbegin(),
@@ -62,6 +73,12 @@ void PreFinalizerHandler::InvokePreFinalizers() {
                        return (pf.callback)(liveness_broker, pf.object);
                      })
           .base());
+  // Newly added objects with prefinalizers will always survive the current GC
+  // cycle, so it's safe to add them after clearing out the older prefinalizers.
+  ordered_pre_finalizers_.insert(ordered_pre_finalizers_.end(),
+                                 new_ordered_pre_finalizers.begin(),
+                                 new_ordered_pre_finalizers.end());
+  current_ordered_pre_finalizers_ = &ordered_pre_finalizers_;
   is_invoking_ = false;
   ordered_pre_finalizers_.shrink_to_fit();
 }
@@ -74,5 +91,11 @@ bool PreFinalizerHandler::CurrentThreadIsCreationThread() {
 #endif
 }
 
+void PreFinalizerHandler::NotifyAllocationInPrefinalizer(size_t size) {
+  DCHECK_GT(bytes_allocated_in_prefinalizers + size,
+            bytes_allocated_in_prefinalizers);
+  bytes_allocated_in_prefinalizers += size;
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/prefinalizer-handler.h

@@ -27,6 +27,11 @@ class PreFinalizerHandler final {
 
   bool IsInvokingPreFinalizers() const { return is_invoking_; }
 
+  void NotifyAllocationInPrefinalizer(size_t);
+  size_t ExtractBytesAllocatedInPrefinalizers() {
+    return std::exchange(bytes_allocated_in_prefinalizers, 0);
+  }
+
  private:
   // Checks that the current thread is the thread that created the heap.
   bool CurrentThreadIsCreationThread();
@@ -36,12 +41,16 @@ class PreFinalizerHandler final {
   // objects) for an object, by processing the ordered_pre_finalizers_
   // back-to-front.
   std::vector<PreFinalizer> ordered_pre_finalizers_;
+  std::vector<PreFinalizer>* current_ordered_pre_finalizers_;
 
   HeapBase& heap_;
   bool is_invoking_ = false;
 #ifdef DEBUG
   int creation_thread_id_;
 #endif
+
+  // Counter of bytes allocated during prefinalizers.
+  size_t bytes_allocated_in_prefinalizers = 0u;
 };
 
 }  // namespace internal

test/unittests/heap/cppgc/prefinalizer-unittest.cc

@@ -246,16 +246,23 @@ class AllocatingPrefinalizer : public GarbageCollected<AllocatingPrefinalizer> {
 
 }  // namespace
 
+#ifdef CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
+TEST_F(PrefinalizerTest, PrefinalizerDoesNotFailOnAllcoation) {
+  auto* object = MakeGarbageCollected<AllocatingPrefinalizer>(
+      GetAllocationHandle(), GetHeap());
+  PreciseGC();
+  USE(object);
+}
+#else
 #ifdef DEBUG
-
 TEST_F(PrefinalizerDeathTest, PrefinalizerFailsOnAllcoation) {
   auto* object = MakeGarbageCollected<AllocatingPrefinalizer>(
       GetAllocationHandle(), GetHeap());
   USE(object);
   EXPECT_DEATH_IF_SUPPORTED(PreciseGC(), "");
 }
-
 #endif  // DEBUG
+#endif  // CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
 
 namespace {
 
@@ -321,5 +328,17 @@ TEST_F(PrefinalizerDeathTest, PrefinalizerCantRessurectObjectOnHeap) {
 #endif  // CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS
 #endif  // V8_ENABLE_CHECKS
 
+#ifdef CPPGC_ALLOW_ALLOCATIONS_IN_PREFINALIZERS
+TEST_F(PrefinalizerTest, AllocatingPrefinalizersInMultipleGCCycles) {
+  auto* object = MakeGarbageCollected<AllocatingPrefinalizer>(
+      GetAllocationHandle(), GetHeap());
+  PreciseGC();
+  auto* other_object = MakeGarbageCollected<AllocatingPrefinalizer>(
+      GetAllocationHandle(), GetHeap());
+  PreciseGC();
+  USE(object);
+  USE(other_object);
+}
+#endif
 }  // namespace internal
 }  // namespace cppgc