[debug] Decouple async event delegate instrumentation from PromiseHooks.

As described in https://crbug.com/1287476, the fact that the
AsyncEventDelegate is currently implemented on top of the PromiseHooks
causes performance problems and makes it difficult to reason about the
exact (observed) semantics; this is because for this we intercept every
JSPromise creation (via PromiseHook::kInit) and walk the synchronous
stack at that point to see if we find one of Promise#then(),
Promise#catch() or Promise#finally() on the stack. And if we do so, we
report that to the AsyncEventDelegate (which is implemented in the
inspector and will then do the async stack/stepping logic on top).

This CL introduces dedicated instrumentation for Promise#then(), which
is also called from Promise#catch() and Promise#finally(), and uses that
instrumentation for the purpose of the AsyncEventDelegate. It also
adjusts the stack walk to not always walk the full stack (which might
lead to wrong results when calls to Promise#then(), which itself can
call back into user JavaScript, are found deeper in the stack), but
instead only check the top-most builtin frames and whatever user
JavaScript frame is underneath it.

On the standalone.js (from https://crbug.com/1287476#c1), when run with
the DevTools default of maxDepth=200, we go from around 4.00ms to around
0.36ms. For everything that does not call Promise#then() - either
explicitly or implicitly - or `await`s, there's now no observable
performance impact of turning on the AsyncEventDelegate.

Bug: chromium:1280519
Fixed: chromium:1287476
Change-Id: I4911bed146381fc46cfeefb763d6dfc32e8f6071
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3386379
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78640}

src/builtins/promise-misc.tq

@@ -23,6 +23,9 @@ extern macro PromiseBuiltinsAssembler::AllocateJSPromise(Context): HeapObject;
 extern macro
 PromiseBuiltinsAssembler::IsContextPromiseHookEnabled(uint32): bool;
 
+extern macro
+PromiseBuiltinsAssembler::IsIsolatePromiseHookEnabled(uint32): bool;
+
 extern macro
 PromiseBuiltinsAssembler::PromiseHookFlags(): uint32;
 
@@ -222,7 +225,7 @@ transitioning macro RunAnyPromiseHookInit(implicit context: Context)(
       RunContextPromiseHookInit(promise, parent);
     }
   }
-  if (IsIsolatePromiseHookEnabledOrHasAsyncEventDelegate(promiseHookFlags)) {
+  if (IsIsolatePromiseHookEnabled(promiseHookFlags)) {
     runtime::PromiseHookInit(promise, parent);
   }
 }

src/builtins/promise-then.tq

@@ -4,8 +4,16 @@
 
 #include 'src/builtins/builtins-promise-gen.h'
 
+namespace runtime {
+extern transitioning runtime
+DebugPromiseThen(implicit context: Context)(JSAny): JSAny;
+}
+
 namespace promise {
 
+extern macro
+CodeStubAssembler::HasAsyncEventDelegate(): bool;
+
 macro
 IsPromiseSpeciesLookupChainIntact(
     nativeContext: NativeContext, promiseMap: Map): bool {
@@ -68,6 +76,14 @@ PromisePrototypeThen(js-implicit context: NativeContext, receiver: JSAny)(
   //    resultCapability).
   PerformPromiseThenImpl(
       promise, onFulfilled, onRejected, resultPromiseOrCapability);
+
+  // Async instrumentation for Promise#then(), Promise#catch() and
+  // Promise#finally(), where the latter two both call eventually
+  // call into Promise#then().
+  if (HasAsyncEventDelegate()) {
+    return runtime::DebugPromiseThen(resultPromise);
+  }
+
   return resultPromise;
 }
 }

src/codegen/code-stub-assembler.cc

@@ -14414,6 +14414,11 @@ TNode<BoolT> CodeStubAssembler::IsAnyPromiseHookEnabled(TNode<Uint32T> flags) {
   return IsSetWord32(flags, mask);
 }
 
+TNode<BoolT> CodeStubAssembler::IsIsolatePromiseHookEnabled(
+    TNode<Uint32T> flags) {
+  return IsSetWord32<Isolate::PromiseHookFields::HasIsolatePromiseHook>(flags);
+}
+
 #ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
 TNode<BoolT> CodeStubAssembler::IsContextPromiseHookEnabled(
     TNode<Uint32T> flags) {

src/codegen/code-stub-assembler.h

@@ -3670,6 +3670,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
 #ifdef V8_ENABLE_JAVASCRIPT_PROMISE_HOOKS
   TNode<BoolT> IsContextPromiseHookEnabled(TNode<Uint32T> flags);
 #endif
+  TNode<BoolT> IsIsolatePromiseHookEnabled(TNode<Uint32T> flags);
   TNode<BoolT> IsAnyPromiseHookEnabled(TNode<Uint32T> flags);
   TNode<BoolT> IsAnyPromiseHookEnabled() {
     return IsAnyPromiseHookEnabled(PromiseHookFlags());

src/execution/isolate.cc

@@ -4834,79 +4834,16 @@ void Isolate::RunAllPromiseHooks(PromiseHookType type,
 
 void Isolate::RunPromiseHook(PromiseHookType type, Handle<JSPromise> promise,
                              Handle<Object> parent) {
-  RunPromiseHookForAsyncEventDelegate(type, promise);
   if (!HasIsolatePromiseHooks()) return;
   DCHECK(promise_hook_ != nullptr);
   promise_hook_(type, v8::Utils::PromiseToLocal(promise),
                 v8::Utils::ToLocal(parent));
 }
 
-void Isolate::RunPromiseHookForAsyncEventDelegate(PromiseHookType type,
-                                                  Handle<JSPromise> promise) {
-  if (!HasAsyncEventDelegate()) return;
-  DCHECK(async_event_delegate_ != nullptr);
-  switch (type) {
-    case PromiseHookType::kResolve:
-      return;
-    case PromiseHookType::kBefore:
-      if (!promise->async_task_id()) return;
-      async_event_delegate_->AsyncEventOccurred(
-          debug::kDebugWillHandle, promise->async_task_id(), false);
-      break;
-    case PromiseHookType::kAfter:
-      if (!promise->async_task_id()) return;
-      async_event_delegate_->AsyncEventOccurred(
-          debug::kDebugDidHandle, promise->async_task_id(), false);
-      break;
-    case PromiseHookType::kInit:
-      debug::DebugAsyncActionType action_type = debug::kDebugPromiseThen;
-      bool last_frame_was_promise_builtin = false;
-      JavaScriptFrameIterator it(this);
-      while (!it.done()) {
-        std::vector<Handle<SharedFunctionInfo>> infos;
-        it.frame()->GetFunctions(&infos);
-        for (size_t i = 1; i <= infos.size(); ++i) {
-          Handle<SharedFunctionInfo> info = infos[infos.size() - i];
-          if (info->IsUserJavaScript()) {
-            // We should not report PromiseThen and PromiseCatch which is called
-            // indirectly, e.g. Promise.all calls Promise.then internally.
-            if (last_frame_was_promise_builtin) {
-              DCHECK_EQ(0, promise->async_task_id());
-              promise->set_async_task_id(++async_task_count_);
-              async_event_delegate_->AsyncEventOccurred(
-                  action_type, promise->async_task_id(),
-                  debug()->IsBlackboxed(info));
-            }
-            return;
-          }
-          last_frame_was_promise_builtin = false;
-          if (info->HasBuiltinId()) {
-            if (info->builtin_id() == Builtin::kPromisePrototypeThen) {
-              action_type = debug::kDebugPromiseThen;
-              last_frame_was_promise_builtin = true;
-            } else if (info->builtin_id() == Builtin::kPromisePrototypeCatch) {
-              action_type = debug::kDebugPromiseCatch;
-              last_frame_was_promise_builtin = true;
-            } else if (info->builtin_id() ==
-                       Builtin::kPromisePrototypeFinally) {
-              action_type = debug::kDebugPromiseFinally;
-              last_frame_was_promise_builtin = true;
-            }
-          }
-        }
-        it.Advance();
-      }
-  }
-}
-
 void Isolate::OnAsyncFunctionSuspended(Handle<JSPromise> promise,
                                        Handle<JSPromise> parent) {
   DCHECK_EQ(0, promise->async_task_id());
-  if (HasIsolatePromiseHooks()) {
-    DCHECK_NE(nullptr, promise_hook_);
-    promise_hook_(PromiseHookType::kInit, v8::Utils::PromiseToLocal(promise),
-                  v8::Utils::PromiseToLocal(parent));
-  }
+  RunPromiseHook(PromiseHookType::kInit, promise, parent);
   if (HasAsyncEventDelegate()) {
     DCHECK_NE(nullptr, async_event_delegate_);
     promise->set_async_task_id(++async_task_count_);
@@ -4920,6 +4857,68 @@ void Isolate::OnAsyncFunctionSuspended(Handle<JSPromise> promise,
   }
 }
 
+void Isolate::OnPromiseThen(Handle<JSPromise> promise) {
+  if (!HasAsyncEventDelegate()) return;
+  Maybe<debug::DebugAsyncActionType> action_type =
+      Nothing<debug::DebugAsyncActionType>();
+  for (JavaScriptFrameIterator it(this); !it.done(); it.Advance()) {
+    std::vector<Handle<SharedFunctionInfo>> infos;
+    it.frame()->GetFunctions(&infos);
+    for (auto it = infos.rbegin(); it != infos.rend(); ++it) {
+      Handle<SharedFunctionInfo> info = *it;
+      if (info->HasBuiltinId()) {
+        // We should not report PromiseThen and PromiseCatch which is called
+        // indirectly, e.g. Promise.all calls Promise.then internally.
+        switch (info->builtin_id()) {
+          case Builtin::kPromisePrototypeCatch:
+            action_type = Just(debug::kDebugPromiseCatch);
+            continue;
+          case Builtin::kPromisePrototypeFinally:
+            action_type = Just(debug::kDebugPromiseFinally);
+            continue;
+          case Builtin::kPromisePrototypeThen:
+            action_type = Just(debug::kDebugPromiseThen);
+            continue;
+          default:
+            return;
+        }
+      }
+      if (info->IsUserJavaScript() && action_type.IsJust()) {
+        DCHECK_EQ(0, promise->async_task_id());
+        promise->set_async_task_id(++async_task_count_);
+        async_event_delegate_->AsyncEventOccurred(action_type.FromJust(),
+                                                  promise->async_task_id(),
+                                                  debug()->IsBlackboxed(info));
+      }
+      return;
+    }
+  }
+}
+
+void Isolate::OnPromiseBefore(Handle<JSPromise> promise) {
+  RunPromiseHook(PromiseHookType::kBefore, promise,
+                 factory()->undefined_value());
+  if (HasAsyncEventDelegate()) {
+    if (promise->async_task_id()) {
+      async_event_delegate_->AsyncEventOccurred(
+          debug::kDebugWillHandle, promise->async_task_id(), false);
+    }
+  }
+  if (debug()->is_active()) PushPromise(promise);
+}
+
+void Isolate::OnPromiseAfter(Handle<JSPromise> promise) {
+  RunPromiseHook(PromiseHookType::kAfter, promise,
+                 factory()->undefined_value());
+  if (HasAsyncEventDelegate()) {
+    if (promise->async_task_id()) {
+      async_event_delegate_->AsyncEventOccurred(
+          debug::kDebugDidHandle, promise->async_task_id(), false);
+    }
+  }
+  if (debug()->is_active()) PopPromise();
+}
+
 void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
   promise_reject_callback_ = callback;
 }

src/execution/isolate.h

@@ -949,9 +949,12 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
     PromiseHookStateUpdated();
   }
 
-  // Async function instrumentation support.
+  // Async function and promise instrumentation support.
   void OnAsyncFunctionSuspended(Handle<JSPromise> promise,
                                 Handle<JSPromise> parent);
+  void OnPromiseThen(Handle<JSPromise> promise);
+  void OnPromiseBefore(Handle<JSPromise> promise);
+  void OnPromiseAfter(Handle<JSPromise> promise);
 
   // Re-throw an exception.  This involves no error reporting since error
   // reporting was handled when the exception was thrown originally.
@@ -1998,9 +2001,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   bool PropagatePendingExceptionToExternalTryCatch(
       ExceptionHandlerType top_handler);
 
-  void RunPromiseHookForAsyncEventDelegate(PromiseHookType type,
-                                           Handle<JSPromise> promise);
-
   bool HasIsolatePromiseHooks() const {
     return PromiseHookFields::HasIsolatePromiseHook::decode(
         promise_hook_flags_);

src/runtime/runtime-debug.cc

@@ -858,6 +858,16 @@ RUNTIME_FUNCTION(Runtime_DebugAsyncFunctionSuspended) {
   return *throwaway;
 }
 
+RUNTIME_FUNCTION(Runtime_DebugPromiseThen) {
+  DCHECK_EQ(1, args.length());
+  HandleScope scope(isolate);
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, promise, 0);
+  if (promise->IsJSPromise()) {
+    isolate->OnPromiseThen(Handle<JSPromise>::cast(promise));
+  }
+  return *promise;
+}
+
 RUNTIME_FUNCTION(Runtime_LiveEditPatchScript) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());

src/runtime/runtime-promise.cc

@@ -124,26 +124,20 @@ RUNTIME_FUNCTION(Runtime_PromiseHookInit) {
 RUNTIME_FUNCTION(Runtime_PromiseHookBefore) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, maybe_promise, 0);
-  if (!maybe_promise->IsJSPromise())
-    return ReadOnlyRoots(isolate).undefined_value();
-  Handle<JSPromise> promise = Handle<JSPromise>::cast(maybe_promise);
-  if (isolate->debug()->is_active()) isolate->PushPromise(promise);
-  isolate->RunPromiseHook(PromiseHookType::kBefore, promise,
-                          isolate->factory()->undefined_value());
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, promise, 0);
+  if (promise->IsJSPromise()) {
+    isolate->OnPromiseBefore(Handle<JSPromise>::cast(promise));
+  }
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
 RUNTIME_FUNCTION(Runtime_PromiseHookAfter) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, maybe_promise, 0);
-  if (!maybe_promise->IsJSPromise())
-    return ReadOnlyRoots(isolate).undefined_value();
-  Handle<JSPromise> promise = Handle<JSPromise>::cast(maybe_promise);
-  if (isolate->debug()->is_active()) isolate->PopPromise();
-  isolate->RunPromiseHook(PromiseHookType::kAfter, promise,
-                          isolate->factory()->undefined_value());
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, promise, 0);
+  if (promise->IsJSPromise()) {
+    isolate->OnPromiseAfter(Handle<JSPromise>::cast(promise));
+  }
   return ReadOnlyRoots(isolate).undefined_value();
 }
 

src/runtime/runtime.h

@@ -130,6 +130,7 @@ namespace internal {
   F(DebugOnFunctionCall, 2, 1)                  \
   F(DebugPopPromise, 0, 1)                      \
   F(DebugPrepareStepInSuspendedGenerator, 0, 1) \
+  F(DebugPromiseThen, 1, 1)                     \
   F(DebugPushPromise, 1, 1)                     \
   F(DebugToggleBlockCoverage, 1, 1)             \
   F(DebugTogglePreciseCoverage, 1, 1)           \