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

multi-thread support proposal #20239

Closed
fs-eire opened this issue Apr 23, 2018 · 7 comments
Closed

multi-thread support proposal #20239

fs-eire opened this issue Apr 23, 2018 · 7 comments
Labels
addons Issues and PRs related to native addons. worker Issues and PRs related to Worker support.

Comments

@fs-eire
Copy link
Contributor

fs-eire commented Apr 23, 2018

Multi-thread support in Node.js is not a new requirement. There was a previous proposal #13143 trying to bring multi-threading capability to node, and there are several projects outside node doing this effort as listed here as well.

The worker proposal, however, seems not go smoothly. I consider one of the reasons is because the change turns out to be too huge. I would like to share another approach of bringing multi-thread capability into node.

The basic idea is to break the work into several stages -
1. Enable multi-thread capability for addon.
2. Enable node API in multi-thread incrementally.
3. Add multi-thread node API when we think it's ready.

The first step is to enable multi-thread capability for addon, that is to say, to make it possible in addon to create another javascript thread which applies node's context. To achieve this, node's initialization and module loader need to be updated multi-thread readiness. Native API/napi should also be updated to expose necessary information.

The next step is to tune node API so that they can be helpful in multi-thread. There can be a lot of discussions and decision making, like -

  • What is the I/O API's expected behavior in multi-thread
  • Replace Buffer's underlying ArrayBuffer by SharedArrayBuffer
  • Revisit the crypto API in multi-thread
  • ...

We are not going to do all those on once. They can be separated to small and independent problems.

The final step is like a natural result, when dependencies and blocking issues are resolved.

The advantage of this approach is, we will not break any feature, or add incomplete feature in every step, while we can keep every changes small and clean. Enabling multi-thread for addon also enables addon developers explore and discover use cases and problems that may happen. JS users should not be affected since no new APIs are exposed before stage (3).

/cc @addaleax @Trott @bnoordhuis @jasnell

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Apr 23, 2018
@addaleax
Copy link
Member

Enable multi-thread capability for addon.

Yes, that would be a neat thing for Node to support anyway – we should definitely allow embedders and addons to use Node.js in multiple threads, whether that’s for Workers or not 👍

The worker proposal, however, seems not go smoothly. I consider one of the reasons is because the change turns out to be too huge.

Yes, and I’m not happy about that either – But the current main obstacle is pretty well-defined: Edge cases in old Windows code in libuv. So, if we were to say that not supporting Windows to begin with is an option, we could actually move forward now.

I mean, I’m glad about concrete suggestions or help. But it probably is worth pointing out that working Worker code for Node does exist at this point.

@addaleax
Copy link
Member

Also, I’ve been mentioning it in the TSC meetings, and I know @mhdawson was trying to reach out to MS people – if you know somebody who is somewhat familiar with debugging Node.js and in particular libuv on Windows, and is familiar with async I/O using the Windows C API, that might be all we need to bring #19377 over the line.

(Even without the PR, those seem to be real bugs on Windows to begin with – they just don’t manifest themselves yet.)

@mhdawson
Copy link
Member

@aruneshchandra ,@digitalinfinity any help on the windows side for #19377 would be greatly appreciated.

@bzoz
Copy link
Contributor

bzoz commented Apr 25, 2018

I've picked up #19377, I'm investigating it right now.

@fs-eire
Copy link
Contributor Author

fs-eire commented Apr 26, 2018

@addaleax I would like to contribute to the part "Enable multi-thread capability for addon". There are 2 changes need to be done:

  • Update DLib to support multiple Environment
  • Add new n-api to support manipulate Environment

For the first one I believe I can find some implementation from Ayo.js.
For the second one, it may be a good idea to have a discussion about the napi design.

hope this can help

@addaleax
Copy link
Member

@fs-eire Yes, I agree with 100 % of what you just said. :) The patch is probably outdated compared to nodejs/node@master, but you could find relevant pieces in ayojs/ayo#118 for the dynamic library stuff.

Also, PRs you might want to be aware of:

@mhdawson
Copy link
Member

mhdawson commented Apr 27, 2018

@fs-eire I'd suggest you attend the next N-API meeting (Thursday at 1:30 EST, Description:https://hangouts.google.com/hangouts/_/event/c0eevtrlajniu7h8cjrdk0f56c8) to kick off the discussion with the N-API team. (I'm hoping that time works for you)

@addaleax addaleax added the addons Issues and PRs related to native addons. label Feb 17, 2019
addaleax added a commit that referenced this issue Feb 25, 2019
Allow loading add-ons from multiple Node.js instances if they are
declared context-aware; in particular, this applies to N-API addons.

Also, plug a memory leak that occurred when registering N-API addons.

Refs: #23319

PR-URL: #26175
Fixes: #21481
Fixes: #21783
Fixes: #25662
Fixes: #20239
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
Allow loading add-ons from multiple Node.js instances if they are
declared context-aware; in particular, this applies to N-API addons.

Also, plug a memory leak that occurred when registering N-API addons.

Refs: #23319

PR-URL: #26175
Fixes: #21481
Fixes: #21783
Fixes: #25662
Fixes: #20239
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants