From 250e197a62e5f3c5438cfbe4f9d6e26fc8823817 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 20:28:51 +0100 Subject: [PATCH] src: store native async execution resources as `v8::Local` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is possible because the async stack is always expected to match the native call stack, and saves performance overhead that comes from the usage of `v8::Global`. PR-URL: https://github.com/nodejs/node/pull/41331 Reviewed-By: Gerhard Stöbich Reviewed-By: Tobias Nießen Reviewed-By: Darshan Sen --- src/env-inl.h | 9 ++++----- src/env.cc | 31 +++++++++++++++++++++---------- src/env.h | 7 +++++-- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 04d4edc90b50d1..9b2c536af96980 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -94,7 +94,7 @@ v8::Local AsyncHooks::js_execution_async_resources() { v8::Local AsyncHooks::native_execution_async_resource(size_t i) { if (i >= native_execution_async_resources_.size()) return {}; - return PersistentToLocal::Strong(native_execution_async_resources_[i]); + return native_execution_async_resources_[i]; } inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, @@ -154,12 +154,11 @@ inline void AsyncHooks::push_async_context(double async_id, #endif // When this call comes from JS (as a way of increasing the stack size), - // `resource` will be empty, because JS caches these values anyway, and - // we should avoid creating strong global references that might keep - // these JS resource objects alive longer than necessary. + // `resource` will be empty, because JS caches these values anyway. if (!resource.IsEmpty()) { native_execution_async_resources_.resize(offset + 1); - native_execution_async_resources_[offset].Reset(env()->isolate(), resource); + // Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>! + native_execution_async_resources_[offset] = resource; } } diff --git a/src/env.cc b/src/env.cc index bbc3f6a9a41d44..3f76c4bb770648 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1096,20 +1096,29 @@ void AsyncHooks::Deserialize(Local context) { async_ids_stack_.Deserialize(context); fields_.Deserialize(context); async_id_fields_.Deserialize(context); + + Local js_execution_async_resources; if (info_->js_execution_async_resources != 0) { - Local arr = context->GetDataFromSnapshotOnce( - info_->js_execution_async_resources) - .ToLocalChecked(); - js_execution_async_resources_.Reset(context->GetIsolate(), arr); + js_execution_async_resources = + context->GetDataFromSnapshotOnce( + info_->js_execution_async_resources).ToLocalChecked(); + } else { + js_execution_async_resources = Array::New(context->GetIsolate()); } + js_execution_async_resources_.Reset( + context->GetIsolate(), js_execution_async_resources); - native_execution_async_resources_.resize( - info_->native_execution_async_resources.size()); + // The native_execution_async_resources_ field requires v8::Local<> instances + // for async calls whose resources were on the stack as JS objects when they + // were entered. We cannot recreate this here; however, storing these values + // on the JS equivalent gives the same result, so we do that instead. for (size_t i = 0; i < info_->native_execution_async_resources.size(); ++i) { + if (info_->native_execution_async_resources[i] == SIZE_MAX) + continue; Local obj = context->GetDataFromSnapshotOnce( info_->native_execution_async_resources[i]) .ToLocalChecked(); - native_execution_async_resources_[i].Reset(context->GetIsolate(), obj); + js_execution_async_resources->Set(context, i, obj).Check(); } info_ = nullptr; } @@ -1155,9 +1164,11 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local context, info.native_execution_async_resources.resize( native_execution_async_resources_.size()); for (size_t i = 0; i < native_execution_async_resources_.size(); i++) { - info.native_execution_async_resources[i] = creator->AddData( - context, - native_execution_async_resources_[i].Get(context->GetIsolate())); + info.native_execution_async_resources[i] = + native_execution_async_resources_[i].IsEmpty() ? SIZE_MAX : + creator->AddData( + context, + native_execution_async_resources_[i]); } CHECK_EQ(contexts_.size(), 1); CHECK_EQ(contexts_[0], env()->context()); diff --git a/src/env.h b/src/env.h index 5cf789f9bb7ee9..63724b07a6b3a9 100644 --- a/src/env.h +++ b/src/env.h @@ -719,8 +719,11 @@ class AsyncHooks : public MemoryRetainer { inline void no_force_checks(); inline Environment* env(); + // NB: This call does not take (co-)ownership of `execution_async_resource`. + // The lifetime of the `v8::Local<>` pointee must last until + // `pop_async_context()` or `clear_async_id_stack()` are called. inline void push_async_context(double async_id, double trigger_async_id, - v8::Local execution_async_resource_); + v8::Local execution_async_resource); inline bool pop_async_context(double async_id); inline void clear_async_id_stack(); // Used in fatal exceptions. @@ -782,7 +785,7 @@ class AsyncHooks : public MemoryRetainer { void grow_async_ids_stack(); v8::Global js_execution_async_resources_; - std::vector> native_execution_async_resources_; + std::vector> native_execution_async_resources_; // Non-empty during deserialization const SerializeInfo* info_ = nullptr;