Reland "cppgc-js,heap: Implement snapshots for embedder fields"

This is a reland of 142dd775b4

Original change's description:
> cppgc-js,heap: Implement snapshots for embedder fields
>
> https://crrev.com/c/3293410 added concurrent processing of C++ objects
> found through V8 embedder fields. The CL missed that those embedder
> fields are not read atomically from JS objects. The problem is that
> embedder fields are only aligned to kTaggedSize on builds with pointer
> compression and are as such mis-aligned for atomic ops. This is not a
> problem for on-heap values as the upper 32bits are anyways computed
> from the cage. Is is a problem for generic C++ values though, as they
> are used with Oilpan.
>
> This CL adds the standard marker snapshot protocol for embedder fields.
>
> Marker:
> 1. Snapshot embedder fields
> 2. Try to mark host object
> 3. On success: process snapshot
>
> Main thread:
> 1. On setting embedder fields mark the object black first
> 2. Emit a write barrier for the embedder fields
>
> This will get simpler with the heap sandbox that uses a separate table
> for embedder fields. Once the sandbox is the default configuration, we
> 	can use it as dependency for the concurrent fast path.
>
> Bug: chromium:1285706
> Change-Id: I6b975ea561be08cda840ef0dd27a11627de93900
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3380983
> Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#78604}

Bug: chromium:1285706
Change-Id: I024e50fc0757fbcd13cb9ffde027dff55f99d25c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3386600
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78631}

src/api/api.cc

@@ -6032,18 +6032,36 @@ void v8::Object::SetAlignedPointerInInternalField(int index, void* value) {
   i::Handle<i::JSReceiver> obj = Utils::OpenHandle(this);
   const char* location = "v8::Object::SetAlignedPointerInInternalField()";
   if (!InternalFieldOK(obj, index, location)) return;
+
+  i::DisallowGarbageCollection no_gc;
+
+  // There's no need to invalidate slots as embedder fields are always
+  // tagged.
+  obj->GetHeap()->NotifyObjectLayoutChange(*obj, no_gc,
+                                           i::InvalidateRecordedSlots::kNo);
+
   Utils::ApiCheck(i::EmbedderDataSlot(i::JSObject::cast(*obj), index)
                       .store_aligned_pointer(obj->GetIsolate(), value),
                   location, "Unaligned pointer");
   DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
   internal::WriteBarrier::MarkingFromInternalFields(i::JSObject::cast(*obj));
+
+#ifdef VERIFY_HEAP
+  obj->GetHeap()->VerifyObjectLayoutChange(*obj, obj->map());
+#endif  // VERIFY_HEAP
 }
 
 void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
                                                    void* values[]) {
   i::Handle<i::JSReceiver> obj = Utils::OpenHandle(this);
-  const char* location = "v8::Object::SetAlignedPointerInInternalFields()";
+
   i::DisallowGarbageCollection no_gc;
+  // There's no need to invalidate slots as embedder fields are always
+  // tagged.
+  obj->GetHeap()->NotifyObjectLayoutChange(*obj, no_gc,
+                                           i::InvalidateRecordedSlots::kNo);
+
+  const char* location = "v8::Object::SetAlignedPointerInInternalFields()";
   i::JSObject js_obj = i::JSObject::cast(*obj);
   int nof_embedder_fields = js_obj.GetEmbedderFieldCount();
   for (int i = 0; i < argc; i++) {
@@ -6059,6 +6077,10 @@ void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
     DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
   }
   internal::WriteBarrier::MarkingFromInternalFields(js_obj);
+
+#ifdef VERIFY_HEAP
+  obj->GetHeap()->VerifyObjectLayoutChange(*obj, obj->map());
+#endif  // VERIFY_HEAP
 }
 
 static void* ExternalValue(i::Object obj) {

src/heap/cppgc-js/cpp-marking-state-inl.h

@@ -7,16 +7,35 @@
 
 #include "src/heap/cppgc-js/cpp-marking-state.h"
 #include "src/heap/embedder-tracing-inl.h"
+#include "src/objects/embedder-data-slot.h"
 #include "src/objects/js-objects.h"
 
 namespace v8 {
 namespace internal {
 
-void CppMarkingState::MarkAndPush(const JSObject& js_object) {
-  DCHECK(js_object.IsApiWrapper());
+bool CppMarkingState::ExtractEmbedderDataSnapshot(
+    Map map, JSObject object, EmbedderDataSnapshot& snapshot) {
+  if (JSObject::GetEmbedderFieldCount(map) < 2) return false;
+
+  EmbedderDataSlot::PopulateEmbedderDataSnapshot(
+      map, object, wrapper_descriptor_.wrappable_type_index, snapshot.first);
+  EmbedderDataSlot::PopulateEmbedderDataSnapshot(
+      map, object, wrapper_descriptor_.wrappable_instance_index,
+      snapshot.second);
+  return true;
+}
+
+void CppMarkingState::MarkAndPush(const EmbedderDataSnapshot& snapshot) {
+  const EmbedderDataSlot type_slot(snapshot.first);
+  const EmbedderDataSlot instance_slot(snapshot.second);
+  MarkAndPush(type_slot, instance_slot);
+}
+
+void CppMarkingState::MarkAndPush(const EmbedderDataSlot type_slot,
+                                  const EmbedderDataSlot instance_slot) {
   LocalEmbedderHeapTracer::WrapperInfo info;
   if (LocalEmbedderHeapTracer::ExtractWrappableInfo(
-          isolate_, js_object, wrapper_descriptor_, &info)) {
+          isolate_, wrapper_descriptor_, type_slot, instance_slot, &info)) {
     marking_state_.MarkAndPush(
         cppgc::internal::HeapObjectHeader::FromObject(info.second));
   }

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

@@ -10,14 +10,20 @@
 #include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/heap/cppgc/marking-state.h"
 #include "src/heap/cppgc/marking-worklists.h"
+#include "src/objects/embedder-data-slot.h"
 
 namespace v8 {
 namespace internal {
 
 class JSObject;
+class EmbedderDataSlot;
 
 class CppMarkingState {
  public:
+  using EmbedderDataSnapshot =
+      std::pair<EmbedderDataSlot::EmbedderDataSlotSnapshot,
+                EmbedderDataSlot::EmbedderDataSlotSnapshot>;
+
   CppMarkingState(Isolate* isolate, const WrapperDescriptor& wrapper_descriptor,
                   cppgc::internal::MarkingStateBase& main_thread_marking_state)
       : isolate_(isolate),
@@ -37,7 +43,11 @@ class CppMarkingState {
 
   void Publish() { marking_state_.Publish(); }
 
-  inline void MarkAndPush(const JSObject& js_object);
+  inline bool ExtractEmbedderDataSnapshot(Map, JSObject, EmbedderDataSnapshot&);
+
+  inline void MarkAndPush(const EmbedderDataSnapshot&);
+  inline void MarkAndPush(const EmbedderDataSlot type_slot,
+                          const EmbedderDataSlot instance_slot);
 
   bool IsLocalEmpty() {
     return marking_state_.marking_worklist().IsLocalEmpty();

src/heap/embedder-tracing-inl.h

@@ -18,11 +18,20 @@ bool LocalEmbedderHeapTracer::ExtractWrappableInfo(
   DCHECK(js_object.IsApiWrapper());
   if (js_object.GetEmbedderFieldCount() < 2) return false;
 
-  if (EmbedderDataSlot(js_object, wrapper_descriptor.wrappable_type_index)
-          .ToAlignedPointerSafe(isolate, &info->first) &&
-      info->first &&
-      EmbedderDataSlot(js_object, wrapper_descriptor.wrappable_instance_index)
-          .ToAlignedPointerSafe(isolate, &info->second) &&
+  return ExtractWrappableInfo(
+      isolate, wrapper_descriptor,
+      EmbedderDataSlot(js_object, wrapper_descriptor.wrappable_type_index),
+      EmbedderDataSlot(js_object, wrapper_descriptor.wrappable_instance_index),
+      info);
+}
+
+// static
+bool LocalEmbedderHeapTracer::ExtractWrappableInfo(
+    Isolate* isolate, const WrapperDescriptor& wrapper_descriptor,
+    const EmbedderDataSlot& type_slot, const EmbedderDataSlot& instance_slot,
+    WrapperInfo* info) {
+  if (type_slot.ToAlignedPointerSafe(isolate, &info->first) && info->first &&
+      instance_slot.ToAlignedPointerSafe(isolate, &info->second) &&
       info->second) {
     return (wrapper_descriptor.embedder_id_for_garbage_collected ==
             WrapperDescriptor::kUnknownEmbedderId) ||

src/heap/embedder-tracing.cc

@@ -197,8 +197,14 @@ void LocalEmbedderHeapTracer::EmbedderWriteBarrier(Heap* heap,
   DCHECK(js_object.IsApiWrapper());
   if (cpp_heap_) {
     DCHECK_NOT_NULL(heap->mark_compact_collector());
-    heap->mark_compact_collector()->local_marking_worklists()->PushWrapper(
-        js_object);
+    const EmbedderDataSlot type_slot(js_object,
+                                     wrapper_descriptor_.wrappable_type_index);
+    const EmbedderDataSlot instance_slot(
+        js_object, wrapper_descriptor_.wrappable_instance_index);
+    heap->mark_compact_collector()
+        ->local_marking_worklists()
+        ->cpp_marking_state()
+        ->MarkAndPush(type_slot, instance_slot);
     return;
   }
   LocalEmbedderHeapTracer::ProcessingScope scope(this);

src/heap/embedder-tracing.h

@@ -77,6 +77,9 @@ class V8_EXPORT_PRIVATE LocalEmbedderHeapTracer final {
   static V8_INLINE bool ExtractWrappableInfo(Isolate*, JSObject,
                                              const WrapperDescriptor&,
                                              WrapperInfo*);
+  static V8_INLINE bool ExtractWrappableInfo(
+      Isolate*, const WrapperDescriptor&, const EmbedderDataSlot& type_slot,
+      const EmbedderDataSlot& instance_slot, WrapperInfo*);
 
   explicit LocalEmbedderHeapTracer(Isolate* isolate) : isolate_(isolate) {}
 

src/heap/heap-write-barrier.cc

@@ -49,11 +49,9 @@ void WriteBarrier::MarkingSlowFromGlobalHandle(Heap* heap, HeapObject value) {
 
 // static
 void WriteBarrier::MarkingSlowFromInternalFields(Heap* heap, JSObject host) {
-  // We are not checking the mark bits of host here as (a) there's no
-  // synchronization with the marker and (b) we are writing into a live object
-  // (independent of the mark bits).
   auto* local_embedder_heap_tracer = heap->local_embedder_heap_tracer();
   if (!local_embedder_heap_tracer->InUse()) return;
+
   local_embedder_heap_tracer->EmbedderWriteBarrier(heap, host);
 }
 

src/heap/marking-visitor-inl.h

@@ -6,6 +6,7 @@
 #define V8_HEAP_MARKING_VISITOR_INL_H_
 
 #include "src/heap/marking-visitor.h"
+#include "src/heap/marking-worklist.h"
 #include "src/heap/objects-visiting-inl.h"
 #include "src/heap/objects-visiting.h"
 #include "src/heap/progress-bar.h"
@@ -258,19 +259,47 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitFixedDoubleArray(
 // Objects participating in embedder tracing =================================
 // ===========================================================================
 
+template <typename ConcreteVisitor, typename MarkingState>
+template <typename T>
+inline int MarkingVisitorBase<ConcreteVisitor, MarkingState>::
+    VisitEmbedderTracingSubClassNoEmbedderTracing(Map map, T object) {
+  return concrete_visitor()->VisitJSObjectSubclass(map, object);
+}
+
+template <typename ConcreteVisitor, typename MarkingState>
+template <typename T>
+inline int MarkingVisitorBase<ConcreteVisitor, MarkingState>::
+    VisitEmbedderTracingSubClassWithEmbedderTracing(Map map, T object) {
+  const bool requires_snapshot =
+      local_marking_worklists_->SupportsExtractWrapper();
+  MarkingWorklists::Local::WrapperSnapshot wrapper_snapshot;
+  const bool valid_snapshot =
+      requires_snapshot &&
+      local_marking_worklists_->ExtractWrapper(map, object, wrapper_snapshot);
+  const int size = concrete_visitor()->VisitJSObjectSubclass(map, object);
+  if (size) {
+    if (valid_snapshot) {
+      // Success: The object needs to be processed for embedder references.
+      local_marking_worklists_->PushExtractedWrapper(wrapper_snapshot);
+    } else if (!requires_snapshot) {
+      // Snapshot not supported. Just fall back to pushing the wrapper itself
+      // instead which will be processed on the main thread.
+      local_marking_worklists_->PushWrapper(object);
+    }
+  }
+  return size;
+}
+
 template <typename ConcreteVisitor, typename MarkingState>
 template <typename T>
 int MarkingVisitorBase<ConcreteVisitor,
                        MarkingState>::VisitEmbedderTracingSubclass(Map map,
                                                                    T object) {
   DCHECK(object.IsApiWrapper());
-  int size = concrete_visitor()->VisitJSObjectSubclass(map, object);
-  if (size && is_embedder_tracing_enabled_) {
-    // Success: The object needs to be processed for embedder references on
-    // the main thread.
-    local_marking_worklists_->PushWrapper(object);
+  if (V8_LIKELY(is_embedder_tracing_enabled_)) {
+    return VisitEmbedderTracingSubClassWithEmbedderTracing(map, object);
   }
-  return size;
+  return VisitEmbedderTracingSubClassNoEmbedderTracing(map, object);
 }
 
 template <typename ConcreteVisitor, typename MarkingState>

src/heap/marking-visitor.h

@@ -218,6 +218,11 @@ class MarkingVisitorBase : public HeapVisitor<int, ConcreteVisitor> {
 
   template <typename T>
   int VisitEmbedderTracingSubclass(Map map, T object);
+  template <typename T>
+  int VisitEmbedderTracingSubClassWithEmbedderTracing(Map map, T object);
+  template <typename T>
+  int VisitEmbedderTracingSubClassNoEmbedderTracing(Map map, T object);
+
   V8_INLINE int VisitFixedArrayWithProgressBar(Map map, FixedArray object,
                                                ProgressBar& progress_bar);
   // Marks the descriptor array black without pushing it on the marking work

src/heap/marking-worklist-inl.h

@@ -8,6 +8,7 @@
 
 #include "src/heap/cppgc-js/cpp-marking-state-inl.h"
 #include "src/heap/marking-worklist.h"
+#include "src/objects/embedder-data-slot.h"
 #include "src/objects/js-objects-inl.h"
 
 namespace v8 {
@@ -46,16 +47,29 @@ bool MarkingWorklists::Local::PopOnHold(HeapObject* object) {
   return on_hold_.Pop(object);
 }
 
+bool MarkingWorklists::Local::SupportsExtractWrapper() {
+  return cpp_marking_state_.get();
+}
+
+bool MarkingWorklists::Local::ExtractWrapper(Map map, JSObject object,
+                                             WrapperSnapshot& snapshot) {
+  DCHECK_NOT_NULL(cpp_marking_state_);
+  return cpp_marking_state_->ExtractEmbedderDataSnapshot(map, object, snapshot);
+}
+
+void MarkingWorklists::Local::PushExtractedWrapper(
+    const WrapperSnapshot& snapshot) {
+  DCHECK_NOT_NULL(cpp_marking_state_);
+  cpp_marking_state_->MarkAndPush(snapshot);
+}
+
 void MarkingWorklists::Local::PushWrapper(HeapObject object) {
-  if (cpp_marking_state_) {
-    cpp_marking_state_->MarkAndPush(JSObject::cast(object));
-  } else {
-    wrapper_.Push(object);
-  }
+  DCHECK_NULL(cpp_marking_state_);
+  wrapper_.Push(object);
 }
 
 bool MarkingWorklists::Local::PopWrapper(HeapObject* object) {
-  DCHECK(!cpp_marking_state_);
+  DCHECK_NULL(cpp_marking_state_);
   return wrapper_.Pop(object);
 }
 

src/heap/marking-worklist.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "src/heap/base/worklist.h"
+#include "src/heap/cppgc-js/cpp-marking-state.h"
 #include "src/heap/marking.h"
 #include "src/objects/heap-object.h"
 
@@ -157,6 +158,11 @@ class V8_EXPORT_PRIVATE MarkingWorklists::Local {
   inline void PushOnHold(HeapObject object);
   inline bool PopOnHold(HeapObject* object);
 
+  using WrapperSnapshot = CppMarkingState::EmbedderDataSnapshot;
+  inline bool ExtractWrapper(Map map, JSObject object,
+                             WrapperSnapshot& snapshot);
+  inline void PushExtractedWrapper(const WrapperSnapshot& snapshot);
+  inline bool SupportsExtractWrapper();
   inline void PushWrapper(HeapObject object);
   inline bool PopWrapper(HeapObject* object);
 
@@ -180,6 +186,10 @@ class V8_EXPORT_PRIVATE MarkingWorklists::Local {
   inline Address SwitchToShared();
   bool IsPerContextMode() const { return is_per_context_mode_; }
 
+  CppMarkingState* cpp_marking_state() const {
+    return cpp_marking_state_.get();
+  }
+
  private:
   bool PopContext(HeapObject* object);
   Address SwitchToContextSlow(Address context);

src/objects/embedder-data-slot-inl.h

@@ -5,11 +5,11 @@
 #ifndef V8_OBJECTS_EMBEDDER_DATA_SLOT_INL_H_
 #define V8_OBJECTS_EMBEDDER_DATA_SLOT_INL_H_
 
-#include "src/objects/embedder-data-slot.h"
-
 #include "src/base/memory.h"
+#include "src/common/globals.h"
 #include "src/heap/heap-write-barrier-inl.h"
 #include "src/objects/embedder-data-array.h"
+#include "src/objects/embedder-data-slot.h"
 #include "src/objects/js-objects-inl.h"
 #include "src/objects/objects-inl.h"
 
@@ -27,6 +27,9 @@ EmbedderDataSlot::EmbedderDataSlot(JSObject object, int embedder_field_index)
     : SlotBase(FIELD_ADDR(
           object, object.GetEmbedderFieldOffset(embedder_field_index))) {}
 
+EmbedderDataSlot::EmbedderDataSlot(const EmbedderDataSlotSnapshot& snapshot)
+    : SlotBase(reinterpret_cast<Address>(&snapshot)) {}
+
 void EmbedderDataSlot::AllocateExternalPointerEntry(Isolate* isolate) {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   // TODO(v8:10391, saelo): Use InitExternalPointerField() once
@@ -190,6 +193,30 @@ void EmbedderDataSlot::gc_safe_store(Isolate* isolate, Address value) {
 #endif
 }
 
+// static
+void EmbedderDataSlot::PopulateEmbedderDataSnapshot(
+    Map map, JSObject js_object, int entry_index,
+    EmbedderDataSlotSnapshot& snapshot) {
+#ifdef V8_COMPRESS_POINTERS
+  STATIC_ASSERT(sizeof(EmbedderDataSlotSnapshot) == sizeof(AtomicTagged_t) * 2);
+#else   // !V8_COMPRESS_POINTERS
+  STATIC_ASSERT(sizeof(EmbedderDataSlotSnapshot) == sizeof(AtomicTagged_t));
+#endif  // !V8_COMPRESS_POINTERS
+  STATIC_ASSERT(sizeof(EmbedderDataSlotSnapshot) == kEmbedderDataSlotSize);
+
+  const Address field_base =
+      FIELD_ADDR(js_object, js_object.GetEmbedderFieldOffset(entry_index));
+
+  reinterpret_cast<AtomicTagged_t*>(&snapshot)[0] =
+      AsAtomicTagged::Relaxed_Load(
+          reinterpret_cast<AtomicTagged_t*>(field_base + kTaggedPayloadOffset));
+#ifdef V8_COMPRESS_POINTERS
+  reinterpret_cast<AtomicTagged_t*>(&snapshot)[1] =
+      AsAtomicTagged::Relaxed_Load(
+          reinterpret_cast<AtomicTagged_t*>(field_base + kRawPayloadOffset));
+#endif  // V8_COMPRESS_POINTERS
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/embedder-data-slot.h

@@ -32,9 +32,16 @@ class Object;
 class EmbedderDataSlot
     : public SlotBase<EmbedderDataSlot, Address, kTaggedSize> {
  public:
+  using EmbedderDataSlotSnapshot = Address;
+  V8_INLINE static void PopulateEmbedderDataSnapshot(Map map,
+                                                     JSObject js_object,
+                                                     int entry_index,
+                                                     EmbedderDataSlotSnapshot&);
+
   EmbedderDataSlot() : SlotBase(kNullAddress) {}
   V8_INLINE EmbedderDataSlot(EmbedderDataArray array, int entry_index);
   V8_INLINE EmbedderDataSlot(JSObject object, int embedder_field_index);
+  V8_INLINE explicit EmbedderDataSlot(const EmbedderDataSlotSnapshot& snapshot);
 
 #if defined(V8_TARGET_BIG_ENDIAN) && defined(V8_COMPRESS_POINTERS)
   static constexpr int kTaggedPayloadOffset = kTaggedSize;

test/cctest/test-api.cc

@@ -7841,6 +7841,12 @@ void CheckInternalFields(
 }
 
 void InternalFieldCallback(bool global_gc) {
+  // Manual GC scope as --stress-incremental-marking starts marking early and
+  // setting internal pointer fields mark the object for a heap layout change,
+  // which prevents it from being reclaimed and the callbacks from being
+  // executed.
+  ManualGCScope manual_gc_scope;
+
   LocalContext env;
   v8::Isolate* isolate = env->GetIsolate();
   v8::HandleScope scope(isolate);

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

@@ -63,9 +63,10 @@ TEST_F(UnifiedHeapTest, FindingV8ToBlinkReference) {
   v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
   v8::Context::Scope context_scope(context);
   uint16_t wrappable_type = WrapperHelper::kTracedEmbedderId;
-  v8::Local<v8::Object> api_object = WrapperHelper::CreateWrapper(
-      context, &wrappable_type,
-      cppgc::MakeGarbageCollected<Wrappable>(allocation_handle()));
+  auto* wrappable_object =
+      cppgc::MakeGarbageCollected<Wrappable>(allocation_handle());
+  v8::Local<v8::Object> api_object =
+      WrapperHelper::CreateWrapper(context, &wrappable_type, wrappable_object);
   Wrappable::destructor_callcount = 0;
   EXPECT_FALSE(api_object.IsEmpty());
   EXPECT_EQ(0u, Wrappable::destructor_callcount);