Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class TransferData;
class BaseObject : public MemoryRetainer {
public:
enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount };
constexpr static uint16_t kDefaultCppGCEmebdderTypeID = 0x90de;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constexpr static uint16_t kDefaultCppGCEmebdderTypeID = 0x90de;
constexpr static uint16_t kDefaultCppGCEmbedderTypeID = 0x90de;


// Associates this object with `object`. It uses the 1st internal field for
// that, and in particular aborts if there is no such field.
Expand Down
22 changes: 2 additions & 20 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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_;
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 0 additions & 6 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@
#include <unordered_set>
#include <vector>

namespace v8 {
class CppHeap;
}

namespace node {

namespace shadow_realm {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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_;
Expand Down
13 changes: 13 additions & 0 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)})),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are embedders expected to get the correct values such as kDefaultCppGCEmebdderTypeID? Shouldn't this be part of NewIsolate() or SetIsolateCreateParamsForNode()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, wrapper descriptors are going to be deprecated so that embedders don't have to care about what the value is: nodejs/node-v8#283.

Read more at https://docs.google.com/document/d/1-sBltmlx4yUIzmNHrRO9lKjqiN-uJ81gCmj9-Gq5d5w/edit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then this will 'just' attach a CppHeap with default options in the future anyway? That still feels like NewIsolate() or SetIsolateCreateParamsForNode() would be the right places for this, as the goal is to initialize a 'Node.js-flavored' Isolate instance, unless I'm missing something

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I turned this into a draft because I think the current form is not complete.

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);
Expand Down Expand Up @@ -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();
}

Expand Down
1 change: 1 addition & 0 deletions src/node_main_instance.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down