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 danielleadams committed Feb 1, 2022
1 parent 59625f7 commit 72921f4
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 72921f4

Please sign in to comment.