Skip to content

Commit

Permalink
src: store native async execution resources as v8::Local
Browse files Browse the repository at this point in the history
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: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
addaleax authored and targos committed Jan 14, 2022
1 parent 6187e81 commit 250e197
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 17 deletions.
9 changes: 4 additions & 5 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];
}

inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init,
Expand Down Expand Up @@ -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;
}
}

Expand Down
31 changes: 21 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();
}
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
7 changes: 5 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);
inline bool pop_async_context(double async_id);
inline void clear_async_id_stack(); // Used in fatal exceptions.

Expand Down Expand Up @@ -782,7 +785,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

0 comments on commit 250e197

Please sign in to comment.