-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: promises
Are you sure you want to change the base?
Conversation
* | ||
* | ||
*/ | ||
class V8_EXPORT MicrotaskQueue { |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Will fix.
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. |
@ofrobots Thanks for the feedback! Do you think it would be better handled as a series of |
(Possibly |
@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. |
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, v8-users@googlegroups.com, 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 |
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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe>
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
HandleScope
s — please point these out! Also missing is a mechanism fordelete
-ing theMicrotaskQueue
on Isolate exit. This approach extends the split betweenEnqueueMicrotask(MicrotaskCallback, void*)
andEnqueueMicrotask(Local<Function>)
from the publicIsolate
to the privateIsolate
.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 useMakeCallback
when entering JS from C++ at top-of-event-loop.