Skip to content
This repository has been archived by the owner on Aug 31, 2018. It is now read-only.

worker: initial implementation #110

Merged
merged 1 commit into from
Oct 22, 2017
Merged

worker: initial implementation #110

merged 1 commit into from
Oct 22, 2017

Conversation

addaleax
Copy link
Contributor

Implement multi-threading support for most of the API.

So far reviewed by @Qard

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included (later)
  • documentation is changed or added (later/other PR)
  • commit message follows commit guidelines
Affected core subsystem(s)

worker

@Qard
Copy link
Member

Qard commented Oct 21, 2017

Not sure how I feel about putting isMainThread in the module wrap. It's probably best to avoid adding more things to top-level global/module scope, especially given that this won't work for es modules.

Perhaps there was some misunderstanding, but my comment on the previous iteration about isMainThread was just about turning process.binding('worker').threadId === 0 in the few places it occurred internally into process.binding('worker').isMainThread by adding a getter for that in the native bindings. That would make the handful of places that threadId === 0 is used a bit clearer.

@TimothyGu
Copy link
Member

I'm also inclined to make the 'worker' binding internal to avoid any misuse by the userland.

@addaleax
Copy link
Contributor Author

especially given that this won't work for es modules.

If we ever migrate core modules to ESM, we’re going to have to do a lot of refactoring anyway…

by adding a getter for that in the native bindings

It doesn’t feel quite right to do that to me, this really is a property of the whole Node environment rather than a single module. Also, it doesn’t quite seem like the bindings are the best place for usability helpers…

I'm also inclined to make the 'worker' binding internal to avoid any misuse by the userland.

Right, yes. This PR was written before they existed but we should definitely do that :) Done!

@Qard
Copy link
Member

Qard commented Oct 22, 2017

Userland code might try to use isMainThread too though, and not port cleanly when switching to es modules in the future. We already have require('workers').isMainThread, which I think should be the recommended mechanism, much like require('cluster').isMaster.

As for the binding for isMainThread, I just figured that would make the internals a bit cleaner, but it's only needed in a couple places. I'm not going to block on that.

@addaleax
Copy link
Contributor Author

@Qard Oh – icymi, since nodejs/node@e6dfd59be02c3 the internals and userland code don’t share their module wrapper anymore, so isMainThread and internalBinding aren’t actually available. :)

@Qard
Copy link
Member

Qard commented Oct 22, 2017

Ah, cool. I was aware of the internalBinding stuff, but I hadn't reviewed it yet so I wasn't aware the module wrappers were split. 👍

Implement multi-threading support for most of the API.

PR-URL: #110
Reviewed-By: Stephen Belanger <[email protected]>
@addaleax addaleax merged commit fe685fb into latest Oct 22, 2017
@TimothyGu TimothyGu deleted the workers-initial branch October 22, 2017 21:04
addaleax added a commit to addaleax/node that referenced this pull request May 22, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117
addaleax added a commit to addaleax/node that referenced this pull request Jun 5, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117
addaleax added a commit to addaleax/node that referenced this pull request Jun 5, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117
addaleax added a commit to nodejs/node that referenced this pull request Jun 6, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117

PR-URL: #20876
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Jun 7, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117

PR-URL: #20876
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Jun 13, 2018
Implement multi-threading support for most of the API.

Thanks to Stephen Belanger for reviewing this change in its
original form, to Olivia Hugger for reviewing the
documentation and some of the tests coming along with it,
and to Alexey Orlenko and Timothy Gu for reviewing other
parts of the tests.

Refs: ayojs/ayo#110
Refs: ayojs/ayo#114
Refs: ayojs/ayo#117

PR-URL: #20876
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants