From a871892b6fff3c9c3b162e5267d5b3e696fed060 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 12 Apr 2022 00:38:57 +0800 Subject: [PATCH 1/6] bootstrap: move embedded snapshot to SnapshotBuilder So that the embedded snapshot can be reused by the worker. --- src/node.cc | 2 +- src/node_external_reference.cc | 8 +++++--- src/node_main_instance.cc | 18 ++---------------- src/node_main_instance.h | 5 ----- src/node_snapshot_stub.cc | 4 ++-- src/node_snapshotable.cc | 26 ++++++++++++++++++++++---- src/node_snapshotable.h | 15 +++++++++++++++ 7 files changed, 47 insertions(+), 31 deletions(-) diff --git a/src/node.cc b/src/node.cc index b43f915c5d55a6..4850908f39900a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1171,7 +1171,7 @@ int Start(int argc, char** argv) { bool use_node_snapshot = per_process::cli_options->per_isolate->node_snapshot; const SnapshotData* snapshot_data = - use_node_snapshot ? NodeMainInstance::GetEmbeddedSnapshotData() + use_node_snapshot ? SnapshotBuilder::GetEmbeddedSnapshotData() : nullptr; uv_loop_configure(uv_default_loop(), UV_METRICS_IDLE_TIME); diff --git a/src/node_external_reference.cc b/src/node_external_reference.cc index 94198719b6a002..9a89977094bb55 100644 --- a/src/node_external_reference.cc +++ b/src/node_external_reference.cc @@ -7,9 +7,11 @@ namespace node { const std::vector& ExternalReferenceRegistry::external_references() { - CHECK(!is_finalized_); - external_references_.push_back(reinterpret_cast(nullptr)); - is_finalized_ = true; + if (!is_finalized_) { + external_references_.push_back(reinterpret_cast(nullptr)); + is_finalized_ = true; + } + return external_references_; } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 7167127c3dbcd3..04a942482f69cb 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -26,8 +26,6 @@ using v8::Isolate; using v8::Local; using v8::Locker; -std::unique_ptr NodeMainInstance::registry_ = - nullptr; NodeMainInstance::NodeMainInstance(Isolate* isolate, uv_loop_t* event_loop, MultiIsolatePlatform* platform, @@ -46,13 +44,6 @@ NodeMainInstance::NodeMainInstance(Isolate* isolate, SetIsolateMiscHandlers(isolate_, {}); } -const std::vector& NodeMainInstance::CollectExternalReferences() { - // Cannot be called more than once. - CHECK_NULL(registry_); - registry_.reset(new ExternalReferenceRegistry()); - return registry_->external_references(); -} - std::unique_ptr NodeMainInstance::Create( Isolate* isolate, uv_loop_t* event_loop, @@ -78,13 +69,8 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, snapshot_data_(snapshot_data) { isolate_params_->array_buffer_allocator = array_buffer_allocator_.get(); if (snapshot_data != nullptr) { - // TODO(joyeecheung): collect external references and set it in - // params.external_references. - const std::vector& external_references = - CollectExternalReferences(); - isolate_params_->external_references = external_references.data(); - isolate_params_->snapshot_blob = - const_cast(&(snapshot_data->blob)); + SnapshotBuilder::InitializeIsolateParams(snapshot_data, + isolate_params_.get()); } isolate_ = Isolate::Allocate(); diff --git a/src/node_main_instance.h b/src/node_main_instance.h index fa4c4db0a886fb..fc44bc9df6f030 100644 --- a/src/node_main_instance.h +++ b/src/node_main_instance.h @@ -65,11 +65,6 @@ class NodeMainInstance { DeleteFnPtr CreateMainEnvironment( int* exit_code); - // If nullptr is returned, the binary is not built with embedded - // snapshot. - static const SnapshotData* GetEmbeddedSnapshotData(); - static const std::vector& CollectExternalReferences(); - static const size_t kNodeContextIndex = 0; NodeMainInstance(const NodeMainInstance&) = delete; NodeMainInstance& operator=(const NodeMainInstance&) = delete; diff --git a/src/node_snapshot_stub.cc b/src/node_snapshot_stub.cc index f167c46a68de22..8d5b3cd67f9ff6 100644 --- a/src/node_snapshot_stub.cc +++ b/src/node_snapshot_stub.cc @@ -2,11 +2,11 @@ // NODE_WANT_INTERNALS, so we define it here manually. #define NODE_WANT_INTERNALS 1 -#include "node_main_instance.h" +#include "node_snapshotable.h" namespace node { -const SnapshotData* NodeMainInstance::GetEmbeddedSnapshotData() { +const SnapshotData* SnapshotBuilder::GetEmbeddedSnapshotData() { return nullptr; } diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 9330da6568ce20..33691557e65d7b 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -49,7 +49,7 @@ std::string FormatBlob(SnapshotData* data) { ss << R"(#include #include "env.h" -#include "node_main_instance.h" +#include "node_snapshotable.h" #include "v8.h" // This file is generated by tools/snapshot. Do not edit. @@ -78,11 +78,12 @@ SnapshotData snapshot_data { // -- isolate_data_indices ends -- // -- env_info begins -- )" << data->env_info - << R"( + << R"( // -- env_info ends -- }; -const SnapshotData* NodeMainInstance::GetEmbeddedSnapshotData() { +const SnapshotData* SnapshotBuilder::GetEmbeddedSnapshotData() { + Mutex::ScopedLock lock(snapshot_data_mutex_); return &snapshot_data; } } // namespace node @@ -91,6 +92,23 @@ const SnapshotData* NodeMainInstance::GetEmbeddedSnapshotData() { return ss.str(); } +std::unique_ptr SnapshotBuilder::registry_ = nullptr; +Mutex SnapshotBuilder::snapshot_data_mutex_; + +const std::vector& SnapshotBuilder::CollectExternalReferences() { + Mutex::ScopedLock lock(snapshot_data_mutex_); + if (registry_ == nullptr) { + registry_.reset(new ExternalReferenceRegistry()); + } + return registry_->external_references(); +} + +void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data, + Isolate::CreateParams* params) { + params->external_references = CollectExternalReferences().data(); + params->snapshot_blob = const_cast(&(data->blob)); +} + void SnapshotBuilder::Generate(SnapshotData* out, const std::vector args, const std::vector exec_args) { @@ -104,7 +122,7 @@ void SnapshotBuilder::Generate(SnapshotData* out, { const std::vector& external_references = - NodeMainInstance::CollectExternalReferences(); + CollectExternalReferences(); SnapshotCreator creator(isolate, external_references.data()); Environment* env; { diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index 30b684eb68a2d6..8b84a1096b5f44 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -5,6 +5,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "base_object.h" +#include "node_mutex.h" #include "util.h" namespace node { @@ -12,6 +13,7 @@ namespace node { class Environment; struct EnvSerializeInfo; struct SnapshotData; +class ExternalReferenceRegistry; #define SERIALIZABLE_OBJECT_TYPES(V) \ V(fs_binding_data, fs::BindingData) \ @@ -132,6 +134,19 @@ class SnapshotBuilder { static void Generate(SnapshotData* out, const std::vector args, const std::vector exec_args); + + // If nullptr is returned, the binary is not built with embedded + // snapshot. + static const SnapshotData* GetEmbeddedSnapshotData(); + static void InitializeIsolateParams(const SnapshotData* data, + v8::Isolate::CreateParams* params); + + private: + static const std::vector& CollectExternalReferences(); + + // Used to synchronize access to the snapshot data + static Mutex snapshot_data_mutex_; + static std::unique_ptr registry_; }; } // namespace node From 9eaebcd1221883ef82225bee80694dd74ad1a96d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 12 Apr 2022 01:55:55 +0800 Subject: [PATCH 2/6] bootstrap: use the isolate snapshot in workers --- src/node_worker.cc | 14 ++++++++++++++ .../parallel/test-worker-nearheaplimit-deadlock.js | 6 +++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index c9743fcf583a08..5ef1122388f645 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -146,6 +146,20 @@ class WorkerThreadData { SetIsolateCreateParamsForNode(¶ms); params.array_buffer_allocator_shared = allocator; + bool use_node_snapshot = true; + if (w_->per_isolate_opts_) { + use_node_snapshot = w_->per_isolate_opts_->node_snapshot; + } else { + // IsolateData is created after the Isolate is created so we'll + // inherit the option from the parent here. + use_node_snapshot = per_process::cli_options->per_isolate->node_snapshot; + } + const SnapshotData* snapshot_data = + use_node_snapshot ? SnapshotBuilder::GetEmbeddedSnapshotData() + : nullptr; + if (snapshot_data != nullptr) { + SnapshotBuilder::InitializeIsolateParams(snapshot_data, ¶ms); + } w->UpdateResourceConstraints(¶ms.constraints); Isolate* isolate = Isolate::Allocate(); diff --git a/test/parallel/test-worker-nearheaplimit-deadlock.js b/test/parallel/test-worker-nearheaplimit-deadlock.js index 1f38bc074da048..cf4c0d972c8719 100644 --- a/test/parallel/test-worker-nearheaplimit-deadlock.js +++ b/test/parallel/test-worker-nearheaplimit-deadlock.js @@ -10,7 +10,11 @@ if (!process.env.HAS_STARTED_WORKER) { resourceLimits: { maxYoungGenerationSizeMb: 0, maxOldGenerationSizeMb: 0 - } + }, + // With node snapshot the OOM can occur during the deserialization of + // the context, so disable it since we want the OOM to occur during + // the creation of the message port. + execArgv: [ ...process.execArgv, '--no-node-snapshot'] }; const worker = new Worker(__filename, opts); From ec5935adb507128e548b6983837aa48f7f00270a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 12 Apr 2022 02:35:53 +0800 Subject: [PATCH 3/6] test: fix calculations in test-worker-resource-limits The heap size limit should be the sum of old generation and young generation size limits, and does not solely depend on the limit of the old generation. --- test/parallel/test-worker-resource-limits.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js index ffda452f6c6335..f79c31b2a18793 100644 --- a/test/parallel/test-worker-resource-limits.js +++ b/test/parallel/test-worker-resource-limits.js @@ -35,10 +35,10 @@ if (!process.env.HAS_STARTED_WORKER) { assert.deepStrictEqual(resourceLimits, testResourceLimits); const array = []; while (true) { - // Leave 10% wiggle room here, and 20% on debug builds. - const wiggleRoom = common.buildType === 'Release' ? 1.1 : 1.2; const usedMB = v8.getHeapStatistics().used_heap_size / 1024 / 1024; - assert(usedMB < resourceLimits.maxOldGenerationSizeMb * wiggleRoom); + const maxReservedSize = resourceLimits.maxOldGenerationSizeMb + + resourceLimits.maxYoungGenerationSizeMb; + assert(usedMB < maxReservedSize); let seenSpaces = 0; for (const { space_name, space_size } of v8.getHeapSpaceStatistics()) { From 6b6ef66de4a14b431af4441854d7607f50d6531a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 19 Apr 2022 03:06:04 +0800 Subject: [PATCH 4/6] fixup! bootstrap: move embedded snapshot to SnapshotBuilder --- node.gyp | 1 + src/node.cc | 3 ++- src/node_main_instance.cc | 1 + src/node_snapshot_builder.h | 43 +++++++++++++++++++++++++++++++ src/node_snapshot_stub.cc | 2 +- src/node_snapshotable.cc | 3 ++- src/node_snapshotable.h | 25 ------------------ tools/snapshot/node_mksnapshot.cc | 2 +- 8 files changed, 51 insertions(+), 29 deletions(-) create mode 100644 src/node_snapshot_builder.h diff --git a/node.gyp b/node.gyp index 0a6eca012daad1..e748208adcde25 100644 --- a/node.gyp +++ b/node.gyp @@ -637,6 +637,7 @@ 'src/node_revert.h', 'src/node_root_certs.h', 'src/node_snapshotable.h', + 'src/node_snapshot_builder.h', 'src/node_sockaddr.h', 'src/node_sockaddr-inl.h', 'src/node_stat_watcher.h', diff --git a/src/node.cc b/src/node.cc index 4850908f39900a..892615f1d8eb30 100644 --- a/src/node.cc +++ b/src/node.cc @@ -25,8 +25,8 @@ #include "debug_utils-inl.h" #include "env-inl.h" -#include "memory_tracker-inl.h" #include "histogram-inl.h" +#include "memory_tracker-inl.h" #include "node_binding.h" #include "node_errors.h" #include "node_internals.h" @@ -38,6 +38,7 @@ #include "node_process-inl.h" #include "node_report.h" #include "node_revert.h" +#include "node_snapshot_builder.h" #include "node_v8_platform-inl.h" #include "node_version.h" diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 04a942482f69cb..e7218148ea044f 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -7,6 +7,7 @@ #include "node_external_reference.h" #include "node_internals.h" #include "node_options-inl.h" +#include "node_snapshot_builder.h" #include "node_snapshotable.h" #include "node_v8_platform-inl.h" #include "util-inl.h" diff --git a/src/node_snapshot_builder.h b/src/node_snapshot_builder.h new file mode 100644 index 00000000000000..183c39bdd76219 --- /dev/null +++ b/src/node_snapshot_builder.h @@ -0,0 +1,43 @@ + +#ifndef SRC_NODE_SNAPSHOT_BUILDER_H_ +#define SRC_NODE_SNAPSHOT_BUILDER_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include +#include "node_mutex.h" +#include "v8.h" + +namespace node { + +class ExternalReferenceRegistry; +struct SnapshotData; + +class SnapshotBuilder { + public: + static std::string Generate(const std::vector args, + const std::vector exec_args); + + // Generate the snapshot into out. + static void Generate(SnapshotData* out, + const std::vector args, + const std::vector exec_args); + + // If nullptr is returned, the binary is not built with embedded + // snapshot. + static const SnapshotData* GetEmbeddedSnapshotData(); + static void InitializeIsolateParams(const SnapshotData* data, + v8::Isolate::CreateParams* params); + + private: + // Used to synchronize access to the snapshot data + static Mutex snapshot_data_mutex_; + static const std::vector& CollectExternalReferences(); + + static std::unique_ptr registry_; +}; +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_SNAPSHOT_BUILDER_H_ diff --git a/src/node_snapshot_stub.cc b/src/node_snapshot_stub.cc index 8d5b3cd67f9ff6..664f878c0671db 100644 --- a/src/node_snapshot_stub.cc +++ b/src/node_snapshot_stub.cc @@ -2,7 +2,7 @@ // NODE_WANT_INTERNALS, so we define it here manually. #define NODE_WANT_INTERNALS 1 -#include "node_snapshotable.h" +#include "node_snapshot_builder.h" namespace node { diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 33691557e65d7b..256d8da7d0080e 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -12,6 +12,7 @@ #include "node_internals.h" #include "node_main_instance.h" #include "node_process.h" +#include "node_snapshot_builder.h" #include "node_v8.h" #include "node_v8_platform-inl.h" @@ -49,7 +50,7 @@ std::string FormatBlob(SnapshotData* data) { ss << R"(#include #include "env.h" -#include "node_snapshotable.h" +#include "node_snapshot_builder.h" #include "v8.h" // This file is generated by tools/snapshot. Do not edit. diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index 8b84a1096b5f44..f0a8bce215e027 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -5,7 +5,6 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "base_object.h" -#include "node_mutex.h" #include "util.h" namespace node { @@ -124,30 +123,6 @@ void SerializeBindingData(Environment* env, EnvSerializeInfo* info); bool IsSnapshotableType(FastStringKey key); - -class SnapshotBuilder { - public: - static std::string Generate(const std::vector args, - const std::vector exec_args); - - // Generate the snapshot into out. - static void Generate(SnapshotData* out, - const std::vector args, - const std::vector exec_args); - - // If nullptr is returned, the binary is not built with embedded - // snapshot. - static const SnapshotData* GetEmbeddedSnapshotData(); - static void InitializeIsolateParams(const SnapshotData* data, - v8::Isolate::CreateParams* params); - - private: - static const std::vector& CollectExternalReferences(); - - // Used to synchronize access to the snapshot data - static Mutex snapshot_data_mutex_; - static std::unique_ptr registry_; -}; } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/tools/snapshot/node_mksnapshot.cc b/tools/snapshot/node_mksnapshot.cc index 60062854327868..d166559a715b14 100644 --- a/tools/snapshot/node_mksnapshot.cc +++ b/tools/snapshot/node_mksnapshot.cc @@ -7,7 +7,7 @@ #include "libplatform/libplatform.h" #include "node_internals.h" -#include "node_snapshotable.h" +#include "node_snapshot_builder.h" #include "util-inl.h" #include "v8.h" From 5e28529210537b98509ca29d6e7af4d71f4d14ea Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 14 Apr 2022 21:51:44 +0800 Subject: [PATCH 5/6] fixup! bootstrap: move embedded snapshot to SnapshotBuilder --- src/node_snapshotable.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 256d8da7d0080e..1938e1b143ae2e 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -93,15 +93,11 @@ const SnapshotData* SnapshotBuilder::GetEmbeddedSnapshotData() { return ss.str(); } -std::unique_ptr SnapshotBuilder::registry_ = nullptr; Mutex SnapshotBuilder::snapshot_data_mutex_; const std::vector& SnapshotBuilder::CollectExternalReferences() { - Mutex::ScopedLock lock(snapshot_data_mutex_); - if (registry_ == nullptr) { - registry_.reset(new ExternalReferenceRegistry()); - } - return registry_->external_references(); + static auto registry = std::make_unique(); + return registry->external_references(); } void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data, From af0191ccc3c393cae712f07d2d40d7244403e9fe Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 19 Apr 2022 03:08:09 +0800 Subject: [PATCH 6/6] fixup! bootstrap: use the isolate snapshot in workers --- src/node_worker.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_worker.cc b/src/node_worker.cc index 5ef1122388f645..04cd60b2453543 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -7,6 +7,7 @@ #include "node_buffer.h" #include "node_options-inl.h" #include "node_perf.h" +#include "node_snapshot_builder.h" #include "util-inl.h" #include "async_wrap-inl.h"