+
Skip to content

Commit eae9a29

Browse files
joyeecheungRafaelGSS
joyeecheung
authored and
RafaelGSS
committed
src: use V8-owned CppHeap
As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time. 1. If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8. 2. If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams. See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate(). PR-URL: #58070 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
1 parent 087c254 commit eae9a29

17 files changed

+81
-65
lines changed

‎src/api/embed_helpers.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -234,14 +234,18 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() {
234234
}
235235

236236
bool platform_finished = false;
237-
impl_->platform->AddIsolateFinishedCallback(isolate, [](void* data) {
238-
*static_cast<bool*>(data) = true;
239-
}, &platform_finished);
240-
impl_->platform->UnregisterIsolate(isolate);
237+
impl_->platform->AddIsolateFinishedCallback(
238+
isolate,
239+
[](void* data) {
240+
bool* ptr = static_cast<bool*>(data);
241+
*ptr = true;
242+
},
243+
&platform_finished);
241244
if (impl_->snapshot_creator.has_value()) {
242245
impl_->snapshot_creator.reset();
243246
}
244247
isolate->Dispose();
248+
impl_->platform->UnregisterIsolate(isolate);
245249

246250
// Wait until the platform has cleaned up all relevant resources.
247251
while (!platform_finished)

‎src/api/environment.cc‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,15 @@
2020
#if HAVE_INSPECTOR
2121
#include "inspector/worker_inspector.h" // ParentInspectorHandle
2222
#endif
23+
#include "v8-cppgc.h"
2324

2425
namespace node {
2526
using errors::TryCatchScope;
2627
using v8::Array;
2728
using v8::Boolean;
2829
using v8::Context;
30+
using v8::CppHeap;
31+
using v8::CppHeapCreateParams;
2932
using v8::EscapableHandleScope;
3033
using v8::Function;
3134
using v8::FunctionCallbackInfo;
@@ -326,6 +329,14 @@ Isolate* NewIsolate(Isolate::CreateParams* params,
326329
// so that the isolate can access the platform during initialization.
327330
platform->RegisterIsolate(isolate, event_loop);
328331

332+
// Ensure that there is always a CppHeap.
333+
if (settings.cpp_heap == nullptr) {
334+
params->cpp_heap =
335+
CppHeap::Create(platform, CppHeapCreateParams{{}}).release();
336+
} else {
337+
params->cpp_heap = settings.cpp_heap;
338+
}
339+
329340
SetIsolateCreateParamsForNode(params);
330341
Isolate::Initialize(isolate, *params);
331342

‎src/env.cc‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ using v8::BackingStore;
4444
using v8::BackingStoreInitializationMode;
4545
using v8::Boolean;
4646
using v8::Context;
47-
using v8::CppHeap;
48-
using v8::CppHeapCreateParams;
4947
using v8::EmbedderGraph;
5048
using v8::EscapableHandleScope;
5149
using v8::ExternalMemoryAccounter;
@@ -580,19 +578,10 @@ IsolateData::IsolateData(Isolate* isolate,
580578
platform_(platform),
581579
snapshot_data_(snapshot_data),
582580
options_(std::move(options)) {
583-
v8::CppHeap* cpp_heap = isolate->GetCppHeap();
584-
585581
uint16_t cppgc_id = kDefaultCppGCEmbedderID;
586582
// We do not care about overflow since we just want this to be different
587583
// from the cppgc id.
588584
uint16_t non_cppgc_id = cppgc_id + 1;
589-
if (cpp_heap == nullptr) {
590-
cpp_heap_ = CppHeap::Create(platform, v8::CppHeapCreateParams{{}});
591-
// TODO(joyeecheung): pass it into v8::Isolate::CreateParams and let V8
592-
// own it when we can keep the isolate registered/task runner discoverable
593-
// during isolate disposal.
594-
isolate->AttachCppHeap(cpp_heap_.get());
595-
}
596585

597586
{
598587
// GC could still be run after the IsolateData is destroyed, so we store
@@ -616,14 +605,7 @@ IsolateData::IsolateData(Isolate* isolate,
616605
}
617606
}
618607

619-
IsolateData::~IsolateData() {
620-
if (cpp_heap_ != nullptr) {
621-
v8::Locker locker(isolate_);
622-
// The CppHeap must be detached before being terminated.
623-
isolate_->DetachCppHeap();
624-
cpp_heap_->Terminate();
625-
}
626-
}
608+
IsolateData::~IsolateData() {}
627609

628610
// Deprecated API, embedders should use v8::Object::Wrap() directly instead.
629611
void SetCppgcReference(Isolate* isolate,

‎src/env.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,6 @@
6868
#include <unordered_set>
6969
#include <vector>
7070

71-
namespace v8 {
72-
class CppHeap;
73-
}
74-
7571
namespace node {
7672

7773
namespace shadow_realm {
@@ -246,7 +242,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
246242
const SnapshotData* snapshot_data_;
247243
std::optional<SnapshotConfig> snapshot_config_;
248244

249-
std::unique_ptr<v8::CppHeap> cpp_heap_;
250245
std::shared_ptr<PerIsolateOptions> options_;
251246
worker::Worker* worker_context_ = nullptr;
252247
PerIsolateWrapperData* wrapper_data_;

‎src/node.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,14 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
12111211
result->platform_ = per_process::v8_platform.Platform();
12121212
}
12131213

1214+
if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) {
1215+
v8::PageAllocator* allocator = nullptr;
1216+
if (result->platform_ != nullptr) {
1217+
allocator = result->platform_->GetPageAllocator();
1218+
}
1219+
cppgc::InitializeProcess(allocator);
1220+
}
1221+
12141222
if (!(flags & ProcessInitializationFlags::kNoInitializeV8)) {
12151223
V8::Initialize();
12161224

@@ -1220,14 +1228,6 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
12201228
absl::SetMutexDeadlockDetectionMode(absl::OnDeadlockCycle::kIgnore);
12211229
}
12221230

1223-
if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) {
1224-
v8::PageAllocator* allocator = nullptr;
1225-
if (result->platform_ != nullptr) {
1226-
allocator = result->platform_->GetPageAllocator();
1227-
}
1228-
cppgc::InitializeProcess(allocator);
1229-
}
1230-
12311231
#if NODE_USE_V8_WASM_TRAP_HANDLER
12321232
bool use_wasm_trap_handler =
12331233
!per_process::cli_options->disable_wasm_trap_handler;

‎src/node.h‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
447447

448448
// This function may only be called once per `Isolate`, and discard any
449449
// pending delayed tasks scheduled for that isolate.
450-
// This needs to be called right before calling `Isolate::Dispose()`.
450+
// This needs to be called right after calling `Isolate::Dispose()`.
451451
virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
452452

453453
// The platform should call the passed function once all state associated
@@ -489,6 +489,21 @@ struct IsolateSettings {
489489
allow_wasm_code_generation_callback = nullptr;
490490
v8::ModifyCodeGenerationFromStringsCallback2
491491
modify_code_generation_from_strings_callback = nullptr;
492+
493+
// When the settings is passed to NewIsolate():
494+
// - If cpp_heap is not nullptr, this CppHeap will be used to create
495+
// the isolate and its ownership will be passed to V8.
496+
// - If this is nullptr, Node.js will create a CppHeap that will be
497+
// owned by V8.
498+
//
499+
// When the settings is passed to SetIsolateUpForNode():
500+
// cpp_heap will be ignored. Embedders must ensure that the
501+
// v8::Isolate has a CppHeap attached while it's still used by
502+
// Node.js, for example using v8::CreateParams.
503+
//
504+
// See https://issues.chromium.org/issues/42203693. In future version
505+
// of V8, this CppHeap will be created by V8 if not provided.
506+
v8::CppHeap* cpp_heap = nullptr;
492507
};
493508

494509
// Represents a startup snapshot blob, e.g. created by passing

‎src/node_contextify.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ ContextifyContext* ContextifyContext::New(Local<Context> v8_context,
328328
.ToLocal(&wrapper)) {
329329
return {};
330330
}
331-
331+
DCHECK_NOT_NULL(env->isolate()->GetCppHeap());
332332
result = cppgc::MakeGarbageCollected<ContextifyContext>(
333333
env->isolate()->GetCppHeap()->GetAllocationHandle(),
334334
env,
@@ -975,6 +975,7 @@ void ContextifyScript::RegisterExternalReferences(
975975

976976
ContextifyScript* ContextifyScript::New(Environment* env,
977977
Local<Object> object) {
978+
DCHECK_NOT_NULL(env->isolate()->GetCppHeap());
978979
return cppgc::MakeGarbageCollected<ContextifyScript>(
979980
env->isolate()->GetCppHeap()->GetAllocationHandle(), env, object);
980981
}

‎src/node_main_instance.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,10 @@ NodeMainInstance::~NodeMainInstance() {
8181
// This should only be done on a main instance that owns its isolate.
8282
// IsolateData must be freed before UnregisterIsolate() is called.
8383
isolate_data_.reset();
84-
platform_->UnregisterIsolate(isolate_);
8584
}
85+
// TODO(joyeecheung): split Isolate::Free() here?
8686
isolate_->Dispose();
87+
platform_->UnregisterIsolate(isolate_);
8788
}
8889

8990
ExitCode NodeMainInstance::Run() {

‎src/node_worker.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,14 @@ class WorkerThreadData {
235235
// new Isolate at the same address can successfully be registered with
236236
// the platform.
237237
// (Refs: https://github.com/nodejs/node/issues/30846)
238-
w_->platform_->UnregisterIsolate(isolate);
238+
// TODO(joyeecheung): we reversed the order because the task runner has
239+
// to be available to handle GC tasks posted during isolate disposal.
240+
// If this flakes comes back again, try splitting Isolate::Delete out
241+
// so that the pointer is still available as map key after disposal
242+
// and we delete it after unregistration.
243+
// Refs: https://github.com/nodejs/node/pull/53086#issuecomment-2128056793
239244
isolate->Dispose();
245+
w_->platform_->UnregisterIsolate(isolate);
240246

241247
// Wait until the platform has cleaned up all relevant resources.
242248
while (!platform_finished) {

‎src/util.cc‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,12 +726,15 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)
726726
SnapshotBuilder::InitializeIsolateParams(data, &params);
727727
}
728728
params.array_buffer_allocator = allocator_.get();
729+
params.cpp_heap = v8::CppHeap::Create(per_process::v8_platform.Platform(),
730+
v8::CppHeapCreateParams{{}})
731+
.release();
729732
Isolate::Initialize(isolate_, params);
730733
}
731734

732735
RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {
733-
per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
734736
isolate_->Dispose();
737+
per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
735738
}
736739

737740
RAIIIsolate::RAIIIsolate(const SnapshotData* data)

0 commit comments

Comments
 (0)
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载