From 9aa1afb52741498345ec483ff72932520c5050ff Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 29 May 2024 20:08:40 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/58070 Reviewed-By: Antoine du Hamel Reviewed-By: Darshan Sen Reviewed-By: Joyee Cheung Reviewed-By: Rafael Gonzaga --- src/api/embed_helpers.cc | 12 ++++++++---- src/api/environment.cc | 11 +++++++++++ src/env.cc | 20 +------------------- src/env.h | 5 ----- src/node.cc | 16 ++++++++-------- src/node.h | 17 ++++++++++++++++- src/node_contextify.cc | 3 ++- src/node_main_instance.cc | 3 ++- src/node_worker.cc | 8 +++++++- src/util.cc | 5 ++++- test/cctest/node_test_fixture.h | 2 +- test/cctest/test_cppgc.cc | 25 +++++++------------------ test/cctest/test_environment.cc | 7 +++++-- test/cctest/test_platform.cc | 5 ++++- test/embedding/embedtest.cc | 3 +++ test/fuzzers/fuzz_env.cc | 2 +- test/fuzzers/fuzz_strings.cc | 2 +- 17 files changed, 81 insertions(+), 65 deletions(-) diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index 6498c09ee97..03800832cd6 100644 --- a/src/api/embed_helpers.cc +++ b/src/api/embed_helpers.cc @@ -234,14 +234,18 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() { } bool platform_finished = false; - impl_->platform->AddIsolateFinishedCallback(isolate, [](void* data) { - *static_cast(data) = true; - }, &platform_finished); - impl_->platform->UnregisterIsolate(isolate); + impl_->platform->AddIsolateFinishedCallback( + isolate, + [](void* data) { + bool* ptr = static_cast(data); + *ptr = true; + }, + &platform_finished); if (impl_->snapshot_creator.has_value()) { impl_->snapshot_creator.reset(); } isolate->Dispose(); + impl_->platform->UnregisterIsolate(isolate); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) diff --git a/src/api/environment.cc b/src/api/environment.cc index 21cbfbfdf12..cdc0c5afb75 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -20,12 +20,15 @@ #if HAVE_INSPECTOR #include "inspector/worker_inspector.h" // ParentInspectorHandle #endif +#include "v8-cppgc.h" namespace node { using errors::TryCatchScope; using v8::Array; using v8::Boolean; using v8::Context; +using v8::CppHeap; +using v8::CppHeapCreateParams; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -326,6 +329,14 @@ Isolate* NewIsolate(Isolate::CreateParams* params, // so that the isolate can access the platform during initialization. platform->RegisterIsolate(isolate, event_loop); + // Ensure that there is always a CppHeap. + if (settings.cpp_heap == nullptr) { + params->cpp_heap = + CppHeap::Create(platform, CppHeapCreateParams{{}}).release(); + } else { + params->cpp_heap = settings.cpp_heap; + } + SetIsolateCreateParamsForNode(params); Isolate::Initialize(isolate, *params); diff --git a/src/env.cc b/src/env.cc index d58ab96971e..6edb0d45b27 100644 --- a/src/env.cc +++ b/src/env.cc @@ -44,8 +44,6 @@ using v8::BackingStore; using v8::BackingStoreInitializationMode; using v8::Boolean; using v8::Context; -using v8::CppHeap; -using v8::CppHeapCreateParams; using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::ExternalMemoryAccounter; @@ -580,19 +578,10 @@ IsolateData::IsolateData(Isolate* isolate, platform_(platform), snapshot_data_(snapshot_data), options_(std::move(options)) { - v8::CppHeap* cpp_heap = isolate->GetCppHeap(); - uint16_t cppgc_id = kDefaultCppGCEmbedderID; // 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 @@ -616,14 +605,7 @@ IsolateData::IsolateData(Isolate* isolate, } } -IsolateData::~IsolateData() { - if (cpp_heap_ != nullptr) { - v8::Locker locker(isolate_); - // The CppHeap must be detached before being terminated. - isolate_->DetachCppHeap(); - cpp_heap_->Terminate(); - } -} +IsolateData::~IsolateData() {} // Deprecated API, embedders should use v8::Object::Wrap() directly instead. void SetCppgcReference(Isolate* isolate, diff --git a/src/env.h b/src/env.h index 95f369a188f..b428f5937b9 100644 --- a/src/env.h +++ b/src/env.h @@ -68,10 +68,6 @@ #include #include -namespace v8 { -class CppHeap; -} - namespace node { namespace shadow_realm { @@ -246,7 +242,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.cc b/src/node.cc index 9f41dc84340..1941404f6fa 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1211,6 +1211,14 @@ InitializeOncePerProcessInternal(const std::vector& args, result->platform_ = per_process::v8_platform.Platform(); } + if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) { + v8::PageAllocator* allocator = nullptr; + if (result->platform_ != nullptr) { + allocator = result->platform_->GetPageAllocator(); + } + cppgc::InitializeProcess(allocator); + } + if (!(flags & ProcessInitializationFlags::kNoInitializeV8)) { V8::Initialize(); @@ -1220,14 +1228,6 @@ InitializeOncePerProcessInternal(const std::vector& args, absl::SetMutexDeadlockDetectionMode(absl::OnDeadlockCycle::kIgnore); } - if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) { - v8::PageAllocator* allocator = nullptr; - if (result->platform_ != nullptr) { - allocator = result->platform_->GetPageAllocator(); - } - cppgc::InitializeProcess(allocator); - } - #if NODE_USE_V8_WASM_TRAP_HANDLER bool use_wasm_trap_handler = !per_process::cli_options->disable_wasm_trap_handler; diff --git a/src/node.h b/src/node.h index 8c72fecaa04..c2b6148c783 100644 --- a/src/node.h +++ b/src/node.h @@ -447,7 +447,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { // This function may only be called once per `Isolate`, and discard any // pending delayed tasks scheduled for that isolate. - // This needs to be called right before calling `Isolate::Dispose()`. + // This needs to be called right after calling `Isolate::Dispose()`. virtual void UnregisterIsolate(v8::Isolate* isolate) = 0; // The platform should call the passed function once all state associated @@ -489,6 +489,21 @@ struct IsolateSettings { allow_wasm_code_generation_callback = nullptr; v8::ModifyCodeGenerationFromStringsCallback2 modify_code_generation_from_strings_callback = nullptr; + + // When the settings is passed to NewIsolate(): + // - If cpp_heap is not nullptr, this CppHeap will be used to create + // the isolate and its ownership will be passed to V8. + // - If this is nullptr, Node.js will create a CppHeap that will be + // owned by V8. + // + // When the settings is passed to SetIsolateUpForNode(): + // cpp_heap will be ignored. Embedders must ensure that the + // v8::Isolate has a CppHeap attached while it's still used by + // Node.js, for example using v8::CreateParams. + // + // See https://issues.chromium.org/issues/42203693. In future version + // of V8, this CppHeap will be created by V8 if not provided. + v8::CppHeap* cpp_heap = nullptr; }; // Represents a startup snapshot blob, e.g. created by passing diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 4eee89d3c01..9669961c6ef 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -328,7 +328,7 @@ ContextifyContext* ContextifyContext::New(Local v8_context, .ToLocal(&wrapper)) { return {}; } - + DCHECK_NOT_NULL(env->isolate()->GetCppHeap()); result = cppgc::MakeGarbageCollected( env->isolate()->GetCppHeap()->GetAllocationHandle(), env, @@ -975,6 +975,7 @@ void ContextifyScript::RegisterExternalReferences( ContextifyScript* ContextifyScript::New(Environment* env, Local object) { + DCHECK_NOT_NULL(env->isolate()->GetCppHeap()); return cppgc::MakeGarbageCollected( env->isolate()->GetCppHeap()->GetAllocationHandle(), env, object); } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 4119ac1b002..2d20b8d3a99 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -81,9 +81,10 @@ NodeMainInstance::~NodeMainInstance() { // This should only be done on a main instance that owns its isolate. // IsolateData must be freed before UnregisterIsolate() is called. isolate_data_.reset(); - platform_->UnregisterIsolate(isolate_); } + // TODO(joyeecheung): split Isolate::Free() here? isolate_->Dispose(); + platform_->UnregisterIsolate(isolate_); } ExitCode NodeMainInstance::Run() { diff --git a/src/node_worker.cc b/src/node_worker.cc index 25be2bca2cd..4b22f1428cd 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -235,8 +235,14 @@ class WorkerThreadData { // new Isolate at the same address can successfully be registered with // the platform. // (Refs: https://github.com/nodejs/node/issues/30846) - w_->platform_->UnregisterIsolate(isolate); + // TODO(joyeecheung): we reversed the order because the task runner has + // to be available to handle GC tasks posted during isolate disposal. + // If this flakes comes back again, try splitting Isolate::Delete out + // so that the pointer is still available as map key after disposal + // and we delete it after unregistration. + // Refs: https://github.com/nodejs/node/pull/53086#issuecomment-2128056793 isolate->Dispose(); + w_->platform_->UnregisterIsolate(isolate); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) { diff --git a/src/util.cc b/src/util.cc index c8b468443c2..ff2d72e9b32 100644 --- a/src/util.cc +++ b/src/util.cc @@ -726,12 +726,15 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data) SnapshotBuilder::InitializeIsolateParams(data, ¶ms); } params.array_buffer_allocator = allocator_.get(); + params.cpp_heap = v8::CppHeap::Create(per_process::v8_platform.Platform(), + v8::CppHeapCreateParams{{}}) + .release(); Isolate::Initialize(isolate_, params); } RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() { - per_process::v8_platform.Platform()->UnregisterIsolate(isolate_); isolate_->Dispose(); + per_process::v8_platform.Platform()->UnregisterIsolate(isolate_); } RAIIIsolate::RAIIIsolate(const SnapshotData* data) diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 3414c0be8ad..82013ffe766 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -123,8 +123,8 @@ class NodeTestFixture : public NodeZeroIsolateTestFixture { void TearDown() override { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; NodeZeroIsolateTestFixture::TearDown(); } diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index edd413ae9b9..fef784e47b3 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -46,18 +46,12 @@ int CppGCed::kDestructCount = 0; int CppGCed::kTraceCount = 0; TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { - v8::Isolate* isolate = - node::NewIsolate(allocator.get(), ¤t_loop, platform.get()); - - // Create and attach the CppHeap before we set up the IsolateData so that - // it recognizes the existing heap. - std::unique_ptr 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()); + node::IsolateSettings settings; + settings.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); + v8::Isolate* isolate = node::NewIsolate( + allocator.get(), ¤t_loop, platform.get(), nullptr, settings); // Try creating Context + IsolateData + Environment. { @@ -100,15 +94,10 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { } platform->DrainTasks(isolate); - - // Cleanup. - isolate->DetachCppHeap(); - cpp_heap->Terminate(); - platform->DrainTasks(isolate); } - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); // Check that all the objects are created and destroyed properly. EXPECT_EQ(CppGCed::kConstructCount, 100); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 008cda77b65..7edf5d2e9c6 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -544,8 +544,8 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { node::FreeIsolateData(isolate_data); } - data->platform->UnregisterIsolate(isolate); isolate->Dispose(); + data->platform->UnregisterIsolate(isolate); uv_run(&loop, UV_RUN_DEFAULT); CHECK_EQ(uv_loop_close(&loop), 0); @@ -625,6 +625,9 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { // Allocate and initialize Isolate. v8::Isolate::CreateParams create_params; create_params.array_buffer_allocator = allocator.get(); + create_params.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); v8::Isolate* isolate = v8::Isolate::Allocate(); CHECK_NOT_NULL(isolate); platform->RegisterIsolate(isolate, ¤t_loop); @@ -675,8 +678,8 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { } // Cleanup. - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); } #endif // _WIN32 diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index 037b6b9cffa..d4a11d9404f 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -66,6 +66,9 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { // Allocate isolate v8::Isolate::CreateParams create_params; create_params.array_buffer_allocator = allocator.get(); + create_params.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); auto isolate = v8::Isolate::Allocate(); CHECK_NOT_NULL(isolate); @@ -104,8 +107,8 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { // Graceful shutdown delegate->Shutdown(); - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); } TEST_F(PlatformTest, TracingControllerNullptr) { diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index f481efc59a6..f0651ad8a85 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -2,6 +2,7 @@ #undef NDEBUG #endif #include +#include "cppgc/platform.h" #include "executable_wrapper.h" #include "node.h" @@ -43,6 +44,7 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { // support in the future, split this configuration out as a // command line option. node::ProcessInitializationFlags::kDisableNodeOptionsEnv, + node::ProcessInitializationFlags::kNoInitializeCppgc, }); for (const std::string& error : result->errors()) @@ -54,6 +56,7 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { std::unique_ptr platform = MultiIsolatePlatform::Create(4); V8::InitializePlatform(platform.get()); + cppgc::InitializeProcess(platform->GetPageAllocator()); V8::Initialize(); int ret = diff --git a/test/fuzzers/fuzz_env.cc b/test/fuzzers/fuzz_env.cc index bace3051f8c..5ca295848a9 100644 --- a/test/fuzzers/fuzz_env.cc +++ b/test/fuzzers/fuzz_env.cc @@ -65,8 +65,8 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } }; diff --git a/test/fuzzers/fuzz_strings.cc b/test/fuzzers/fuzz_strings.cc index 8f5e1a473e3..936876cdae2 100644 --- a/test/fuzzers/fuzz_strings.cc +++ b/test/fuzzers/fuzz_strings.cc @@ -72,8 +72,8 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } };