From 98c15bef2c114fb587da853faf2b85b535ebaa06 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 29 May 2024 19:59:19 +0200 Subject: [PATCH] src: remove dependency on wrapper-descriptor-based CppHeap As V8 has moved away from wrapper-descriptor-based CppHeap, this patch: 1. Create the CppHeap without using wrapper descirptors. 2. Deprecates node::SetCppgcReference() in favor of v8::Object::Wrap() since the wrapper descriptor is no longer relevant. It is still kept as a compatibility layer for addons that need to also work on Node.js versions without v8::Object::Wrap(). --- src/env-inl.h | 25 --------------- src/env.cc | 49 +++++++++++------------------ src/env.h | 4 --- src/node.h | 26 +++++---------- test/addons/cppgc-object/binding.cc | 14 ++++----- test/cctest/test_cppgc.cc | 31 +++++++----------- 6 files changed, 45 insertions(+), 104 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 116cbf4c4d5..c771c165609 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -62,31 +62,6 @@ inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -inline void IsolateData::SetCppgcReference(v8::Isolate* isolate, - v8::Local object, - void* wrappable) { - v8::CppHeap* heap = isolate->GetCppHeap(); - CHECK_NOT_NULL(heap); - v8::WrapperDescriptor descriptor = heap->wrapper_descriptor(); - uint16_t required_size = std::max(descriptor.wrappable_instance_index, - descriptor.wrappable_type_index); - CHECK_GT(object->InternalFieldCount(), required_size); - - uint16_t* id_ptr = nullptr; - { - Mutex::ScopedLock lock(isolate_data_mutex_); - auto it = - wrapper_data_map_.find(descriptor.embedder_id_for_garbage_collected); - CHECK_NE(it, wrapper_data_map_.end()); - id_ptr = &(it->second->cppgc_id); - } - - object->SetAlignedPointerInInternalField(descriptor.wrappable_type_index, - id_ptr); - object->SetAlignedPointerInInternalField(descriptor.wrappable_instance_index, - wrappable); -} - inline uint16_t* IsolateData::embedder_id_for_cppgc() const { return &(wrapper_data_->cppgc_id); } diff --git a/src/env.cc b/src/env.cc index 799b36aaebd..f80ab25ddeb 100644 --- a/src/env.cc +++ b/src/env.cc @@ -23,6 +23,7 @@ #include "util-inl.h" #include "v8-cppgc.h" #include "v8-profiler.h" +#include "v8-sandbox.h" // v8::Object::Wrap(), v8::Object::Unwrap() #include #include @@ -71,7 +72,6 @@ using v8::TryCatch; using v8::Uint32; using v8::Undefined; using v8::Value; -using v8::WrapperDescriptor; using worker::Worker; int const ContextEmbedderTag::kNodeContextTag = 0x6e6f64; @@ -533,6 +533,14 @@ void IsolateData::CreateProperties() { CreateEnvProxyTemplate(this); } +// Previously, the general convention of the wrappable layout for cppgc in +// the ecosystem is: +// [ 0 ] -> embedder id +// [ 1 ] -> wrappable instance +// Now V8 has deprecated this layout-based tracing enablement, embedders +// should simply use v8::Object::Wrap() and v8::Object::Unwrap(). We preserve +// this layout only to distinguish internally how the memory of a Node.js +// wrapper is managed or whether a wrapper is managed by Node.js. constexpr uint16_t kDefaultCppGCEmbedderID = 0x90de; Mutex IsolateData::isolate_data_mutex_; std::unordered_map> @@ -554,36 +562,16 @@ IsolateData::IsolateData(Isolate* isolate, v8::CppHeap* cpp_heap = isolate->GetCppHeap(); uint16_t cppgc_id = kDefaultCppGCEmbedderID; - if (cpp_heap != nullptr) { - // The general convention of the wrappable layout for cppgc in the - // ecosystem is: - // [ 0 ] -> embedder id - // [ 1 ] -> wrappable instance - // If the Isolate includes a CppHeap attached by another embedder, - // And if they also use the field 0 for the ID, we DCHECK that - // the layout matches our layout, and record the embedder ID for cppgc - // to avoid accidentally enabling cppgc on non-cppgc-managed wrappers . - v8::WrapperDescriptor descriptor = cpp_heap->wrapper_descriptor(); - if (descriptor.wrappable_type_index == BaseObject::kEmbedderType) { - cppgc_id = descriptor.embedder_id_for_garbage_collected; - DCHECK_EQ(descriptor.wrappable_instance_index, BaseObject::kSlot); - } - // If the CppHeap uses the slot we use to put non-cppgc-traced BaseObject - // for embedder ID, V8 could accidentally enable cppgc on them. So - // safe guard against this. - DCHECK_NE(descriptor.wrappable_type_index, BaseObject::kSlot); - } else { - cpp_heap_ = CppHeap::Create( - platform, - CppHeapCreateParams{ - {}, - WrapperDescriptor( - BaseObject::kEmbedderType, BaseObject::kSlot, cppgc_id)}); - isolate->AttachCppHeap(cpp_heap_.get()); - } // We do not care about overflow since we just want this to be different // from the cppgc id. uint16_t non_cppgc_id = cppgc_id + 1; + if (cpp_heap == nullptr) { + cpp_heap_ = CppHeap::Create(platform, v8::CppHeapCreateParams{{}}); + // TODO(joyeecheung): pass it into v8::Isolate::CreateParams and let V8 + // own it when we can keep the isolate registered/task runner discoverable + // during isolate disposal. + isolate->AttachCppHeap(cpp_heap_.get()); + } { // GC could still be run after the IsolateData is destroyed, so we store @@ -615,11 +603,12 @@ IsolateData::~IsolateData() { } } -// Public API +// Deprecated API, embedders should use v8::Object::Wrap() directly instead. void SetCppgcReference(Isolate* isolate, Local object, void* wrappable) { - IsolateData::SetCppgcReference(isolate, object, wrappable); + v8::Object::Wrap( + isolate, object, wrappable); } void IsolateData::MemoryInfo(MemoryTracker* tracker) const { diff --git a/src/env.h b/src/env.h index a864da46fb2..38d85e48215 100644 --- a/src/env.h +++ b/src/env.h @@ -165,10 +165,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { uint16_t* embedder_id_for_cppgc() const; uint16_t* embedder_id_for_non_cppgc() const; - static inline void SetCppgcReference(v8::Isolate* isolate, - v8::Local object, - void* wrappable); - inline uv_loop_t* event_loop() const; inline MultiIsolatePlatform* platform() const; inline const SnapshotData* snapshot_data() const; diff --git a/src/node.h b/src/node.h index 95e13cd9ea2..a987ebf9d71 100644 --- a/src/node.h +++ b/src/node.h @@ -1548,24 +1548,14 @@ void RegisterSignalHandler(int signal, bool reset_handler = false); #endif // _WIN32 -// Configure the layout of the JavaScript object with a cppgc::GarbageCollected -// instance so that when the JavaScript object is reachable, the garbage -// collected instance would have its Trace() method invoked per the cppgc -// contract. To make it work, the process must have called -// cppgc::InitializeProcess() before, which is usually the case for addons -// loaded by the stand-alone Node.js executable. Embedders of Node.js can use -// either need to call it themselves or make sure that -// ProcessInitializationFlags::kNoInitializeCppgc is *not* set for cppgc to -// work. -// If the CppHeap is owned by Node.js, which is usually the case for addon, -// the object must be created with at least two internal fields available, -// and the first two internal fields would be configured by Node.js. -// This may be superseded by a V8 API in the future, see -// https://bugs.chromium.org/p/v8/issues/detail?id=13960. Until then this -// serves as a helper for Node.js isolates. -NODE_EXTERN void SetCppgcReference(v8::Isolate* isolate, - v8::Local object, - void* wrappable); +// This is kept as a compatibility layer for addons to wrap cppgc-managed +// objects on Node.js versions without v8::Object::Wrap(). Addons created to +// work with only Node.js versions with v8::Object::Wrap() should use that +// instead. +NODE_DEPRECATED("Use v8::Object::Wrap()", + NODE_EXTERN void SetCppgcReference(v8::Isolate* isolate, + v8::Local object, + void* wrappable)); } // namespace node diff --git a/test/addons/cppgc-object/binding.cc b/test/addons/cppgc-object/binding.cc index 1b70ff11dc5..7fc16a87b84 100644 --- a/test/addons/cppgc-object/binding.cc +++ b/test/addons/cppgc-object/binding.cc @@ -1,8 +1,10 @@ +#include #include #include #include #include #include +#include #include #include @@ -15,8 +17,10 @@ class CppGCed : public cppgc::GarbageCollected { static void New(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); v8::Local js_object = args.This(); - CppGCed* gc_object = cppgc::MakeGarbageCollected( - isolate->GetCppHeap()->GetAllocationHandle()); + auto* heap = isolate->GetCppHeap(); + assert(heap != nullptr); + CppGCed* gc_object = + cppgc::MakeGarbageCollected(heap->GetAllocationHandle()); node::SetCppgcReference(isolate, js_object, gc_object); args.GetReturnValue().Set(js_object); } @@ -24,12 +28,6 @@ class CppGCed : public cppgc::GarbageCollected { static v8::Local GetConstructor( v8::Local context) { auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New); - auto ot = ft->InstanceTemplate(); - v8::WrapperDescriptor descriptor = - context->GetIsolate()->GetCppHeap()->wrapper_descriptor(); - uint16_t required_size = std::max(descriptor.wrappable_instance_index, - descriptor.wrappable_type_index); - ot->SetInternalFieldCount(required_size + 1); return ft->GetFunction(context).ToLocalChecked(); } diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index 49665174615..edd413ae9b9 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -3,16 +3,12 @@ #include #include #include +#include #include #include "node_test_fixture.h" // This tests that Node.js can work with an existing CppHeap. -// Mimic the Blink layout. -static int kWrappableTypeIndex = 0; -static int kWrappableInstanceIndex = 1; -static uint16_t kEmbedderID = 0x1; - // Mimic a class that does not know about Node.js. class CppGCed : public cppgc::GarbageCollected { public: @@ -23,12 +19,11 @@ class CppGCed : public cppgc::GarbageCollected { static void New(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); v8::Local js_object = args.This(); - CppGCed* gc_object = cppgc::MakeGarbageCollected( - isolate->GetCppHeap()->GetAllocationHandle()); - js_object->SetAlignedPointerInInternalField(kWrappableTypeIndex, - &kEmbedderID); - js_object->SetAlignedPointerInInternalField(kWrappableInstanceIndex, - gc_object); + auto* heap = isolate->GetCppHeap(); + CHECK_NOT_NULL(heap); + CppGCed* gc_object = + cppgc::MakeGarbageCollected(heap->GetAllocationHandle()); + node::SetCppgcReference(isolate, js_object, gc_object); kConstructCount++; args.GetReturnValue().Set(js_object); } @@ -36,8 +31,6 @@ class CppGCed : public cppgc::GarbageCollected { static v8::Local GetConstructor( v8::Local context) { auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New); - auto ot = ft->InstanceTemplate(); - ot->SetInternalFieldCount(2); return ft->GetFunction(context).ToLocalChecked(); } @@ -58,12 +51,12 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { // Create and attach the CppHeap before we set up the IsolateData so that // it recognizes the existing heap. - std::unique_ptr cpp_heap = v8::CppHeap::Create( - platform.get(), - v8::CppHeapCreateParams( - {}, - v8::WrapperDescriptor( - kWrappableTypeIndex, kWrappableInstanceIndex, kEmbedderID))); + std::unique_ptr cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}); + + // TODO(joyeecheung): pass it into v8::Isolate::CreateParams and let V8 + // own it when we can keep the isolate registered/task runner discoverable + // during isolate disposal. isolate->AttachCppHeap(cpp_heap.get()); // Try creating Context + IsolateData + Environment.