cppgc: Dynamically trace previously in-construction objects

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

src/heap/cppgc/heap-inl.h

@@ -14,17 +14,13 @@ namespace internal {
 
 void* Heap::Allocate(size_t size, GCInfoIndex index) {
   DCHECK(is_allocation_allowed());
-  void* result = object_allocator_.AllocateObject(size, index);
-  objects_.push_back(&HeapObjectHeader::FromPayload(result));
-  return result;
+  return object_allocator_.AllocateObject(size, index);
 }
 
 void* Heap::Allocate(size_t size, GCInfoIndex index,
                      CustomSpaceIndex space_index) {
   DCHECK(is_allocation_allowed());
-  void* result = object_allocator_.AllocateObject(size, index, space_index);
-  objects_.push_back(&HeapObjectHeader::FromPayload(result));
-  return result;
+  return object_allocator_.AllocateObject(size, index, space_index);
 }
 
 }  // namespace internal

src/heap/cppgc/heap.cc

@@ -103,8 +103,9 @@ void Heap::CollectGarbage(GCConfig config) {
   // TODO(chromium:1056170): Replace with proper mark-sweep algorithm.
   // "Marking".
   marker_ = std::make_unique<Marker>(this);
-  marker_->StartMarking(Marker::MarkingConfig(config.stack_state));
-  marker_->FinishMarking();
+  Marker::MarkingConfig marking_config(config.stack_state);
+  marker_->StartMarking(marking_config);
+  marker_->FinishMarking(marking_config);
   // "Sweeping and finalization".
   {
     // Pre finalizers are forbidden from allocating objects

src/heap/cppgc/heap.h

@@ -116,10 +116,6 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   size_t ObjectPayloadSize() const;
 
-  // Temporary getter until proper visitation of on-stack objects is
-  // implemented.
-  std::vector<HeapObjectHeader*>& objects() { return objects_; }
-
  private:
   bool in_no_gc_scope() const { return no_gc_scope_ > 0; }
   bool is_allocation_allowed() const { return no_allocation_scope_ == 0; }
@@ -134,7 +130,6 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
   std::unique_ptr<Stack> stack_;
   std::unique_ptr<PreFinalizerHandler> prefinalizer_handler_;
   std::unique_ptr<Marker> marker_;
-  std::vector<HeapObjectHeader*> objects_;
 
   PersistentRegion strong_persistent_region_;
   PersistentRegion weak_persistent_region_;

src/heap/cppgc/marker.cc

@@ -22,6 +22,11 @@ class ResetLocalAllocationBufferVisitor final
     return true;
   }
 };
+
+void ResetLocalAllocationBuffers(Heap* heap) {
+  ResetLocalAllocationBufferVisitor visitor;
+  visitor.Traverse(&heap->raw_heap());
+}
 }  // namespace
 
 namespace {
@@ -62,12 +67,10 @@ Marker::~Marker() {
     NotFullyConstructedWorklist::View view(&not_fully_constructed_worklist_,
                                            kMutatorThreadId);
     while (view.Pop(&item)) {
-      // TODO(chromium:1056170): uncomment following check after implementing
-      // FromInnerAddress.
-      //
-      // HeapObjectHeader* const header = HeapObjectHeader::FromInnerAddress(
-      //     reinterpret_cast<Address>(const_cast<void*>(item)));
-      // DCHECK(header->IsMarked())
+      const HeapObjectHeader& header =
+          BasePage::FromPayload(item)->ObjectHeaderFromInnerAddress(
+              static_cast<ConstAddress>(item));
+      DCHECK(header.IsMarked());
     }
 #else
     not_fully_constructed_worklist_.Clear();
@@ -80,9 +83,16 @@ void Marker::StartMarking(MarkingConfig config) {
   VisitRoots();
 }
 
-void Marker::FinishMarking() {
+void Marker::FinishMarking(MarkingConfig config) {
+  config_ = config;
+
+  // Reset LABs before trying to conservatively mark in-construction objects.
+  // This is also needed in preparation for sweeping.
+  ResetLocalAllocationBuffers(heap_);
   if (config_.stack_state_ == MarkingConfig::StackState::kNoHeapPointers) {
     FlushNotFullyConstructedObjects();
+  } else {
+    MarkNotFullyConstructedObjects();
   }
   AdvanceMarkingWithDeadline(v8::base::TimeDelta::Max());
 }
@@ -102,12 +112,9 @@ void Marker::ProcessWeakness() {
 }
 
 void Marker::VisitRoots() {
-  {
-    // Reset LABs before scanning roots. LABs are cleared to allow
-    // ObjectStartBitmap handling without considering LABs.
-    ResetLocalAllocationBufferVisitor visitor;
-    visitor.Traverse(&heap_->raw_heap());
-  }
+  // Reset LABs before scanning roots. LABs are cleared to allow
+  // ObjectStartBitmap handling without considering LABs.
+  ResetLocalAllocationBuffers(heap_);
 
   heap_->GetStrongPersistentRegion().Trace(marking_visitor_.get());
   if (config_.stack_state_ != MarkingConfig::StackState::kNoHeapPointers)
@@ -161,6 +168,16 @@ void Marker::FlushNotFullyConstructedObjects() {
   DCHECK(not_fully_constructed_worklist_.IsLocalViewEmpty(kMutatorThreadId));
 }
 
+void Marker::MarkNotFullyConstructedObjects() {
+  NotFullyConstructedItem item;
+  NotFullyConstructedWorklist::View view(&not_fully_constructed_worklist_,
+                                         kMutatorThreadId);
+  while (view.Pop(&item)) {
+    marking_visitor_->ConservativelyMarkAddress(
+        BasePage::FromPayload(item), reinterpret_cast<ConstAddress>(item));
+  }
+}
+
 void Marker::ClearAllWorklistsForTesting() {
   marking_worklist_.Clear();
   not_fully_constructed_worklist_.Clear();

src/heap/cppgc/marker.h

@@ -78,8 +78,9 @@ class V8_EXPORT_PRIVATE Marker {
   // trigger incremental/concurrent marking if needed.
   void StartMarking(MarkingConfig config);
   // Finalize marking. This method stops incremental/concurrent marking
-  // if exsists and performs atomic pause marking.
-  void FinishMarking();
+  // if exsists and performs atomic pause marking. FinishMarking may
+  // update the MarkingConfig, e.g. if the stack state has changed.
+  void FinishMarking(MarkingConfig config);
 
   void ProcessWeakness();
 
@@ -94,6 +95,10 @@ class V8_EXPORT_PRIVATE Marker {
 
   void ClearAllWorklistsForTesting();
 
+  MutatorThreadMarkingVisitor* GetMarkingVisitorForTesting() {
+    return marking_visitor_.get();
+  }
+
  protected:
   virtual std::unique_ptr<MutatorThreadMarkingVisitor>
   CreateMutatorThreadMarkingVisitor();
@@ -103,6 +108,7 @@ class V8_EXPORT_PRIVATE Marker {
 
   bool AdvanceMarkingWithDeadline(v8::base::TimeDelta);
   void FlushNotFullyConstructedObjects();
+  void MarkNotFullyConstructedObjects();
 
   Heap* const heap_;
   MarkingConfig config_ = MarkingConfig::Default();

src/heap/cppgc/marking-visitor.cc

@@ -7,6 +7,7 @@
 #include "include/cppgc/garbage-collected.h"
 #include "include/cppgc/internal/accessors.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/heap-page.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/page-memory-inl.h"
 #include "src/heap/cppgc/sanitizers.h"
@@ -116,29 +117,21 @@ void MarkingVisitor::FlushWorklists() {
 }
 
 void MarkingVisitor::DynamicallyMarkAddress(ConstAddress address) {
-  for (auto* header : marker_->heap()->objects()) {
-    if (address >= header->Payload() &&
-        address < (header->Payload() + header->GetSize())) {
-      header->TryMarkAtomic();
-    }
+  HeapObjectHeader& header =
+      BasePage::FromPayload(address)->ObjectHeaderFromInnerAddress(
+          const_cast<Address>(address));
+  DCHECK(!IsInConstruction(header));
+  if (MarkHeaderNoTracing(&header)) {
+    marking_worklist_.Push(
+        {reinterpret_cast<void*>(header.Payload()),
+         GlobalGCInfoTable::GCInfoFromIndex(header.GetGCInfoIndex()).trace});
   }
-  // TODO(chromium:1056170): Implement dynamically getting HeapObjectHeader
-  // for handling previously_not_fully_constructed objects. Requires object
-  // start bitmap.
 }
 
-void MarkingVisitor::VisitPointer(const void* address) {
-  // TODO(chromium:1056170): Add page bloom filter
-
-  const BasePage* page = BasePage::FromInnerAddress(
-      marker_->heap(), static_cast<ConstAddress>(address));
-
-  if (!page) return;
-
-  DCHECK_EQ(marker_->heap(), page->heap());
-
+void MarkingVisitor::ConservativelyMarkAddress(const BasePage* page,
+                                               ConstAddress address) {
   HeapObjectHeader* const header =
-      page->TryObjectHeaderFromInnerAddress(const_cast<void*>(address));
+      page->TryObjectHeaderFromInnerAddress(const_cast<Address>(address));
 
   if (!header || header->IsMarked()) return;
 
@@ -181,6 +174,20 @@ void MarkingVisitor::VisitPointer(const void* address) {
   AccountMarkedBytes(*header);
 }
 
+void MarkingVisitor::VisitPointer(const void* address) {
+  // TODO(chromium:1056170): Add page bloom filter
+
+  const BasePage* page =
+      reinterpret_cast<const BasePage*>(marker_->heap()->page_backend()->Lookup(
+          static_cast<ConstAddress>(address)));
+
+  if (!page) return;
+
+  DCHECK_EQ(marker_->heap(), page->heap());
+
+  ConservativelyMarkAddress(page, reinterpret_cast<ConstAddress>(address));
+}
+
 MutatorThreadMarkingVisitor::MutatorThreadMarkingVisitor(Marker* marker)
     : MarkingVisitor(marker, Marker::kMutatorThreadId) {}
 

src/heap/cppgc/marking-visitor.h

@@ -9,8 +9,6 @@
 #include "include/cppgc/trace-trait.h"
 #include "include/v8config.h"
 #include "src/heap/cppgc/globals.h"
-#include "src/heap/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/heap-page.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/marker.h"
 #include "src/heap/cppgc/stack.h"
@@ -19,6 +17,9 @@
 namespace cppgc {
 namespace internal {
 
+class BasePage;
+class HeapObjectHeader;
+
 class MarkingVisitor : public VisitorBase, public StackVisitor {
  public:
   MarkingVisitor(Marker*, int);
@@ -30,6 +31,7 @@ class MarkingVisitor : public VisitorBase, public StackVisitor {
   void FlushWorklists();
 
   void DynamicallyMarkAddress(ConstAddress);
+  void ConservativelyMarkAddress(const BasePage*, ConstAddress);
 
   void AccountMarkedBytes(const HeapObjectHeader&);
   size_t marked_bytes() const { return marked_bytes_; }

src/heap/cppgc/sweeper.cc

@@ -115,7 +115,7 @@ class PrepareForSweepVisitor final
   explicit PrepareForSweepVisitor(SpaceStates* states) : states_(states) {}
 
   bool VisitNormalPageSpace(NormalPageSpace* space) {
-    space->ResetLinearAllocationBuffer();
+    DCHECK(!space->linear_allocation_buffer().size());
     space->free_list().Clear();
     (*states_)[space->index()].unswept_pages = space->RemoveAllPages();
     return true;

src/heap/cppgc/sweeper.h

@@ -24,6 +24,7 @@ class V8_EXPORT_PRIVATE Sweeper final {
   Sweeper(const Sweeper&) = delete;
   Sweeper& operator=(const Sweeper&) = delete;
 
+  // Sweeper::Start assumes the heap holds no linear allocation buffers.
   void Start(Config);
   void Finish();
 

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

@@ -8,6 +8,7 @@
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/marking-visitor.h"
 #include "test/unittests/heap/cppgc/tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -23,7 +24,7 @@ class MarkerTest : public testing::TestWithHeap {
   void DoMarking(MarkingConfig config) {
     Marker marker(Heap::From(GetHeap()));
     marker.StartMarking(config);
-    marker.FinishMarking();
+    marker.FinishMarking(config);
     marker.ProcessWeakness();
   }
 };
@@ -194,5 +195,47 @@ TEST_F(MarkerTest, NestedObjectsOnStackAreMarked) {
   EXPECT_TRUE(HeapObjectHeader::FromPayload(root->child()->child()).IsMarked());
 }
 
+namespace {
+class GCedWithCallback : public GarbageCollected<GCedWithCallback> {
+ public:
+  template <typename Callback>
+  explicit GCedWithCallback(Callback callback) {
+    callback(this);
+  }
+
+  void Trace(Visitor*) const {}
+};
+}  // namespace
+
+TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedEmptyStack) {
+  Marker marker(Heap::From(GetHeap()));
+  marker.StartMarking(
+      MarkingConfig(MarkingConfig::StackState::kMayContainHeapPointers));
+  GCedWithCallback* object = MakeGarbageCollected<GCedWithCallback>(
+      GetHeap(), [&marker](GCedWithCallback* obj) {
+        Member<GCedWithCallback> member(obj);
+        marker.GetMarkingVisitorForTesting()->Trace(member);
+      });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(object).IsMarked());
+  marker.FinishMarking(
+      MarkingConfig(MarkingConfig::StackState::kNoHeapPointers));
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
+}
+
+TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
+  Marker marker(Heap::From(GetHeap()));
+  marker.StartMarking(
+      MarkingConfig(MarkingConfig::StackState::kMayContainHeapPointers));
+  MakeGarbageCollected<GCedWithCallback>(
+      GetHeap(), [&marker](GCedWithCallback* obj) {
+        Member<GCedWithCallback> member(obj);
+        marker.GetMarkingVisitorForTesting()->Trace(member);
+        EXPECT_FALSE(HeapObjectHeader::FromPayload(obj).IsMarked());
+        marker.FinishMarking(
+            MarkingConfig(MarkingConfig::StackState::kMayContainHeapPointers));
+        EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
+      });
+}
+
 }  // namespace internal
 }  // namespace cppgc

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

@@ -12,6 +12,7 @@
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/page-memory-inl.h"
 #include "src/heap/cppgc/page-memory.h"
@@ -23,6 +24,21 @@ namespace internal {
 
 namespace {
 
+class ResetLocalAllocationBufferVisitor final
+    : public HeapVisitor<ResetLocalAllocationBufferVisitor> {
+ public:
+  bool VisitLargePageSpace(LargePageSpace*) { return true; }
+  bool VisitNormalPageSpace(NormalPageSpace* space) {
+    space->ResetLinearAllocationBuffer();
+    return true;
+  }
+};
+
+void ResetLocalAllocationBuffers(Heap* heap) {
+  ResetLocalAllocationBufferVisitor visitor;
+  visitor.Traverse(&heap->raw_heap());
+}
+
 size_t g_destructor_callcount;
 
 template <size_t Size>
@@ -41,7 +57,9 @@ class SweeperTest : public testing::TestWithHeap {
   SweeperTest() { g_destructor_callcount = 0; }
 
   void Sweep() {
-    Sweeper& sweeper = Heap::From(GetHeap())->sweeper();
+    Heap* heap = Heap::From(GetHeap());
+    ResetLocalAllocationBuffers(heap);
+    Sweeper& sweeper = heap->sweeper();
     sweeper.Start(Sweeper::Config::kAtomic);
     sweeper.Finish();
   }