Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v8: patch for thenables in PromiseHook API #33560

Closed
wants to merge 1 commit into from
Closed

v8: patch for thenables in PromiseHook API #33560

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions deps/v8/src/builtins/builtins-microtask-queue-gen.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,20 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
const TNode<Object> thenable = LoadObjectField(
microtask, PromiseResolveThenableJobTask::kThenableOffset);

// Run the promise before/debug hook if enabled.
RunPromiseHook(Runtime::kPromiseHookBefore, microtask_context,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking through the history of promise hooks (v8:4643 and such) trying to understand why this would've been left out and I haven't found anything, but it does seem suspicious...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it was just missed. I think "thenables" just weren't considered. I don't see anything in v8:4643 suggesting otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. They have it for the other branches in there, but not thenables. Makes me think there could be a specific reason for that. As far as I can tell though it's just been overlooked. In testing it locally though, it fixes the problem perfectly for our needs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was a discussion at diagnostics summit in munich if v8 should call promise hooks for non user visible promises or not. The agreement was it is not needed as it improves performance and assumption was that no hooks are needed for "invisible" promises. Promisehooks are quite expensive for v8 - in special since they did more optimizations of promises/await that time.
To me this looks like some corner case regarding visibility of the promise created here. The Promise itself is not visible in the sense that user can access it as it is only used within the await statemachine. On the other hand the side effects are visible.

CAST(promise_to_resolve));

{
ScopedExceptionHandler handler(this, &if_exception, &var_exception);
CallBuiltin(Builtins::kPromiseResolveThenableJob, native_context,
promise_to_resolve, thenable, then);
}

// Run the promise after/debug hook if enabled.
RunPromiseHook(Runtime::kPromiseHookAfter, microtask_context,
CAST(promise_to_resolve));

RewindEnteredContext(saved_entered_context_count);
SetCurrentContext(current_context);
Goto(&done);
Expand Down