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`.

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>

Author: addaleax

src/env-inl.h

@@ -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,
@@ -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;
   }
 }
 

src/env.cc

@@ -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;
 }
@@ -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());

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<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.
 
@@ -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;