src: improve MakeCallback() performance

src/api/callback.cc

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

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

src/env.cc

@@ -1096,20 +1096,27 @@ 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) {
     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;
 }
@@ -1157,7 +1164,7 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> 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());
@@ -1185,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());

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