From 1aa6ac738d469cc386e869e3f9f480e41eaa15de Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 1 Sep 2025 17:24:25 +0200 Subject: [PATCH 1/2] src: track async resources via pointers to stack-allocated handles This addresses an existing `TODO` to remove the need for a separate `LocalVector`. Since all relevant handles are already present on the stack, we can track their addresses instead of storing the objects in a separate container. --- src/api/callback.cc | 23 ++++++++++++++++------- src/async_wrap.cc | 2 +- src/env-inl.h | 6 ++++-- src/env.cc | 21 ++++++++++----------- src/env.h | 15 +++++---------- src/node_internals.h | 10 ++++++++-- src/node_task_queue.cc | 3 ++- 7 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index a7070c2516f1ad..3436bb4d798ecb 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -47,17 +47,26 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags) flags, async_wrap->context_frame()) {} -InternalCallbackScope::InternalCallbackScope(Environment* env, - Local object, - const async_context& asyncContext, - int flags, - Local context_frame) +InternalCallbackScope::InternalCallbackScope( + Environment* env, + std::variant, Local*> object, + const async_context& asyncContext, + int flags, + Local context_frame) : env_(env), async_context_(asyncContext), - object_(object), skip_hooks_(flags & kSkipAsyncHooks), skip_task_queues_(flags & kSkipTaskQueues) { CHECK_NOT_NULL(env); + + if (std::holds_alternative>(object)) { + object_storage_ = std::get>(object); + object_ = &object_storage_; + } else { + object_ = std::get*>(object); + CHECK_NOT_NULL(object_); + } + env->PushAsyncCallbackScope(); if (!env->can_call_into_js()) { @@ -84,7 +93,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, isolate, async_context_frame::exchange(isolate, context_frame)); env->async_hooks()->push_async_context( - async_context_.async_id, async_context_.trigger_async_id, object); + async_context_.async_id, async_context_.trigger_async_id, object_); pushed_ids_ = true; diff --git a/src/async_wrap.cc b/src/async_wrap.cc index d9b2c76ede38c0..3dba0373de71ee 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -278,7 +278,7 @@ void AsyncWrap::PushAsyncContext(const FunctionCallbackInfo& args) { // then the checks in push_async_ids() and pop_async_id() will. double async_id = args[0]->NumberValue(env->context()).FromJust(); double trigger_async_id = args[1]->NumberValue(env->context()).FromJust(); - env->async_hooks()->push_async_context(async_id, trigger_async_id, {}); + env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr); } diff --git a/src/env-inl.h b/src/env-inl.h index 8145e014207334..302dfe05475032 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -106,8 +106,10 @@ 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 native_execution_async_resources_[i]; + if (i >= native_execution_async_resources_.size() || + native_execution_async_resources_[i] == nullptr) + return {}; + return *native_execution_async_resources_[i]; } inline v8::Local AsyncHooks::provider_string(int idx) { diff --git a/src/env.cc b/src/env.cc index 0253004c80cb03..0a9133987f2025 100644 --- a/src/env.cc +++ b/src/env.cc @@ -122,7 +122,8 @@ void Environment::ResetPromiseHooks(Local init, // Remember to keep this code aligned with pushAsyncContext() in JS. void AsyncHooks::push_async_context(double async_id, double trigger_async_id, - Local resource) { + Local* resource) { + CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty()); // Since async_hooks is experimental, do only perform the check // when async_hooks is enabled. if (fields_[kCheck] > 0) { @@ -140,14 +141,14 @@ void AsyncHooks::push_async_context(double async_id, #ifdef DEBUG for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++) - CHECK(native_execution_async_resources_[i].IsEmpty()); + CHECK_NULL(native_execution_async_resources_[i]); #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. - if (!resource.IsEmpty()) { + if (resource != nullptr) { native_execution_async_resources_.resize(offset + 1); - // Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>! + // Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>! native_execution_async_resources_[offset] = resource; } } @@ -172,11 +173,11 @@ bool AsyncHooks::pop_async_context(double async_id) { fields_[kStackLength] = offset; if (offset < native_execution_async_resources_.size() && - !native_execution_async_resources_[offset].IsEmpty()) [[likely]] { + native_execution_async_resources_[offset] != nullptr) [[likely]] { #ifdef DEBUG for (uint32_t i = offset + 1; i < native_execution_async_resources_.size(); i++) { - CHECK(native_execution_async_resources_[i].IsEmpty()); + CHECK_NULL(native_execution_async_resources_[i]); } #endif native_execution_async_resources_.resize(offset); @@ -1721,7 +1722,6 @@ AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info) fields_(isolate, kFieldsCount, MAYBE_FIELD_PTR(info, fields)), async_id_fields_( isolate, kUidFieldsCount, MAYBE_FIELD_PTR(info, async_id_fields)), - native_execution_async_resources_(isolate), info_(info) { HandleScope handle_scope(isolate); if (info == nullptr) { @@ -1810,10 +1810,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local context, native_execution_async_resources_.size()); for (size_t i = 0; i < native_execution_async_resources_.size(); i++) { info.native_execution_async_resources[i] = - native_execution_async_resources_[i].IsEmpty() ? SIZE_MAX : - creator->AddData( - context, - native_execution_async_resources_[i]); + native_execution_async_resources_[i] == nullptr + ? SIZE_MAX + : creator->AddData(context, *native_execution_async_resources_[i]); } // At the moment, promise hooks are not supported in the startup snapshot. diff --git a/src/env.h b/src/env.h index 753dc5ec422d4d..783a6e74ead67b 100644 --- a/src/env.h +++ b/src/env.h @@ -59,6 +59,7 @@ #include #include #include +#include #include #include #include @@ -324,7 +325,7 @@ class AsyncHooks : public MemoryRetainer { // `pop_async_context()` or `clear_async_id_stack()` are called. void push_async_context(double async_id, double trigger_async_id, - v8::Local execution_async_resource); + v8::Local* execution_async_resource); bool pop_async_context(double async_id); void clear_async_id_stack(); // Used in fatal exceptions. @@ -386,15 +387,9 @@ class AsyncHooks : public MemoryRetainer { v8::Global js_execution_async_resources_; - // TODO(@jasnell): Note that this is technically illegal use of - // v8::Locals which should be kept on the stack. Here, the entries - // in this object grows and shrinks with the C stack, and entries - // will be in the right handle scopes, but v8::Locals are supposed - // to remain on the stack and not the heap. For general purposes - // this *should* be ok but may need to be looked at further should - // v8 become stricter in the future about v8::Locals being held in - // the stack. - v8::LocalVector native_execution_async_resources_; + // We avoid storing the handles directly here, because they are already + // properly allocated on the stack, we just need access to them here. + std::deque*> native_execution_async_resources_; // Non-empty during deserialization const SerializeInfo* info_ = nullptr; diff --git a/src/node_internals.h b/src/node_internals.h index 275534285ec28f..22d7139aee3881 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -245,9 +245,14 @@ class InternalCallbackScope { // compatibility issues, but it shouldn't.) kSkipTaskQueues = 2 }; + // You need to either guarantee that this `InternalCallbackScope` is + // stack-allocated itself, OR that `object` is a pointer to a stack-allocated + // `v8::Local` which outlives this scope (e.g. for the + // public `CallbackScope` which indirectly allocates an instance of + // this class for ABI stability purposes). InternalCallbackScope( Environment* env, - v8::Local object, + std::variant, v8::Local*> object, const async_context& asyncContext, int flags = kNoFlags, v8::Local context_frame = v8::Local()); @@ -263,7 +268,8 @@ class InternalCallbackScope { private: Environment* env_; async_context async_context_; - v8::Local object_; + v8::Local object_storage_; + v8::Local* object_; bool skip_hooks_; bool skip_task_queues_; bool failed_ = false; diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 0a5aba6e31fa79..c4257110d8b520 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -100,10 +100,11 @@ void PromiseRejectCallback(PromiseRejectMessage message) { if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol()) .To(&trigger_async_id)) return; + Local promise_as_obj = promise; if (async_id != AsyncWrap::kInvalidAsyncId && trigger_async_id != AsyncWrap::kInvalidAsyncId) { env->async_hooks()->push_async_context( - async_id, trigger_async_id, promise); + async_id, trigger_async_id, &promise_as_obj); } USE(callback->Call( From 226e529664437af01d788b64381241c95d182ee8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 1 Sep 2025 21:48:55 +0200 Subject: [PATCH 2/2] fixup! src: track async resources via pointers to stack-allocated handles --- src/node_internals.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_internals.h b/src/node_internals.h index 22d7139aee3881..12ea72b61b0a5e 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -37,6 +37,7 @@ #include #include +#include #include struct sockaddr;