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

consider implementing v8::Platform on top of libuv instead of using the default platform #12980

Closed
jeisinger opened this issue May 11, 2017 · 14 comments
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding. v8 engine Issues and PRs related to the V8 dependency. v8 platform Issues and PRs related to Node's v8::Platform implementation.

Comments

@jeisinger
Copy link
Contributor

currently node.js just blocks if there are pending tasks on libuv's message loop before exiting, however, with e.g. WebAssembly's async compile API, it might be the case that there's ongoing work on one of v8::Platform's background threads, and eventually in the future, a task will be posted to the foreground thread to resolve the promise for the compilation.

If node.js would implement its own version of v8::Platform instead of relying on the default platform shipped with v8, it would have visibility into the number of on-going tasks, and could wait for the promise to resolve before exiting.

From a cursory look, posting tasks to the main loop of libuv would be easy. I didn't see a way to post delayed tasks? Also, node.js would have to implement its own worker pool.

There's no need to implement idle tasks, the platform can just indicate that it doesn't support idle tasks

@fhinkel fhinkel added the v8 engine Issues and PRs related to the V8 dependency. label May 11, 2017
@fhinkel
Copy link
Member

fhinkel commented May 11, 2017

/cc @nodejs/v8

@joshgav
Copy link
Contributor

joshgav commented May 11, 2017

and @nodejs/chakracore too :)

@matthewloring
Copy link

Just as an fyi, I played around with this a few months back and have some initial prototype work here: https://github.com/matthewloring/node/commit/c76caae8de9c689db1018506b2fa8d3ac4d38e3e. Happy to pick it up again or collaborate on this if others are interested.

@jeisinger
Copy link
Contributor Author

Interesting!

It's not necessary to implement idle task support, the platform can just indicate that it doesn't support idle tasks. For the rest, why not post stuff directly to libuv's event loop?

The Call* methods could be implemented on top of a uv_timer and uv_work_queue respectively. The tracing stuff, I'd just take over from v8's libplatform

@joshgav
Copy link
Contributor

joshgav commented May 17, 2017

@jeisinger

If node.js would implement its own version of v8::Platform instead of relying on the default platform shipped with v8 ... posting tasks to the main loop of libuv would be easy.... Also, node.js would have to implement its own worker pool.

To further clarify, I believe you are suggesting that the static struct {...} v8_platform defined in node.cc here be abstracted out as e.g. NodePlatform, with e.g. NodeV8Platform and NodeChakraPlatform being implementations of it also provided by Node. @matthewloring's referenced commit does part of that.

Then, Node would substitute the uv background tasks for V8 background tasks in our (i.e. Node's) platform implementations.

Chakra's implementation, as an example, would perhaps substitute JsCreateRuntime for V8::InitializePlatform. NodePlatform would probably evolve into a standard runtime hosting API for JS.

Is that what you're suggesting? Thanks!

@joshgav
Copy link
Contributor

joshgav commented May 18, 2017

On further review, I'm conflating v8::Platform and node::v8_platform in a distracting way. In truth the latter is fairly complete as is in factoring platform setup into it's own space, and finishing that up should probably be an independent issue which I'll try to work on eventually.

Nevertheless...

If we want a custom derivation of v8::Platform in Node as @jeisinger suggested and as in Matt's commit, I think we should copy v8-platform.h into Node as node-platform.h and derive from a shared abstraction.

cc @mhdawson @nodejs/api

@jeisinger
Copy link
Contributor Author

Sorry, I'm indeed quite confused but what you mean @joshgav. I'd propose to first replace the bundling v8::Platform implementation with a custom node version, and then continue with your shared abstraction plan. Does that make sense?

@joshgav
Copy link
Contributor

joshgav commented May 18, 2017

@jeisinger

first replace the bundling v8::Platform implementation with a custom node version, and then continue with your shared abstraction plan

It's the proposed order that I'm not sure about. We (Node) have been trying to move away from tight coupling to V8 APIs, and writing our own implementation of v8::Platform seems to move us the other way. If we need our own implementation, wouldn't it be better to define a node::Platform (probably a copy of v8::Platform) first and derive from that?

@jeisinger
Copy link
Contributor Author

Hum, it sounds like we're still not talking about the same thing. Let me try to explain a bit more:

v8::Platform is a pure virtual interface, and in order to start v8, you need to create a class that derives from that interface and pass an instance of that class to v8.

if you copy v8::Platform and call it node::Platform (or anything else - copying is the point here), you can no longer use v8.

To help embedders of v8 to get started, we bundle a library that provides an example class call "DefaultPlatform". For more advanced usages of v8, DefaultPlatform is not suitable.

In this issue, I want to point out that the usage of v8 by node falls into this advanced category, and from v8 6.0 on, the shortcomings of DefaultPlatform will become user visible.

What Matt started was to copy the DefaultPlatform into node, which is a reasonable first step. Maybe you're suggesting that this copy should inherit from some node::Platform interface? As explained above, it has to inherit from v8::Platform.

I'm also not sure how much value there would be in such a node::Platform, as really all that v8::Platform does is giving access to the embedder's message loop which in node's case is handled by libuv.

@joshgav
Copy link
Contributor

joshgav commented May 19, 2017

you're suggesting that this copy should inherit from some node::Platform interface? ... it has to inherit from v8::Platform.

Yes, I'm suggesting a node::Platform interface because I think we'll all benefit from a shared definition. Let me explain and describe the small adjustments which I think would be needed to avoid having to inherit from v8::Platform. I added a bit to Matt's code here: joshgav@88da02c to demonstrate and make this easier to reason about.

Please bear with me, all this is relevant to @jeisinger's original request cause I think Node shouldn't implement a custom platform till it defines and manages such a platform itself. The further we go with v8::Platform, the harder it will be to decouple from it for engines like Chakra and embedders like Electron.

Also, a platform defined and managed by Node will make it easier to recognize proposed changes in one engine or embedder which others need to take account of - for example the Trace and Inspector initializers which were introduced over the past year into both v8::Platform and node::v8_platform.

A good example of this is in my demo code now: it doesn't compile primarily because of Line 248, which calls V8::InitializePlatform(v8::Platform). If Node defined and provided its own platform, it would be clear that another signature is needed - perhaps the InitializePlatform method should accept a void* and cast to v8::Platform internally.

Building on this and using Chakra as an example, if it provided a similar platform initialization function, say by extending JsCreateRuntime, it wouldn't be able to accept a v8::Platform, but it could accept a void* pointer.

In summary, by defining and deriving from a node::Platform instead of v8::Platform we would make Node more flexible and would more clearly see where we're choosing to lower that flexibility and couple tightly to specific implementations.

Does that make sense? What do you think? Thanks!

@jeisinger
Copy link
Contributor Author

defining a node::Platform and then casting that to void* and having v8 cast that to v8::Platform is not allowed by the C++ standard.

@joshgav
Copy link
Contributor

joshgav commented May 19, 2017

@jeisinger

defining a node::Platform and then casting that to void* and having v8 cast that to v8::Platform is not allowed by the C++ standard.

Also, I don't think it would work unless v8::Platform itself derived from node::Platform, which I'm pretty sure you (and Chakra) wouldn't want to do :). Indeed, I don't know how to solve that problem and am hoping you and @nodejs/chakracore have suggestions :). How do we define a shared interface without depending on a header from Node?

What do you think of the argument for a shared interface itself?

@jeisinger
Copy link
Contributor Author

I'd recommend introducing a wrapper library around the underlying VM that has an API you control. E.g. in chrome, we have the "gin" library that implements interfaces such as v8:: Platform, and the rest of chrome can instantiate v8 via a simple API provided by gin.

@addaleax addaleax added the libuv Issues and PRs related to the libuv dependency or the uv binding. label May 23, 2017
@addaleax
Copy link
Member

Just so y’all know, there has been some previous discussion about this topic in #11855 (if I’m not mixing things up).

addaleax pushed a commit that referenced this issue Aug 17, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding. v8 engine Issues and PRs related to the V8 dependency. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants