Revert "unified-young-gen: Trace cross-heap references"

This reverts commit 43f03448d3.

Reason for revert: Data race caught by TSAN:
https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20isolates/22640/overview

Original change's description:
> unified-young-gen: Trace cross-heap references
>
> The CL enables the marking visitor in CppGC to trace
> v8::TracedReferences (by just reusing the unified heap visitor from the
> full GC). In addition, it specifies VisitJSApiObject for
> NewSpaceVisitors to be able to trace wrappers from Minor MC in case
> --cppgc-young-generation is enabled.
>
> Bug: v8:13475
> Change-Id: I04ba1f2a22e05caebf53dc8d64f2488c42ab8579
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4026896
> Commit-Queue: Anton Bikineev <bikineev@chromium.org>
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#84313}

Bug: v8:13475
Change-Id: I8b8351774a121ca2296efa3c8d3a588fa7380d86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4032053
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84314}

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

@@ -220,8 +220,12 @@ class UnifiedHeapConcurrentMarker
 std::unique_ptr<cppgc::Visitor>
 UnifiedHeapConcurrentMarker::CreateConcurrentMarkingVisitor(
     cppgc::internal::ConcurrentMarkingState& marking_state) const {
+  if (collection_type_ == CppHeap::CollectionType::kMajor)
     return std::make_unique<ConcurrentUnifiedHeapMarkingVisitor>(
-      heap(), v8_heap_, marking_state, collection_type_);
+        heap(), v8_heap_, marking_state);
+  else
+    return std::make_unique<ConcurrentMinorGCMarkingVisitor>(heap(), v8_heap_,
+                                                             marking_state);
 }
 
 void FatalOutOfMemoryHandlerImpl(const std::string& reason,
@@ -308,8 +312,13 @@ UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap,
                                      cppgc::internal::MarkingConfig config)
     : cppgc::internal::MarkerBase(heap, platform, config),
       mutator_unified_heap_marking_state_(v8_heap, nullptr),
-      marking_visitor_(std::make_unique<MutatorUnifiedHeapMarkingVisitor>(
+      marking_visitor_(config.collection_type == CppHeap::CollectionType::kMajor
-          heap, mutator_marking_state_, mutator_unified_heap_marking_state_)),
+                           ? std::make_unique<MutatorUnifiedHeapMarkingVisitor>(
+                                 heap, mutator_marking_state_,
+                                 mutator_unified_heap_marking_state_)
+                           : std::make_unique<MutatorMinorGCMarkingVisitor>(
+                                 heap, mutator_marking_state_,
+                                 mutator_unified_heap_marking_state_)),
       conservative_marking_visitor_(heap, mutator_marking_state_,
                                     *marking_visitor_) {
   concurrent_marker_ = std::make_unique<UnifiedHeapConcurrentMarker>(
@@ -677,18 +686,6 @@ void CppHeap::InitializeTracing(CollectionType collection_type,
       marking_config);
 }
 
-namespace {
-MarkingWorklists::Local* GetV8MarkingWorklists(
-    Isolate* isolate, cppgc::internal::CollectionType collection_type) {
-  auto* heap = isolate->heap();
-  if (collection_type == cppgc::internal::CollectionType::kMajor) {
-    return heap->mark_compact_collector()->local_marking_worklists();
-  } else {
-    return heap->minor_mark_compact_collector()->local_marking_worklists();
-  }
-}
-}  // namespace
-
 void CppHeap::StartTracing() {
   if (!TracingInitialized()) return;
   if (isolate_) {
@@ -698,7 +695,9 @@ void CppHeap::StartTracing() {
     marker_.get()
         ->To<UnifiedHeapMarker>()
         .GetMutatorUnifiedHeapMarkingState()
-        .Update(GetV8MarkingWorklists(isolate_, *collection_type_));
+        .Update(isolate_->heap()
+                    ->mark_compact_collector()
+                    ->local_marking_worklists());
   }
   marker_->StartMarking();
   marking_done_ = false;
@@ -743,11 +742,11 @@ void CppHeap::EnterFinalPause(cppgc::EmbedderStackState stack_state) {
   auto& marker = marker_.get()->To<UnifiedHeapMarker>();
   // Scan global handles conservatively in case we are attached to an Isolate.
   // TODO(1029379): Support global handle marking visitors with minor GC.
-  if (isolate_) {
+  if (isolate_ && !generational_gc_supported()) {
     auto& heap = *isolate()->heap();
     marker.conservative_visitor().SetConservativeTracedHandlesMarkingVisitor(
         std::make_unique<ConservativeTracedHandlesMarkingVisitor>(
-            heap, *GetV8MarkingWorklists(isolate_, *collection_type_)));
+            heap, *heap.mark_compact_collector()->local_marking_worklists()));
   }
   marker.EnterAtomicPause(stack_state);
   compactor_.CancelIfShouldNotCompact(MarkingType::kAtomic, stack_state);

src/heap/cppgc-js/unified-heap-marking-visitor.cc

@@ -14,18 +14,6 @@
 namespace v8 {
 namespace internal {
 
-namespace {
-std::unique_ptr<MarkingWorklists::Local> GetV8MarkingWorklists(
-    Heap* heap, cppgc::internal::CollectionType collection_type) {
-  if (!heap) return {};
-  auto* worklist =
-      (collection_type == cppgc::internal::CollectionType::kMajor)
-          ? heap->mark_compact_collector()->marking_worklists()
-          : heap->minor_mark_compact_collector()->marking_worklists();
-  return std::make_unique<MarkingWorklists::Local>(worklist);
-}
-}  // namespace
-
 UnifiedHeapMarkingVisitorBase::UnifiedHeapMarkingVisitorBase(
     HeapBase& heap, cppgc::internal::BasicMarkingState& marking_state,
     UnifiedHeapMarkingState& unified_heap_marking_state)
@@ -79,11 +67,13 @@ MutatorUnifiedHeapMarkingVisitor::MutatorUnifiedHeapMarkingVisitor(
 
 ConcurrentUnifiedHeapMarkingVisitor::ConcurrentUnifiedHeapMarkingVisitor(
     HeapBase& heap, Heap* v8_heap,
-    cppgc::internal::ConcurrentMarkingState& marking_state,
+    cppgc::internal::ConcurrentMarkingState& marking_state)
-    CppHeap::CollectionType collectione_type)
     : UnifiedHeapMarkingVisitorBase(heap, marking_state,
                                     concurrent_unified_heap_marking_state_),
-      local_marking_worklist_(GetV8MarkingWorklists(v8_heap, collectione_type)),
+      local_marking_worklist_(
+          v8_heap ? std::make_unique<MarkingWorklists::Local>(
+                        v8_heap->mark_compact_collector()->marking_worklists())
+                  : nullptr),
       concurrent_unified_heap_marking_state_(v8_heap,
                                              local_marking_worklist_.get()) {}
 

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

@@ -67,12 +67,23 @@ class V8_EXPORT_PRIVATE MutatorUnifiedHeapMarkingVisitor
   ~MutatorUnifiedHeapMarkingVisitor() override = default;
 };
 
+class V8_EXPORT_PRIVATE MutatorMinorGCMarkingVisitor final
+    : public MutatorUnifiedHeapMarkingVisitor {
+ public:
+  using MutatorUnifiedHeapMarkingVisitor::MutatorUnifiedHeapMarkingVisitor;
+  ~MutatorMinorGCMarkingVisitor() override = default;
+
+ protected:
+  // Override and make the function empty, since we don't want to trace V8
+  // reference during cppgc's minor GC.
+  void Visit(const TracedReferenceBase&) final {}
+};
+
 class V8_EXPORT_PRIVATE ConcurrentUnifiedHeapMarkingVisitor
     : public UnifiedHeapMarkingVisitorBase {
  public:
   ConcurrentUnifiedHeapMarkingVisitor(HeapBase&, Heap*,
-                                      cppgc::internal::ConcurrentMarkingState&,
+                                      cppgc::internal::ConcurrentMarkingState&);
-                                      CppHeap::CollectionType);
   ~ConcurrentUnifiedHeapMarkingVisitor() override;
 
  protected:
@@ -89,6 +100,20 @@ class V8_EXPORT_PRIVATE ConcurrentUnifiedHeapMarkingVisitor
   UnifiedHeapMarkingState concurrent_unified_heap_marking_state_;
 };
 
+// Same visitor as for full GCs unified heap, but avoids visiting
+// TracedReferences.
+class V8_EXPORT_PRIVATE ConcurrentMinorGCMarkingVisitor final
+    : public ConcurrentUnifiedHeapMarkingVisitor {
+ public:
+  using ConcurrentUnifiedHeapMarkingVisitor::
+      ConcurrentUnifiedHeapMarkingVisitor;
+
+ private:
+  // Override and make the function empty, since we don't want to trace V8
+  // reference during cppgc's minor GC.
+  void Visit(const TracedReferenceBase&) final {}
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/heap/marking-visitor-inl.h

@@ -592,23 +592,6 @@ int YoungGenerationMarkingVisitorBase<
   return size;
 }
 
-template <typename ConcreteVisitor, typename MarkingState>
-int YoungGenerationMarkingVisitorBase<
-    ConcreteVisitor, MarkingState>::VisitJSApiObject(Map map, JSObject object) {
-  if (!worklists_local_->SupportsExtractWrapper())
-    return this->VisitJSObject(map, object);
-  if (!concrete_visitor()->ShouldVisit(object)) return 0;
-  MarkingWorklists::Local::WrapperSnapshot wrapper_snapshot;
-  const bool valid_snapshot =
-      worklists_local_->ExtractWrapper(map, object, wrapper_snapshot);
-  const int size = concrete_visitor()->VisitJSObjectSubclass(map, object);
-  if (size && valid_snapshot) {
-    // Success: The object needs to be processed for embedder references.
-    worklists_local_->PushExtractedWrapper(wrapper_snapshot);
-  }
-  return size;
-}
-
 template <typename ConcreteVisitor, typename MarkingState>
 void YoungGenerationMarkingVisitorBase<ConcreteVisitor, MarkingState>::
     MarkObjectViaMarkingWorklist(HeapObject object) {

src/heap/marking-visitor.h

@@ -244,8 +244,6 @@ class YoungGenerationMarkingVisitorBase
 
   V8_INLINE int VisitJSArrayBuffer(Map map, JSArrayBuffer object);
 
-  V8_INLINE int VisitJSApiObject(Map map, JSObject object);
-
  protected:
   ConcreteVisitor* concrete_visitor() {
     return static_cast<ConcreteVisitor*>(this);

src/heap/objects-visiting-inl.h

@@ -216,7 +216,7 @@ template <typename ConcreteVisitor>
 int NewSpaceVisitor<ConcreteVisitor>::VisitJSApiObject(Map map,
                                                        JSObject object) {
   ConcreteVisitor* visitor = static_cast<ConcreteVisitor*>(this);
-  return visitor->VisitJSApiObject(map, object);
+  return visitor->VisitJSObject(map, object);
 }
 
 template <typename ConcreteVisitor>
@@ -233,16 +233,6 @@ int NewSpaceVisitor<ConcreteVisitor>::VisitWeakCell(Map map,
   return 0;
 }
 
-template <typename ConcreteVisitor>
-template <typename T, typename TBodyDescriptor>
-int NewSpaceVisitor<ConcreteVisitor>::VisitJSObjectSubclass(Map map, T object) {
-  if (!this->ShouldVisit(object)) return 0;
-  this->VisitMapPointer(object);
-  int size = TBodyDescriptor::SizeOf(map, object);
-  TBodyDescriptor::IterateBody(map, object, size, this);
-  return size;
-}
-
 }  // namespace internal
 }  // namespace v8
 

src/heap/objects-visiting.h

@@ -148,9 +148,6 @@ class NewSpaceVisitor : public HeapVisitor<int, ConcreteVisitor> {
 
   int VisitSharedFunctionInfo(Map map, SharedFunctionInfo object);
   int VisitWeakCell(Map map, WeakCell weak_cell);
-
-  template <typename T, typename TBodyDescriptor = typename T::BodyDescriptor>
-  int VisitJSObjectSubclass(Map map, T object);
 };
 
 class WeakObjectRetainer;

src/heap/scavenger-inl.h

@@ -479,7 +479,6 @@ class ScavengeVisitor final : public NewSpaceVisitor<ScavengeVisitor> {
   V8_INLINE void VisitEmbeddedPointer(Code host, RelocInfo* rinfo) final;
   V8_INLINE int VisitEphemeronHashTable(Map map, EphemeronHashTable object);
   V8_INLINE int VisitJSArrayBuffer(Map map, JSArrayBuffer object);
-  V8_INLINE int VisitJSApiObject(Map map, JSObject object);
 
  private:
   template <typename TSlot>
@@ -560,10 +559,6 @@ int ScavengeVisitor::VisitJSArrayBuffer(Map map, JSArrayBuffer object) {
   return size;
 }
 
-int ScavengeVisitor::VisitJSApiObject(Map map, JSObject object) {
-  return VisitJSObject(map, object);
-}
-
 int ScavengeVisitor::VisitEphemeronHashTable(Map map,
                                              EphemeronHashTable table) {
   // Register table with the scavenger, so it can take care of the weak keys

test/unittests/heap/cppgc-js/young-unified-heap-unittest.cc

@@ -87,47 +87,6 @@ TEST_F(YoungUnifiedHeapTest, CollectUnreachableCppGCObject) {
   EXPECT_EQ(1u, Wrappable::destructor_callcount);
 }
 
-TEST_F(YoungUnifiedHeapTest, FindingV8ToCppGCReference) {
-  v8::HandleScope scope(v8_isolate());
-  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
-  v8::Context::Scope context_scope(context);
-
-  uint16_t wrappable_type = WrapperHelper::kTracedEmbedderId;
-  auto* wrappable_object =
-      cppgc::MakeGarbageCollected<Wrappable>(allocation_handle());
-  v8::Local<v8::Object> api_object =
-      WrapperHelper::CreateWrapper(context, &wrappable_type, wrappable_object);
-  EXPECT_FALSE(api_object.IsEmpty());
-
-  Wrappable::destructor_callcount = 0;
-  CollectYoungGarbageWithoutEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
-  EXPECT_EQ(0u, Wrappable::destructor_callcount);
-
-  WrapperHelper::ResetWrappableConnection(api_object);
-  CollectGarbageWithoutEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
-  EXPECT_EQ(1u, Wrappable::destructor_callcount);
-}
-
-TEST_F(YoungUnifiedHeapTest, FindingCppGCToV8Reference) {
-  v8::HandleScope handle_scope(v8_isolate());
-  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
-  v8::Context::Scope context_scope(context);
-
-  auto* wrappable_object =
-      cppgc::MakeGarbageCollected<Wrappable>(allocation_handle());
-
-  {
-    v8::HandleScope inner_handle_scope(v8_isolate());
-    auto local = v8::Object::New(v8_isolate());
-    EXPECT_TRUE(local->IsObject());
-    wrappable_object->SetWrapper(v8_isolate(), local);
-  }
-
-  CollectYoungGarbageWithEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
-  auto local = wrappable_object->wrapper().Get(v8_isolate());
-  EXPECT_TRUE(local->IsObject());
-}
-
 }  // namespace internal
 }  // namespace v8