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.

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

Promises patch v8 #2

Open
wants to merge 3 commits into
base: promises
Choose a base branch
from
Open

Promises patch v8 #2

wants to merge 3 commits into from

Conversation

chrisdickinson
Copy link
Owner

This is a WIP, intended to open the discussion on how to proceed with a pluggable MicrotaskQueue. My C++ is rusty, and I've probably made mistakes WRT to not implementing the appropriate HandleScopes — please point these out! Also missing is a mechanism for delete-ing the MicrotaskQueue on Isolate exit. This approach extends the split between EnqueueMicrotask(MicrotaskCallback, void*) and EnqueueMicrotask(Local<Function>) from the public Isolate to the private Isolate.

I've included an example commit that shows how Node could consume this API, by placing microtasks into the nextTick queue. Ultimately we may not unify the queues, but we would make use of this API to allow us to use MakeCallback when entering JS from C++ at top-of-event-loop.

*
*
*/
class V8_EXPORT MicrotaskQueue {

Choose a reason for hiding this comment

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

Probably should explain that using Isolate::CreateParams is necessary, in the comments above. Similar to how every other class that makes use of Isolate::CreateParams.

void Isolate::EnqueueMicrotask(Handle<Object> microtask) {
DCHECK(microtask->IsJSFunction() || microtask->IsCallHandlerInfo());

Choose a reason for hiding this comment

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

Why not leave this DCHECK in place and drop the abort() call?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point! Will fix.

@ofrobots
Copy link

I am worried that a pluggable MicrotaskQueue might be a significant API burden that JS VMs might not want to expose in this manner.

Once VMs implement async/await, keeping this functional would be really hard. For async functions, the thing being added to the micro task queue is not a function but rather some implicitly stored VM state. I would expect it to be unlikely that V8 would want to expose such internal continuation state to an embedder.

@chrisdickinson
Copy link
Owner Author

@ofrobots Thanks for the feedback! Do you think it would be better handled as a series of Enqueue, Pre, and Post(optional err) hooks?

@chrisdickinson
Copy link
Owner Author

(Possibly Enqueue, Pre, and Post would also pass an opaque handle to embedders, so we can associate data with the callback?)

@ofrobots
Copy link

@chrisdickinson Yep, that would be better and avoid exposing continuation state.

BTW, Déjà vu. I have seen this API discussed before. The Angular folks were interested in something like this too before they decided to pursue zones w/ TC39. /cc @littledan, @mhevery.

@littledan
Copy link

In general, if you want to make a change to V8, then filing a bug in the V8 bug tracker and/or uploading a change to Rietveldt will get our attention faster than making a pull request against Node's checked-out v8 dependency. See https://github.com/v8/v8/wiki/Contributing for more information. Feel free to contact me, or even better, [email protected], if you have any trouble getting started.

I share @ofrobots 's concerns about whether this is the right way to do it. In particular, in this case, while Promises are currently done through JSFunctions, they might be reimplemented as a MicrotaskCallback in the future as a performance optimization. If we do have different kinds of things on the microtask queue that are distinguished from each other, I think we should use some other marker rather than this C++ type.

If we are going to expand what the microtask queue is, then we should have a good understanding of other use cases which are related, and if we can solve several problems at once, or whether we really need a new feature. Some web frameworks are also interested in being able to intercept the same event, of wrapping various microtask operations; it'd be great if we only exposed one way to do it, rather than one for Node and one for the web. /cc @ajklein

ceejbot pushed a commit that referenced this pull request Jan 12, 2018
Remove a pointless adapter frame  by fixing up the function's formal
parameter count.  Before:

    frame #0: 0x000033257ea446d5 onParserExecute(...)
    frame #1: 0x000033257ea3b93f <adaptor>
    frame #2: 0x000033257ea41959 <internal>
    frame #3: 0x000033257e9840ff <entry>

After:

    frame #0: 0x00000956287446d5 onParserExecute(...)
    frame #1: 0x0000095628741959 <internal>
    frame #2: 0x00000956286840ff <entry>

PR-URL: nodejs#17693
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants