cppgc: Rework testing GC infrastructure

Instead of moving the stand-alone logic to the base heap, allows
specific heaps to override their stand-alone GC behavior. This allows
CppHeap to reuse the unified heap bottlenecks and visitors for
testing. This works as long as any v8 references are empty as there is
no Isolate attached to the heap in this case.

- Reverts parts of https://crrev.com/c/2716291
- Relands parts of https://crrev.com/c/2718146

In addition, add tests covering v8::CppHeap and cppgc::Heap.

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

include/cppgc/testing.h

@@ -18,20 +18,6 @@ class HeapHandle;
  */
 namespace testing {
 
-/**
- * Testing helper used to acces heap internals.
- */
-class V8_EXPORT Heap final {
- public:
-  /**
-   * Atomically collects garbage on the C++ heap.
-   *
-   * \param heap_handle The corresponding heap.
-   * \param stack_state The stack state to assume for the garbage collection.
-   */
-  void CollectGarbage(HeapHandle& heap_handle, EmbedderStackState stack_state);
-};
-
 /**
  * Overrides the state of the stack with the provided value. Takes precedence
  * over other parameters that set the stack state. Must no be nested.

include/v8-cppgc.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <vector>
 
+#include "cppgc/common.h"
 #include "cppgc/custom-space.h"
 #include "cppgc/heap-statistics.h"
 #include "cppgc/internal/write-barrier.h"
@@ -118,6 +119,20 @@ class V8_EXPORT CppHeap {
   cppgc::HeapStatistics CollectStatistics(
       cppgc::HeapStatistics::DetailLevel detail_level);
 
+  /**
+   * Enables a detached mode that allows testing garbage collection using
+   * `cppgc::testing` APIs. Once used, the heap cannot be attached to an
+   * `Isolate` anymore.
+   */
+  void EnableDetachedGarbageCollectionsForTesting();
+
+  /**
+   * Performs a stop-the-world garbage collection for testing purposes.
+   *
+   * \param stack_state The stack state to assume for the garbage collection.
+   */
+  void CollectGarbageForTesting(cppgc::EmbedderStackState stack_state);
+
  private:
   CppHeap() = default;
 

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

@@ -64,6 +64,15 @@ cppgc::HeapStatistics CppHeap::CollectStatistics(
       detail_level);
 }
 
+void CppHeap::EnableDetachedGarbageCollectionsForTesting() {
+  return internal::CppHeap::From(this)
+      ->EnableDetachedGarbageCollectionsForTesting();
+}
+
+void CppHeap::CollectGarbageForTesting(cppgc::EmbedderStackState stack_state) {
+  return internal::CppHeap::From(this)->CollectGarbageForTesting(stack_state);
+}
+
 void JSHeapConsistency::DijkstraMarkingBarrierSlow(
     cppgc::HeapHandle& heap_handle, const TracedReferenceBase& ref) {
   auto& heap_base = cppgc::internal::HeapBase::From(heap_handle);
@@ -144,7 +153,7 @@ UnifiedHeapConcurrentMarker::CreateConcurrentMarkingVisitor(
 
 class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
  public:
-  UnifiedHeapMarker(Key, Heap& v8_heap, cppgc::internal::HeapBase& cpp_heap,
+  UnifiedHeapMarker(Key, Heap* v8_heap, cppgc::internal::HeapBase& cpp_heap,
                     cppgc::Platform* platform, MarkingConfig config);
 
   ~UnifiedHeapMarker() final = default;
@@ -166,7 +175,7 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
   cppgc::internal::ConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
-UnifiedHeapMarker::UnifiedHeapMarker(Key key, Heap& v8_heap,
+UnifiedHeapMarker::UnifiedHeapMarker(Key key, Heap* v8_heap,
                                      cppgc::internal::HeapBase& heap,
                                      cppgc::Platform* platform,
                                      MarkingConfig config)
@@ -221,6 +230,7 @@ void CppHeap::Terminate() {
 }
 
 void CppHeap::AttachIsolate(Isolate* isolate) {
+  CHECK(!in_detached_testing_mode);
   CHECK_NULL(isolate_);
   isolate_ = isolate;
   static_cast<CppgcPlatformAdapter*>(platform())
@@ -287,7 +297,8 @@ void CppHeap::TracePrologue(TraceFlags flags) {
   }
   marker_ =
       cppgc::internal::MarkerFactory::CreateAndStartMarking<UnifiedHeapMarker>(
-          *isolate_->heap(), AsBase(), platform_.get(), marking_config);
+          isolate_ ? isolate_->heap() : nullptr, AsBase(), platform_.get(),
+          marking_config);
   marking_done_ = false;
 }
 
@@ -390,5 +401,32 @@ void CppHeap::ReportBufferedAllocationSizeIfPossible() {
   buffered_allocated_bytes_ = 0;
 }
 
+void CppHeap::CollectGarbageForTesting(
+    cppgc::internal::GarbageCollector::Config::StackState stack_state) {
+  if (in_no_gc_scope()) return;
+
+  // Finish sweeping in case it is still running.
+  sweeper().FinishIfRunning();
+
+  if (isolate_) {
+    // Go through EmbedderHeapTracer API and perform a unified heap collection.
+    GarbageCollectionForTesting(stack_state);
+  } else {
+    // Perform an atomic GC, with starting incremental/concurrent marking and
+    // immediately finalizing the garbage collection.
+    TracePrologue(TraceFlags::kForced);
+    EnterFinalPause(stack_state);
+    AdvanceTracing(std::numeric_limits<double>::infinity());
+    TraceEpilogue(nullptr);
+  }
+}
+
+void CppHeap::EnableDetachedGarbageCollectionsForTesting() {
+  CHECK(!in_detached_testing_mode);
+  CHECK_NULL(isolate_);
+  no_gc_scope_--;
+  in_detached_testing_mode = true;
+}
+
 }  // namespace internal
 }  // namespace v8

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

@@ -50,6 +50,11 @@ class V8_EXPORT_PRIVATE CppHeap final
 
   void Terminate();
 
+  void EnableDetachedGarbageCollectionsForTesting();
+
+  void CollectGarbageForTesting(
+      cppgc::internal::GarbageCollector::Config::StackState);
+
   // v8::EmbedderHeapTracer interface.
   void RegisterV8References(
       const std::vector<std::pair<void*, void*> >& embedder_fields) final;
@@ -83,6 +88,8 @@ class V8_EXPORT_PRIVATE CppHeap final
   int64_t buffered_allocated_bytes_ = 0;
 
   v8::WrapperDescriptor wrapper_descriptor_;
+
+  bool in_detached_testing_mode = false;
 };
 
 }  // namespace internal

src/heap/cppgc-js/unified-heap-marking-state.h

@@ -7,6 +7,7 @@
 
 #include "include/v8-cppgc.h"
 #include "include/v8.h"
+#include "src/base/logging.h"
 #include "src/heap/heap.h"
 
 namespace v8 {
@@ -22,7 +23,7 @@ class BasicTracedReferenceExtractor {
 
 class UnifiedHeapMarkingState {
  public:
-  explicit UnifiedHeapMarkingState(Heap& heap) : heap_(heap) {}
+  explicit UnifiedHeapMarkingState(Heap* heap) : heap_(heap) {}
 
   UnifiedHeapMarkingState(const UnifiedHeapMarkingState&) = delete;
   UnifiedHeapMarkingState& operator=(const UnifiedHeapMarkingState&) = delete;
@@ -30,11 +31,16 @@ class UnifiedHeapMarkingState {
   inline void MarkAndPush(const TracedReferenceBase&);
 
  private:
-  Heap& heap_;
+  Heap* heap_;
 };
 
 void UnifiedHeapMarkingState::MarkAndPush(const TracedReferenceBase& ref) {
-  heap_.RegisterExternallyReferencedObject(
+  // The same visitor is used in testing scenarios without attaching the heap to
+  // an Isolate under the assumption that no non-empty v8 references are found.
+  // Having the following DCHECK crash means that the heap is in detached mode
+  // but we find traceable pointers into an Isolate.
+  DCHECK_NOT_NULL(heap_);
+  heap_->RegisterExternallyReferencedObject(
       BasicTracedReferenceExtractor::ObjectReference(ref));
 }
 

src/heap/cppgc/heap-base.cc

@@ -147,75 +147,5 @@ HeapStatistics HeapBase::CollectStatistics(
   return HeapStatisticsCollector().CollectStatistics(this);
 }
 
-void HeapBase::StandAloneGarbageCollectionForTesting(
-    GarbageCollector::Config::StackState stack_state) {
-  GarbageCollector::Config config = {
-      GarbageCollector::Config::CollectionType::kMajor, stack_state,
-      GarbageCollector::Config::MarkingType::kAtomic,
-      GarbageCollector::Config::SweepingType::kAtomic};
-
-  if (in_no_gc_scope()) return;
-
-  if (!IsMarking()) {
-    StartStandAloneGarbageCollection(config);
-  }
-  DCHECK(IsMarking());
-  FinalizeStandAloneGarbageCollection(config);
-}
-
-void HeapBase::StartStandAloneGarbageCollection(
-    GarbageCollector::Config config) {
-  DCHECK(!IsMarking());
-  DCHECK(!in_no_gc_scope());
-
-  // Finish sweeping in case it is still running.
-  sweeper_.FinishIfRunning();
-
-  epoch_++;
-
-#if defined(CPPGC_YOUNG_GENERATION)
-  if (config.collection_type == Config::CollectionType::kMajor)
-    Unmarker unmarker(&raw_heap());
-#endif
-
-  const Marker::MarkingConfig marking_config{
-      config.collection_type, config.stack_state, config.marking_type,
-      config.is_forced_gc};
-  marker_ = MarkerFactory::CreateAndStartMarking<Marker>(*this, platform_.get(),
-                                                         marking_config);
-}
-
-void HeapBase::FinalizeStandAloneGarbageCollection(
-    GarbageCollector::Config config) {
-  DCHECK(IsMarking());
-  DCHECK(!in_no_gc_scope());
-  CHECK(!in_disallow_gc_scope());
-  if (override_stack_state_) {
-    config.stack_state = *override_stack_state_;
-  }
-  in_atomic_pause_ = true;
-  {
-    // This guards atomic pause marking, meaning that no internal method or
-    // external callbacks are allowed to allocate new objects.
-    cppgc::subtle::DisallowGarbageCollectionScope no_gc_scope(*this);
-    marker_->FinishMarking(config.stack_state);
-  }
-  marker_.reset();
-  ExecutePreFinalizers();
-  // TODO(chromium:1056170): replace build flag with dedicated flag.
-#if DEBUG
-  MarkingVerifier verifier(*this);
-  verifier.Run(config.stack_state);
-#endif
-
-  subtle::NoGarbageCollectionScope no_gc(*this);
-  const Sweeper::SweepingConfig sweeping_config{
-      config.sweeping_type,
-      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep};
-  sweeper_.Start(sweeping_config);
-  in_atomic_pause_ = false;
-  sweeper_.NotifyDoneIfNeeded();
-}
-
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/heap-base.h

@@ -163,8 +163,6 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   HeapStatistics CollectStatistics(HeapStatistics::DetailLevel);
 
-  size_t epoch() const { return epoch_; }
-
   EmbedderStackState stack_state_of_prev_gc() const {
     return stack_state_of_prev_gc_;
   }
@@ -173,10 +171,6 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   }
 
  protected:
-  // Starts and finalizes stand-alone garbage collections.
-  void StartStandAloneGarbageCollection(GarbageCollector::Config);
-  void FinalizeStandAloneGarbageCollection(GarbageCollector::Config);
-
   // Used by the incremental scheduler to finalize a GC if supported.
   virtual void FinalizeIncrementalGarbageCollectionIfNeeded(
       cppgc::Heap::StackState) = 0;
@@ -224,12 +218,6 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   bool in_atomic_pause_ = false;
 
-  size_t epoch_ = 0;
-
- private:
-  void StandAloneGarbageCollectionForTesting(
-      GarbageCollector::Config::StackState);
-
   friend class MarkerBase::IncrementalMarkingTask;
   friend class testing::TestWithHeap;
   friend class cppgc::subtle::DisallowGarbageCollectionScope;

src/heap/cppgc/heap.cc

@@ -116,10 +116,10 @@ void Heap::CollectGarbage(Config config) {
   config_ = config;
 
   if (!IsMarking()) {
-    StartStandAloneGarbageCollection(config);
+    StartGarbageCollection(config);
   }
   DCHECK(IsMarking());
-  FinalizeStandAloneGarbageCollection(config);
+  FinalizeGarbageCollection(config.stack_state);
 }
 
 void Heap::StartIncrementalGarbageCollection(Config config) {
@@ -131,7 +131,7 @@ void Heap::StartIncrementalGarbageCollection(Config config) {
 
   config_ = config;
 
-  StartStandAloneGarbageCollection(config);
+  StartGarbageCollection(config);
 }
 
 void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
@@ -144,7 +144,60 @@ void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
 
   DCHECK_NE(Config::MarkingType::kAtomic, config_.marking_type);
   config_ = config;
-  FinalizeStandAloneGarbageCollection(config_);
+  FinalizeGarbageCollection(config.stack_state);
+}
+
+void Heap::StartGarbageCollection(Config config) {
+  DCHECK(!IsMarking());
+  DCHECK(!in_no_gc_scope());
+
+  // Finish sweeping in case it is still running.
+  sweeper_.FinishIfRunning();
+
+  epoch_++;
+
+#if defined(CPPGC_YOUNG_GENERATION)
+  if (config.collection_type == Config::CollectionType::kMajor)
+    Unmarker unmarker(&raw_heap());
+#endif
+
+  const Marker::MarkingConfig marking_config{
+      config.collection_type, config.stack_state, config.marking_type,
+      config.is_forced_gc};
+  marker_ = MarkerFactory::CreateAndStartMarking<Marker>(
+      AsBase(), platform_.get(), marking_config);
+}
+
+void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
+  DCHECK(IsMarking());
+  DCHECK(!in_no_gc_scope());
+  CHECK(!in_disallow_gc_scope());
+  config_.stack_state = stack_state;
+  if (override_stack_state_) {
+    config_.stack_state = *override_stack_state_;
+  }
+  in_atomic_pause_ = true;
+  {
+    // This guards atomic pause marking, meaning that no internal method or
+    // external callbacks are allowed to allocate new objects.
+    cppgc::subtle::DisallowGarbageCollectionScope no_gc_scope(*this);
+    marker_->FinishMarking(config_.stack_state);
+  }
+  marker_.reset();
+  ExecutePreFinalizers();
+  // TODO(chromium:1056170): replace build flag with dedicated flag.
+#if DEBUG
+  MarkingVerifier verifier(*this);
+  verifier.Run(config_.stack_state);
+#endif
+
+  subtle::NoGarbageCollectionScope no_gc(*this);
+  const Sweeper::SweepingConfig sweeping_config{
+      config_.sweeping_type,
+      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep};
+  sweeper_.Start(sweeping_config);
+  in_atomic_pause_ = false;
+  sweeper_.NotifyDoneIfNeeded();
 }
 
 void Heap::DisableHeapGrowingForTesting() { growing_.DisableForTesting(); }
@@ -153,10 +206,8 @@ void Heap::FinalizeIncrementalGarbageCollectionIfNeeded(
     Config::StackState stack_state) {
   StatsCollector::EnabledScope stats_scope(
       stats_collector(), StatsCollector::kMarkIncrementalFinalize);
-  FinalizeStandAloneGarbageCollection(config_);
+  FinalizeGarbageCollection(stack_state);
 }
 
-size_t Heap::epoch() const { return HeapBase::epoch(); }
-
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/heap.h

@@ -36,11 +36,14 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
   void StartIncrementalGarbageCollection(Config) final;
   void FinalizeIncrementalGarbageCollectionIfRunning(Config);
 
-  size_t epoch() const final;
+  size_t epoch() const final { return epoch_; }
 
   void DisableHeapGrowingForTesting();
 
  private:
+  void StartGarbageCollection(Config);
+  void FinalizeGarbageCollection(Config::StackState);
+
   void FinalizeIncrementalGarbageCollectionIfNeeded(Config::StackState) final;
 
   Config config_;
@@ -49,6 +52,8 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
 
   const MarkingType marking_support_;
   const SweepingType sweeping_support_;
+
+  size_t epoch_;
 };
 
 }  // namespace internal

src/heap/cppgc/marking-verifier.h

@@ -37,7 +37,7 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
   MarkingVerifierBase(const MarkingVerifierBase&) = delete;
   MarkingVerifierBase& operator=(const MarkingVerifierBase&) = delete;
 
-  void Run(GarbageCollector::Config::StackState);
+  void Run(Heap::Config::StackState);
 
  protected:
   MarkingVerifierBase(HeapBase&, std::unique_ptr<cppgc::Visitor>);

src/heap/cppgc/testing.cc

@@ -10,12 +10,6 @@
 namespace cppgc {
 namespace testing {
 
-void Heap::CollectGarbage(HeapHandle& heap_handle,
-                          EmbedderStackState stack_state) {
-  auto& heap = internal::HeapBase::From(heap_handle);
-  heap.StandAloneGarbageCollectionForTesting(stack_state);
-}
-
 OverrideEmbedderStackStateScope::OverrideEmbedderStackStateScope(
     HeapHandle& heap_handle, EmbedderStackState state)
     : heap_handle_(heap_handle) {
@@ -25,8 +19,7 @@ OverrideEmbedderStackStateScope::OverrideEmbedderStackStateScope(
 }
 
 OverrideEmbedderStackStateScope::~OverrideEmbedderStackStateScope() {
-  auto& heap = internal::HeapBase::From(heap_handle_);
-  heap.override_stack_state_.reset();
+  internal::HeapBase::From(heap_handle_).override_stack_state_.reset();
 }
 
 }  // namespace testing

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

@@ -6,6 +6,7 @@
 #include "include/cppgc/garbage-collected.h"
 #include "include/cppgc/persistent.h"
 #include "include/cppgc/platform.h"
+#include "include/cppgc/testing.h"
 #include "include/v8-cppgc.h"
 #include "include/v8.h"
 #include "src/api/api-inl.h"
@@ -140,6 +141,7 @@ TEST_F(UnifiedHeapDetachedTest, AllocationBeforeConfigureHeap) {
     cpp_heap.AsBase().sweeper().FinishIfRunning();
     EXPECT_TRUE(weak_holder);
   }
+  USE(object);
   {
     js_heap.SetEmbedderStackStateForNextFinalization(
         EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
@@ -149,5 +151,29 @@ TEST_F(UnifiedHeapDetachedTest, AllocationBeforeConfigureHeap) {
   }
 }
 
+TEST_F(UnifiedHeapDetachedTest, StandAloneCppGC) {
+  // Test ensures that stand-alone C++ GC are possible when using CppHeap. This
+  // works even in the presence of wrappables using TracedReference as long
+  // as the reference is empty.
+  auto heap = v8::CppHeap::Create(
+      V8::GetCurrentPlatform(),
+      CppHeapCreateParams{{}, WrapperHelper::DefaultWrapperDescriptor()});
+  auto* object =
+      cppgc::MakeGarbageCollected<Wrappable>(heap->GetAllocationHandle());
+  cppgc::WeakPersistent<Wrappable> weak_holder{object};
+
+  heap->EnableDetachedGarbageCollectionsForTesting();
+  {
+    heap->CollectGarbageForTesting(
+        cppgc::EmbedderStackState::kMayContainHeapPointers);
+    EXPECT_TRUE(weak_holder);
+  }
+  USE(object);
+  {
+    heap->CollectGarbageForTesting(cppgc::EmbedderStackState::kNoHeapPointers);
+    EXPECT_FALSE(weak_holder);
+  }
+}
+
 }  // namespace internal
 }  // namespace v8