From bb0bd36b6bc8d26122cb11a48cbadf6f1b982a13 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 20:28:17 +0100 Subject: [PATCH 1/6] src: guard slightly costly check in MakeCallback more strongly --- src/api/callback.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index fb0e5586eb400a..4d2675d501d1b5 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -64,8 +64,15 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - // If you hit this assertion, you forgot to enter the v8::Context first. - CHECK_EQ(Environment::GetCurrent(isolate), env); + Local current_context = isolate->GetCurrentContext(); + // If you hit this assertion, the caller forgot to enter the right Node.js + // Environment's v8::Context first. + // We first check `env->context() != current_context` because the contexts + // likely *are* the same, in which case we can skip the slightly more + // expensive Environment::GetCurrent() call. + if (UNLIKELY(env->context() != current_context)) { + CHECK_EQ(Environment::GetCurrent(isolate), env); + } env->isolate()->SetIdle(false); From f97d04eca40eaf0a47e24da0b995409c20880554 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 20:28:51 +0100 Subject: [PATCH 2/6] src: store native async execution resources as `v8::Local` 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`. --- src/env-inl.h | 5 +++-- src/env.cc | 23 +++++++++++++++-------- src/env.h | 7 +++++-- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 04d4edc90b50d1..5499861181bed6 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, @@ -159,7 +159,8 @@ inline void AsyncHooks::push_async_context(double async_id, // these JS resource objects alive longer than necessary. 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..8dc8fc3a502ce4 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1096,20 +1096,27 @@ 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) { 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; } @@ -1157,7 +1164,7 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local context, 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())); + 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; From 853853fc7c5aeae20654b758d3a874ce981ea11f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 20:51:42 +0100 Subject: [PATCH 3/6] src: split out async stack corruption detection from inline fn This is fairly expensive code that unnecessarily bloats the contents of the inline function. --- src/env-inl.h | 18 ++++-------------- src/env.cc | 15 +++++++++++++++ src/env.h | 2 ++ 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 5499861181bed6..76f73e3b73eb42 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -168,25 +168,15 @@ inline void AsyncHooks::push_async_context(double async_id, inline bool AsyncHooks::pop_async_context(double async_id) { // In case of an exception then this may have already been reset, if the // stack was multiple MakeCallback()'s deep. - if (fields_[kStackLength] == 0) return false; + if (UNLIKELY(fields_[kStackLength] == 0)) return false; // Ask for the async_id to be restored as a check that the stack // hasn't been corrupted. // Since async_hooks is experimental, do only perform the check // when async_hooks is enabled. - if (fields_[kCheck] > 0 && async_id_fields_[kExecutionAsyncId] != async_id) { - fprintf(stderr, - "Error: async hook stack has become corrupted (" - "actual: %.f, expected: %.f)\n", - async_id_fields_.GetValue(kExecutionAsyncId), - async_id); - DumpBacktrace(stderr); - fflush(stderr); - if (!env()->abort_on_uncaught_exception()) - exit(1); - fprintf(stderr, "\n"); - fflush(stderr); - ABORT_NO_BACKTRACE(); + if (UNLIKELY(fields_[kCheck] > 0 && + async_id_fields_[kExecutionAsyncId] != async_id)) { + FailWithCorruptedAsyncStack(async_id); } uint32_t offset = fields_[kStackLength] - 1; diff --git a/src/env.cc b/src/env.cc index 8dc8fc3a502ce4..7c40603dfbdedf 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1192,6 +1192,21 @@ void AsyncHooks::grow_async_ids_stack() { async_ids_stack_.GetJSArray()).Check(); } +void AsyncHooks::FailWithCorruptedAsyncStack(double expected_async_id) { + fprintf(stderr, + "Error: async hook stack has become corrupted (" + "actual: %.f, expected: %.f)\n", + async_id_fields_.GetValue(kExecutionAsyncId), + expected_async_id); + DumpBacktrace(stderr); + fflush(stderr); + if (!env()->abort_on_uncaught_exception()) + exit(1); + fprintf(stderr, "\n"); + fflush(stderr); + ABORT_NO_BACKTRACE(); +} + void Environment::Exit(int exit_code) { if (options()->trace_exit) { HandleScope handle_scope(isolate()); diff --git a/src/env.h b/src/env.h index 63724b07a6b3a9..5af2eaa6669f0b 100644 --- a/src/env.h +++ b/src/env.h @@ -774,6 +774,8 @@ class AsyncHooks : public MemoryRetainer { friend class Environment; // So we can call the constructor. explicit AsyncHooks(v8::Isolate* isolate, const SerializeInfo* info); + [[noreturn]] void FailWithCorruptedAsyncStack(double expected_async_id); + // Stores the ids of the current execution context stack. AliasedFloat64Array async_ids_stack_; // Attached to a Uint32Array that tracks the number of active hooks for From b98e4bd20d2f7d412bf61fae4b005a35bd710a06 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 23:34:46 +0100 Subject: [PATCH 4/6] fixup! src: guard slightly costly check in MakeCallback more strongly --- src/api/callback.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index 4d2675d501d1b5..1287eb466fddd3 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -74,7 +74,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, CHECK_EQ(Environment::GetCurrent(isolate), env); } - env->isolate()->SetIdle(false); + isolate->SetIdle(false); env->async_hooks()->push_async_context( async_context_.async_id, async_context_.trigger_async_id, object); From 069373eada54b8f168c50089d94f73f9cf0a66e3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 Dec 2021 23:35:41 +0100 Subject: [PATCH 5/6] fixup! src: store native async execution resources as `v8::Local` --- src/env-inl.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 76f73e3b73eb42..e679780900abc9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -154,9 +154,7 @@ 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); // Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>! From a7787bbcabeeb8b63889c7baffdabb208ace999e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 28 Dec 2021 18:46:39 +0100 Subject: [PATCH 6/6] fixup! src: store native async execution resources as `v8::Local` --- src/env.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/env.cc b/src/env.cc index 7c40603dfbdedf..e3e06598a57529 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1113,6 +1113,8 @@ void AsyncHooks::Deserialize(Local context) { // 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(); @@ -1162,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]); + 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());