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.
nodejs · May 17, 2024 · abce61b · abce61b
1 parent 9807ede
commit abce61b
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 26 deletions.
diff --git 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
@@ -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<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
     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> object,

diff --git a/src/env.h b/src/env.h
@@ -67,10 +67,6 @@
 #include <unordered_set>
 #include <vector>
 
-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<SnapshotConfig> snapshot_config_;
 
-  std::unique_ptr<v8::CppHeap> cpp_heap_;
   std::shared_ptr<PerIsolateOptions> options_;
   worker::Worker* worker_context_ = nullptr;
   PerIsolateWrapperData* wrapper_data_;

diff --git 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<Isolate::CreateParams>()),
       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
@@ -54,6 +54,7 @@ class NodeMainInstance {
   std::vector<std::string> args_;
   std::vector<std::string> exec_args_;
   std::unique_ptr<ArrayBufferAllocator> array_buffer_allocator_;
+  std::unique_ptr<v8::CppHeap> cpp_heap_;
   v8::Isolate* isolate_;
   MultiIsolatePlatform* platform_;
   std::unique_ptr<IsolateData> isolate_data_;