Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: improve MakeCallback() performance #41331

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,17 @@ 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<Context> 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);
isolate->SetIdle(false);

env->async_hooks()->push_async_context(
async_context_.async_id, async_context_.trigger_async_id, object);
Expand Down
27 changes: 8 additions & 19 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ v8::Local<v8::Array> AsyncHooks::js_execution_async_resources() {

v8::Local<v8::Object> 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];
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
}

inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init,
Expand Down Expand Up @@ -154,38 +154,27 @@ 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;
addaleax marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Remember to keep this code aligned with popAsyncContext() in JS.
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;
Expand Down
46 changes: 36 additions & 10 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1096,20 +1096,29 @@ void AsyncHooks::Deserialize(Local<Context> context) {
async_ids_stack_.Deserialize(context);
fields_.Deserialize(context);
async_id_fields_.Deserialize(context);

Local<Array> js_execution_async_resources;
if (info_->js_execution_async_resources != 0) {
Local<Array> arr = context->GetDataFromSnapshotOnce<Array>(
info_->js_execution_async_resources)
.ToLocalChecked();
js_execution_async_resources_.Reset(context->GetIsolate(), arr);
js_execution_async_resources =
context->GetDataFromSnapshotOnce<Array>(
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<Object> obj = context->GetDataFromSnapshotOnce<Object>(
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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung This code is effectively doing the same thing as before, but I did want to ask – what exactly are the semantics of the async stack after a snapshot? Is it expected that you start out in a state where there is an active async context, even though the underlying async operation doesn’t even exist in the deserialized instance? (Or, another way: Should the AsyncHooks state be serialized/deserialized at all?)

}
info_ = nullptr;
}
Expand Down Expand Up @@ -1155,9 +1164,11 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> 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());
Expand Down Expand Up @@ -1185,6 +1196,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());
Expand Down
9 changes: 7 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> execution_async_resource_);
v8::Local<v8::Object> execution_async_resource);
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
inline bool pop_async_context(double async_id);
inline void clear_async_id_stack(); // Used in fatal exceptions.

Expand Down Expand Up @@ -771,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
Expand All @@ -782,7 +787,7 @@ class AsyncHooks : public MemoryRetainer {
void grow_async_ids_stack();

v8::Global<v8::Array> js_execution_async_resources_;
std::vector<v8::Global<v8::Object>> native_execution_async_resources_;
std::vector<v8::Local<v8::Object>> native_execution_async_resources_;

// Non-empty during deserialization
const SerializeInfo* info_ = nullptr;
Expand Down