Skip to content

Commit

Permalink
src: remove dependency on wrapper-descriptor-based CppHeap
Browse files Browse the repository at this point in the history
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().
  • Loading branch information
joyeecheung authored and nodejs-github-bot committed Jul 6, 2024
1 parent 9a98687 commit a4731c9
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 104 deletions.
25 changes: 0 additions & 25 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,6 @@ inline uv_loop_t* IsolateData::event_loop() const {
return event_loop_;
}

inline void IsolateData::SetCppgcReference(v8::Isolate* isolate,
v8::Local<v8::Object> 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);
}
Expand Down
49 changes: 19 additions & 30 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <algorithm>
#include <atomic>
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
Expand All @@ -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
Expand Down Expand Up @@ -615,11 +603,12 @@ IsolateData::~IsolateData() {
}
}

// Public API
// Deprecated API, embedders should use v8::Object::Wrap() directly instead.
void SetCppgcReference(Isolate* isolate,
Local<Object> object,
void* wrappable) {
IsolateData::SetCppgcReference(isolate, object, wrappable);
v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
isolate, object, wrappable);
}

void IsolateData::MemoryInfo(MemoryTracker* tracker) const {
Expand Down
4 changes: 0 additions & 4 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> object,
void* wrappable);

inline uv_loop_t* event_loop() const;
inline MultiIsolatePlatform* platform() const;
inline const SnapshotData* snapshot_data() const;
Expand Down
26 changes: 8 additions & 18 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> 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<v8::Object> object,
void* wrappable));

} // namespace node

Expand Down
14 changes: 6 additions & 8 deletions test/addons/cppgc-object/binding.cc
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#include <assert.h>
#include <cppgc/allocation.h>
#include <cppgc/garbage-collected.h>
#include <cppgc/heap.h>
#include <node.h>
#include <v8-cppgc.h>
#include <v8-sandbox.h>
#include <v8.h>
#include <algorithm>

Expand All @@ -15,21 +17,17 @@ class CppGCed : public cppgc::GarbageCollected<CppGCed> {
static void New(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
v8::Local<v8::Object> js_object = args.This();
CppGCed* gc_object = cppgc::MakeGarbageCollected<CppGCed>(
isolate->GetCppHeap()->GetAllocationHandle());
auto* heap = isolate->GetCppHeap();
assert(heap != nullptr);
CppGCed* gc_object =
cppgc::MakeGarbageCollected<CppGCed>(heap->GetAllocationHandle());
node::SetCppgcReference(isolate, js_object, gc_object);
args.GetReturnValue().Set(js_object);
}

static v8::Local<v8::Function> GetConstructor(
v8::Local<v8::Context> 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();
}

Expand Down
31 changes: 12 additions & 19 deletions test/cctest/test_cppgc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,12 @@
#include <cppgc/heap.h>
#include <node.h>
#include <v8-cppgc.h>
#include <v8-sandbox.h>
#include <v8.h>
#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<CppGCed> {
public:
Expand All @@ -23,21 +19,18 @@ class CppGCed : public cppgc::GarbageCollected<CppGCed> {
static void New(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
v8::Local<v8::Object> js_object = args.This();
CppGCed* gc_object = cppgc::MakeGarbageCollected<CppGCed>(
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<CppGCed>(heap->GetAllocationHandle());
node::SetCppgcReference(isolate, js_object, gc_object);
kConstructCount++;
args.GetReturnValue().Set(js_object);
}

static v8::Local<v8::Function> GetConstructor(
v8::Local<v8::Context> context) {
auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New);
auto ot = ft->InstanceTemplate();
ot->SetInternalFieldCount(2);
return ft->GetFunction(context).ToLocalChecked();
}

Expand All @@ -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<v8::CppHeap> cpp_heap = v8::CppHeap::Create(
platform.get(),
v8::CppHeapCreateParams(
{},
v8::WrapperDescriptor(
kWrappableTypeIndex, kWrappableInstanceIndex, kEmbedderID)));
std::unique_ptr<v8::CppHeap> 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.
Expand Down

0 comments on commit a4731c9

Please sign in to comment.