From 52ce477749992c11bcab3d46b53d3bd3b1253bf2 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Fri, 17 Mar 2023 22:42:33 +0000 Subject: [PATCH 1/5] src: stop copying code cache The code cache is quite large - around 1.3 MiB. Change the code to use non-owning buffers to avoid copying it. For starting up an otherwise empty main isolate, this saves around 1.3 MiB of unique set size memory (9.9 MiB -> 8.6 MiB) and 1.1ms elapsed time (22.9 ms -> 21.8 ms). Copying the code cache is unnecessary since: 1. for the builtin snapshot, the code cache data has static lifetime. 2. for non-builtin snapshots, we create copies of the code cache data in `SnapshotDeserializer::ReadVector`. These copies are owned by the `Environment` (through `IsolateData` -> `SnapshotData`), so they won't be deallocated. 3. a worker thread can copy a parent's isolate's code cache, but in that case we still know that the parent isolate will outlive the worker isolate. (Admittedly point (2) feels a little fragile from a lifetime perspective, and I would be happy to restrict this optimization to the builtin snapshot.) ```console $ perf stat -r 100 -e ... ./node -e 0 Performance counter stats for './node -e 0' (100 runs): 21.78 msec task-clock 2760 page-faults 113161604 instructions 18437648 branches 423230 branch-misses 853093 cache-references 41474 cache-misses 0.0225473 +- 0.0000504 seconds time elapsed ( +- 0.22% ) $ perf stat -r 100 -e ... ./node-main -e 0 Performance counter stats for './node-main -e 0' (100 runs): 22.91 msec task-clock 3102 page-faults 114890673 instructions 18751329 branches 428909 branch-misses 895721 cache-references 45202 cache-misses 0.0233760 +- 0.0000741 seconds time elapsed ( +- 0.32% ) ``` --- src/node_builtins.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 776ac8f2358aca..f7bd8c631ba693 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -501,14 +501,16 @@ void BuiltinLoader::CopyCodeCache(std::vector* out) const { void BuiltinLoader::RefreshCodeCache(const std::vector& in) { RwLock::ScopedLock lock(code_cache_->mutex); + code_cache_->map.reserve(in.size()); + CHECK_EQ(code_cache_->map.size(), 0); for (auto const& item : in) { - size_t length = item.data.size(); - uint8_t* buffer = new uint8_t[length]; - memcpy(buffer, item.data.data(), length); auto new_cache = std::make_unique( - buffer, length, v8::ScriptCompiler::CachedData::BufferOwned); + item.data.data(), + item.data.size(), + v8::ScriptCompiler::CachedData::BufferNotOwned); code_cache_->map[item.id] = std::move(new_cache); } + CHECK_EQ(code_cache_->map.size(), in.size()); code_cache_->has_code_cache = true; } From 7c6b5af5b29f426c2c9787c9e3a1317681b374e1 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Sun, 19 Mar 2023 00:13:17 +0000 Subject: [PATCH 2/5] fixup! src: stop copying code cache --- src/node_builtins.cc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node_builtins.cc b/src/node_builtins.cc index f7bd8c631ba693..890c5d36444163 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -502,15 +502,16 @@ void BuiltinLoader::CopyCodeCache(std::vector* out) const { void BuiltinLoader::RefreshCodeCache(const std::vector& in) { RwLock::ScopedLock lock(code_cache_->mutex); code_cache_->map.reserve(in.size()); - CHECK_EQ(code_cache_->map.size(), 0); + DCHECK(code_cache_->map.empty()); for (auto const& item : in) { - auto new_cache = std::make_unique( - item.data.data(), - item.data.size(), - v8::ScriptCompiler::CachedData::BufferNotOwned); - code_cache_->map[item.id] = std::move(new_cache); + auto result = code_cache_->map.emplace( + item.id, + std::make_unique( + item.data.data(), + item.data.size(), + v8::ScriptCompiler::CachedData::BufferNotOwned)); + DCHECK(result.second); } - CHECK_EQ(code_cache_->map.size(), in.size()); code_cache_->has_code_cache = true; } From 158062da274be1e150451f2c919ab0e5b6afd089 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Sun, 19 Mar 2023 00:17:25 +0000 Subject: [PATCH 3/5] fixup! src: stop copying code cache --- src/node_builtins.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 890c5d36444163..245b17b80adf4c 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -510,6 +510,7 @@ void BuiltinLoader::RefreshCodeCache(const std::vector& in) { item.data.data(), item.data.size(), v8::ScriptCompiler::CachedData::BufferNotOwned)); + USE(result.second); DCHECK(result.second); } code_cache_->has_code_cache = true; From a286b919f3146892d258e3cece081a9bd26161d8 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Sun, 19 Mar 2023 00:20:58 +0000 Subject: [PATCH 4/5] fixup! src: stop copying code cache --- src/node.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node.h b/src/node.h index e5b23f30d9530b..59461c17fa4853 100644 --- a/src/node.h +++ b/src/node.h @@ -505,6 +505,9 @@ struct IsolateSettings { // feature during the build step by passing the --disable-shared-readonly-heap // flag to the configure script. // +// The snapshot *must* be kept alive during the execution of the environment / +// isolate that it is derived from. +// // Snapshots are an *experimental* feature. In particular, the embedder API // exposed through this class is subject to change or removal between Node.js // versions, including possible API and ABI breakage. From eb99a865a0076b51581c57919e9ccfc7e1775c5f Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Mon, 10 Apr 2023 00:07:36 +0000 Subject: [PATCH 5/5] fixup! src: stop copying code cache --- src/node.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node.h b/src/node.h index 59461c17fa4853..c9b8960e85cec5 100644 --- a/src/node.h +++ b/src/node.h @@ -505,8 +505,8 @@ struct IsolateSettings { // feature during the build step by passing the --disable-shared-readonly-heap // flag to the configure script. // -// The snapshot *must* be kept alive during the execution of the environment / -// isolate that it is derived from. +// The snapshot *must* be kept alive during the execution of the Isolate +// that was created using it. // // Snapshots are an *experimental* feature. In particular, the embedder API // exposed through this class is subject to change or removal between Node.js