From abce61b3d991bfab017a5e65a6d6312e79aa7730 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 17 May 2024 18:09:53 +0100 Subject: [PATCH] src: attach CppHeap to an v8::Isolate with isolate params Create a `CppHeap` in the `NodeMainInstance` instead and attach the `CppHeap` with `Isolate::CreateParams`. Existing cppgc addon tests should continue to work wihtout change. --- src/base_object.h | 1 + src/env.cc | 22 ++-------------------- src/env.h | 6 ------ src/node_main_instance.cc | 13 +++++++++++++ src/node_main_instance.h | 1 + 5 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/base_object.h b/src/base_object.h index b0ada1d7f38fed..11923610eeb9f5 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -44,6 +44,7 @@ class TransferData; class BaseObject : public MemoryRetainer { public: enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount }; + constexpr static uint16_t kDefaultCppGCEmebdderTypeID = 0x90de; // Associates this object with `object`. It uses the 1st internal field for // that, and in particular aborts if there is no such field. diff --git a/src/env.cc b/src/env.cc index 3d36416cdfa0f9..3580d105d60761 100644 --- a/src/env.cc +++ b/src/env.cc @@ -41,7 +41,6 @@ using v8::Array; using v8::Boolean; using v8::Context; using v8::CppHeap; -using v8::CppHeapCreateParams; using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::Function; @@ -520,7 +519,6 @@ void IsolateData::CreateProperties() { CreateEnvProxyTemplate(this); } -constexpr uint16_t kDefaultCppGCEmebdderID = 0x90de; Mutex IsolateData::isolate_data_mutex_; std::unordered_map> IsolateData::wrapper_data_map_; @@ -540,11 +538,11 @@ IsolateData::IsolateData(Isolate* isolate, new PerIsolateOptions(*(per_process::cli_options->per_isolate))); v8::CppHeap* cpp_heap = isolate->GetCppHeap(); - uint16_t cppgc_id = kDefaultCppGCEmebdderID; + uint16_t cppgc_id = BaseObject::kDefaultCppGCEmebdderTypeID; if (cpp_heap != nullptr) { // The general convention of the wrappable layout for cppgc in the // ecosystem is: - // [ 0 ] -> embedder id + // [ 0 ] -> embedder type 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 @@ -559,14 +557,6 @@ IsolateData::IsolateData(Isolate* isolate, // 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. @@ -594,14 +584,6 @@ IsolateData::IsolateData(Isolate* isolate, } } -IsolateData::~IsolateData() { - if (cpp_heap_ != nullptr) { - // The CppHeap must be detached before being terminated. - isolate_->DetachCppHeap(); - cpp_heap_->Terminate(); - } -} - // Public API void SetCppgcReference(Isolate* isolate, Local object, diff --git a/src/env.h b/src/env.h index 58419bd31f1d65..e6e5528e1fee95 100644 --- a/src/env.h +++ b/src/env.h @@ -67,10 +67,6 @@ #include #include -namespace v8 { -class CppHeap; -} - namespace node { namespace shadow_realm { @@ -145,7 +141,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { MultiIsolatePlatform* platform = nullptr, ArrayBufferAllocator* node_allocator = nullptr, const SnapshotData* snapshot_data = nullptr); - ~IsolateData(); SET_MEMORY_INFO_NAME(IsolateData) SET_SELF_SIZE(IsolateData) @@ -253,7 +248,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { const SnapshotData* snapshot_data_; std::optional snapshot_config_; - std::unique_ptr cpp_heap_; std::shared_ptr options_; worker::Worker* worker_context_ = nullptr; PerIsolateWrapperData* wrapper_data_; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 22b35e33e8fa50..558e91720b57db 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -25,10 +25,13 @@ namespace node { using v8::Context; +using v8::CppHeap; +using v8::CppHeapCreateParams; using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Locker; +using v8::WrapperDescriptor; NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, uv_loop_t* event_loop, @@ -38,12 +41,20 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, : args_(args), exec_args_(exec_args), array_buffer_allocator_(ArrayBufferAllocator::Create()), + cpp_heap_(CppHeap::Create( + platform, + CppHeapCreateParams{ + {}, + WrapperDescriptor(BaseObject::kEmbedderType, + BaseObject::kSlot, + BaseObject::kDefaultCppGCEmebdderTypeID)})), isolate_(nullptr), platform_(platform), isolate_data_(), isolate_params_(std::make_unique()), snapshot_data_(snapshot_data) { isolate_params_->array_buffer_allocator = array_buffer_allocator_.get(); + isolate_params_->cpp_heap = cpp_heap_.get(); isolate_ = NewIsolate(isolate_params_.get(), event_loop, platform, snapshot_data); @@ -83,6 +94,8 @@ NodeMainInstance::~NodeMainInstance() { isolate_data_.reset(); platform_->UnregisterIsolate(isolate_); } + // CppHeap is terminated by the isolate, no allocation is allowed at this + // point. isolate_->Dispose(); } diff --git a/src/node_main_instance.h b/src/node_main_instance.h index 2752ff4d0ebc07..fb19710a63ba75 100644 --- a/src/node_main_instance.h +++ b/src/node_main_instance.h @@ -54,6 +54,7 @@ class NodeMainInstance { std::vector args_; std::vector exec_args_; std::unique_ptr array_buffer_allocator_; + std::unique_ptr cpp_heap_; v8::Isolate* isolate_; MultiIsolatePlatform* platform_; std::unique_ptr isolate_data_;