cppgc: Mark in construction objects externally

In construction objects don't have anything to sync with on the
allocation side since they weren't marked as fully constructed yet.
This could mean the initialization of the marking bit on the mutator
thread and setting the mark bit on a concurrent thread could race
(potentially resulting in losing the mark bit when the gc info index
overwrites it).

This CL fixes this issue by using a set of in construction objects.
In construction objects are no longer marked. Instead they are pushed
to the set and the heap object header is marked when they are popped
from the worklist. Since the set avoids duplicates, this allows us to
both avoid worklist explosion (due to pushing the same in construction
 object multiple times) and avoid the data race on the mark bit.

This CL uses an unordered_set to record objects. Synchronization uses
a lock, which could be costly but is not expected to be obtained often.

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

src/heap/cppgc/marker.cc

@@ -194,12 +194,9 @@ MarkerBase::~MarkerBase() {
   if (!marking_worklists_.not_fully_constructed_worklist()->IsEmpty()) {
 #if DEBUG
     DCHECK_NE(MarkingConfig::StackState::kNoHeapPointers, config_.stack_state);
-    HeapObjectHeader* header;
-    MarkingWorklists::NotFullyConstructedWorklist::Local& local =
-        mutator_marking_state_.not_fully_constructed_worklist();
-    while (local.Pop(&header)) {
-      DCHECK(header->IsMarked());
-    }
+    std::unordered_set<HeapObjectHeader*> objects =
+        mutator_marking_state_.not_fully_constructed_worklist().Extract();
+    for (HeapObjectHeader* object : objects) DCHECK(object->IsMarked());
 #else
     marking_worklists_.not_fully_constructed_worklist()->Clear();
 #endif
@@ -384,15 +381,14 @@ bool MarkerBase::ProcessWorklistsWithDeadline(
 }
 
 void MarkerBase::MarkNotFullyConstructedObjects() {
-  HeapObjectHeader* header;
-  MarkingWorklists::NotFullyConstructedWorklist::Local& local =
-      mutator_marking_state_.not_fully_constructed_worklist();
-  while (local.Pop(&header)) {
-    DCHECK(header);
-    DCHECK(header->IsMarked<HeapObjectHeader::AccessMode::kNonAtomic>());
+  std::unordered_set<HeapObjectHeader*> objects =
+      mutator_marking_state_.not_fully_constructed_worklist().Extract();
+  for (HeapObjectHeader* object : objects) {
+    DCHECK(object);
+    DCHECK(object->IsMarked<HeapObjectHeader::AccessMode::kNonAtomic>());
     // TraceConservativelyIfNeeded will either push to a worklist
     // or trace conservatively and call AccountMarkedBytes.
-    conservative_visitor().TraceConservativelyIfNeeded(*header);
+    conservative_visitor().TraceConservativelyIfNeeded(*object);
   }
 }
 

src/heap/cppgc/marking-state.cc

@@ -4,16 +4,19 @@
 
 #include "src/heap/cppgc/marking-state.h"
 
+#include <unordered_set>
+
 namespace cppgc {
 namespace internal {
 
 void MarkingState::FlushNotFullyConstructedObjects() {
-  not_fully_constructed_worklist().Publish();
-  if (!not_fully_constructed_worklist_.IsGlobalEmpty()) {
-    previously_not_fully_constructed_worklist_.Merge(
-        &not_fully_constructed_worklist_);
+  std::unordered_set<HeapObjectHeader*> objects =
+      not_fully_constructed_worklist_.Extract();
+  for (HeapObjectHeader* object : objects) {
+    if (MarkNoPush(*object))
+      previously_not_fully_constructed_worklist_.Push(object);
   }
-  DCHECK(not_fully_constructed_worklist_.IsGlobalEmpty());
+  DCHECK(not_fully_constructed_worklist_.IsEmpty());
 }
 
 }  // namespace internal

src/heap/cppgc/marking-state.h

@@ -44,7 +44,6 @@ class MarkingState {
 
   void Publish() {
     marking_worklist_.Publish();
-    not_fully_constructed_worklist_.Publish();
     previously_not_fully_constructed_worklist_.Publish();
     weak_callback_worklist_.Publish();
     write_barrier_worklist_.Publish();
@@ -57,11 +56,11 @@ class MarkingState {
   MarkingWorklists::MarkingWorklist::Local& marking_worklist() {
     return marking_worklist_;
   }
-  MarkingWorklists::NotFullyConstructedWorklist::Local&
+  MarkingWorklists::NotFullyConstructedWorklist&
   not_fully_constructed_worklist() {
     return not_fully_constructed_worklist_;
   }
-  MarkingWorklists::NotFullyConstructedWorklist::Local&
+  MarkingWorklists::PreviouslyNotFullyConstructedWorklist::Local&
   previously_not_fully_constructed_worklist() {
     return previously_not_fully_constructed_worklist_;
   }
@@ -78,9 +77,9 @@ class MarkingState {
 #endif  // DEBUG
 
   MarkingWorklists::MarkingWorklist::Local marking_worklist_;
-  MarkingWorklists::NotFullyConstructedWorklist::Local
+  MarkingWorklists::NotFullyConstructedWorklist&
       not_fully_constructed_worklist_;
-  MarkingWorklists::NotFullyConstructedWorklist::Local
+  MarkingWorklists::PreviouslyNotFullyConstructedWorklist::Local
       previously_not_fully_constructed_worklist_;
   MarkingWorklists::WeakCallbackWorklist::Local weak_callback_worklist_;
   MarkingWorklists::WriteBarrierWorklist::Local write_barrier_worklist_;
@@ -95,7 +94,7 @@ MarkingState::MarkingState(HeapBase& heap, MarkingWorklists& marking_worklists)
 #endif  // DEBUG
       marking_worklist_(marking_worklists.marking_worklist()),
       not_fully_constructed_worklist_(
-          marking_worklists.not_fully_constructed_worklist()),
+          *marking_worklists.not_fully_constructed_worklist()),
       previously_not_fully_constructed_worklist_(
           marking_worklists.previously_not_fully_constructed_worklist()),
       weak_callback_worklist_(marking_worklists.weak_callback_worklist()),
@@ -112,11 +111,9 @@ void MarkingState::MarkAndPush(const void* object, TraceDescriptor desc) {
 void MarkingState::MarkAndPush(HeapObjectHeader& header, TraceDescriptor desc) {
   DCHECK_NOT_NULL(desc.callback);
 
-  if (!MarkNoPush(header)) return;
-
   if (header.IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>()) {
     not_fully_constructed_worklist_.Push(&header);
-  } else {
+  } else if (MarkNoPush(header)) {
     marking_worklist_.Push(desc);
   }
 }

src/heap/cppgc/marking-worklists.cc

@@ -18,5 +18,41 @@ void MarkingWorklists::ClearForTesting() {
   weak_callback_worklist_.Clear();
 }
 
+void MarkingWorklists::NotFullyConstructedWorklist::Push(
+    HeapObjectHeader* object) {
+  DCHECK_NOT_NULL(object);
+  v8::base::MutexGuard guard(&lock_);
+  objects_.insert(object);
+}
+
+std::unordered_set<HeapObjectHeader*>
+MarkingWorklists::NotFullyConstructedWorklist::Extract() {
+  v8::base::MutexGuard guard(&lock_);
+  std::unordered_set<HeapObjectHeader*> extracted;
+  std::swap(extracted, objects_);
+  DCHECK(objects_.empty());
+  return extracted;
+}
+
+void MarkingWorklists::NotFullyConstructedWorklist::Clear() {
+  v8::base::MutexGuard guard(&lock_);
+  objects_.clear();
+}
+
+bool MarkingWorklists::NotFullyConstructedWorklist::IsEmpty() {
+  v8::base::MutexGuard guard(&lock_);
+  return objects_.empty();
+}
+
+MarkingWorklists::NotFullyConstructedWorklist::~NotFullyConstructedWorklist() {
+  DCHECK(IsEmpty());
+}
+
+bool MarkingWorklists::NotFullyConstructedWorklist::ContainsForTesting(
+    HeapObjectHeader* object) {
+  v8::base::MutexGuard guard(&lock_);
+  return objects_.find(object) != objects_.end();
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/marking-worklists.h

@@ -5,7 +5,10 @@
 #ifndef V8_HEAP_CPPGC_MARKING_WORKLISTS_H_
 #define V8_HEAP_CPPGC_MARKING_WORKLISTS_H_
 
+#include <unordered_set>
+
 #include "include/cppgc/visitor.h"
+#include "src/base/platform/mutex.h"
 #include "src/heap/base/worklist.h"
 
 namespace cppgc {
@@ -27,18 +30,40 @@ class MarkingWorklists {
   // Since the work list is currently a temporary object this is not a problem.
   using MarkingWorklist =
       heap::base::Worklist<MarkingItem, 512 /* local entries */>;
-  using NotFullyConstructedWorklist =
+  using PreviouslyNotFullyConstructedWorklist =
       heap::base::Worklist<HeapObjectHeader*, 16 /* local entries */>;
   using WeakCallbackWorklist =
       heap::base::Worklist<WeakCallbackItem, 64 /* local entries */>;
   using WriteBarrierWorklist =
       heap::base::Worklist<HeapObjectHeader*, 64 /*local entries */>;
 
+  class V8_EXPORT_PRIVATE NotFullyConstructedWorklist {
+   public:
+    void Push(HeapObjectHeader*);
+    std::unordered_set<HeapObjectHeader*> Extract();
+    void Clear();
+    bool IsEmpty();
+
+    ~NotFullyConstructedWorklist();
+
+    bool ContainsForTesting(HeapObjectHeader*);
+
+   private:
+    void* operator new(size_t) = delete;
+    void* operator new[](size_t) = delete;
+    void operator delete(void*) = delete;
+    void operator delete[](void*) = delete;
+
+    v8::base::Mutex lock_;
+    std::unordered_set<HeapObjectHeader*> objects_;
+  };
+
   MarkingWorklist* marking_worklist() { return &marking_worklist_; }
   NotFullyConstructedWorklist* not_fully_constructed_worklist() {
     return &not_fully_constructed_worklist_;
   }
-  NotFullyConstructedWorklist* previously_not_fully_constructed_worklist() {
+  PreviouslyNotFullyConstructedWorklist*
+  previously_not_fully_constructed_worklist() {
     return &previously_not_fully_constructed_worklist_;
   }
   WriteBarrierWorklist* write_barrier_worklist() {
@@ -53,7 +78,8 @@ class MarkingWorklists {
  private:
   MarkingWorklist marking_worklist_;
   NotFullyConstructedWorklist not_fully_constructed_worklist_;
-  NotFullyConstructedWorklist previously_not_fully_constructed_worklist_;
+  PreviouslyNotFullyConstructedWorklist
+      previously_not_fully_constructed_worklist_;
   WriteBarrierWorklist write_barrier_worklist_;
   WeakCallbackWorklist weak_callback_worklist_;
 };

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

@@ -233,7 +233,7 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedEmptyStack) {
         Member<GCedWithCallback> member(obj);
         marker->VisitorForTesting().Trace(member);
       });
-  EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(object).IsMarked());
   marker()->FinishMarking(MarkingConfig::StackState::kMayContainHeapPointers);
   EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
 }
@@ -247,7 +247,7 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
       GetAllocationHandle(), [marker = marker()](GCedWithCallback* obj) {
         Member<GCedWithCallback> member(obj);
         marker->VisitorForTesting().Trace(member);
-        EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
+        EXPECT_FALSE(HeapObjectHeader::FromPayload(obj).IsMarked());
         marker->FinishMarking(
             MarkingConfig::StackState::kMayContainHeapPointers);
         EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
@@ -376,7 +376,7 @@ TEST_F(IncrementalMarkingTest, IncrementalStepDuringAllocation) {
         holder->member_ = obj;
         EXPECT_FALSE(header->IsMarked());
         FinishSteps(MarkingConfig::StackState::kMayContainHeapPointers);
-        EXPECT_TRUE(header->IsMarked());
+        EXPECT_FALSE(header->IsMarked());
       });
   FinishSteps(MarkingConfig::StackState::kNoHeapPointers);
   EXPECT_TRUE(header->IsMarked());

test/unittests/heap/cppgc/marking-visitor-unittest.cc

@@ -49,6 +49,8 @@ class TestMarkingVisitor : public MarkingVisitor {
   explicit TestMarkingVisitor(Marker* marker)
       : MarkingVisitor(marker->heap(), marker->MarkingStateForTesting()) {}
   ~TestMarkingVisitor() { marking_state_.Publish(); }
+
+  MarkingState& marking_state() { return marking_state_; }
 };
 
 }  // namespace
@@ -216,7 +218,11 @@ TEST_F(MarkingVisitorTest, MarkMemberInConstruction) {
             Member<GCedWithInConstructionCallback> object(obj);
             visitor.Trace(object);
           });
-  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_TRUE(visitor.marking_state()
+                  .not_fully_constructed_worklist()
+                  .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, MarkMemberMixinInConstruction) {
@@ -228,7 +234,11 @@ TEST_F(MarkingVisitorTest, MarkMemberMixinInConstruction) {
             Member<MixinWithInConstructionCallback> mixin(obj);
             visitor.Trace(mixin);
           });
-  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_TRUE(visitor.marking_state()
+                  .not_fully_constructed_worklist()
+                  .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
@@ -240,7 +250,11 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
             WeakMember<GCedWithInConstructionCallback> object(obj);
             visitor.Trace(object);
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_FALSE(visitor.marking_state()
+                   .not_fully_constructed_worklist()
+                   .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
@@ -252,7 +266,11 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
             WeakMember<MixinWithInConstructionCallback> mixin(obj);
             visitor.Trace(mixin);
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_FALSE(visitor.marking_state()
+                   .not_fully_constructed_worklist()
+                   .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, MarkPersistentInConstruction) {
@@ -264,7 +282,11 @@ TEST_F(MarkingVisitorTest, MarkPersistentInConstruction) {
             Persistent<GCedWithInConstructionCallback> object(obj);
             visitor.TraceRootForTesting(object, SourceLocation::Current());
           });
-  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_TRUE(visitor.marking_state()
+                  .not_fully_constructed_worklist()
+                  .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, MarkPersistentMixinInConstruction) {
@@ -276,7 +298,11 @@ TEST_F(MarkingVisitorTest, MarkPersistentMixinInConstruction) {
             Persistent<MixinWithInConstructionCallback> mixin(obj);
             visitor.TraceRootForTesting(mixin, SourceLocation::Current());
           });
-  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(gced);
+  EXPECT_TRUE(visitor.marking_state()
+                  .not_fully_constructed_worklist()
+                  .ContainsForTesting(&header));
+  EXPECT_FALSE(header.IsMarked());
 }
 
 }  // namespace internal