src: add API to get MultiIsolatePlatform used in node main thread by helloshuangzi · Pull Request #20447 · nodejs/node

helloshuangzi · 2018-05-01T02:58:37Z

This is used to support multi-thread JavaScript by node addon as below.
A node addon can be created to initialize v8 Isolates in separate threads, which is used to run JavaScript in multi-thread. It requires the MultiIsolatePlatform (used in the node main thread) to register those isolates respectively.

As an example you may concern, when we try to re-implement napajs Worker::WorkerThreadFunc by node public APIs, it seems node::GetNodeMultiIsolatePlatform is a missing piece from node API.

#20239 is also an issue about this topic. @fs-eire

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

…ain thread

addaleax · 2018-05-01T09:50:16Z

src/node.h

 NODE_EXTERN void FreeEnvironment(Environment* env);

+// This returns the MultiIsolatePlatform used by node main.
+// If NODE_USE_V8_PLATFORM is not defined, it will return nullptr.


Maybe specific that the macro needs to have been defined when Node.js was built, not when the addon is being built?

Also, what's "node main"? This comment could be expanded a little.

bnoordhuis

See also #17859 and #17946.

bnoordhuis · 2018-05-01T10:46:40Z

src/node.cc

 }


+MultiIsolatePlatform* GetNodeMultiIsolatePlatform() {


Redundant name, it's already in the node:: namespace. Just GetMultiIsolatePlatform() or GetMainThreadMultiIsolatePlatform()?

change it to GetMainThreadMultiIsolatePlatform as a more accurate name.

bnoordhuis · 2018-05-01T10:47:34Z

src/node.h

 NODE_EXTERN void FreeEnvironment(Environment* env);

+// This returns the MultiIsolatePlatform used by node main.
+// If NODE_USE_V8_PLATFORM is not defined, it will return nullptr.


Also, what's "node main"? This comment could be expanded a little.

bnoordhuis

LGTM, I think. The commit log should conform to the style guide, though.

bnoordhuis · 2018-05-02T12:03:58Z

src/node.h


+// This returns the MultiIsolatePlatform used in the main thread of Node.js.
+// If NODE_USE_V8_PLATFORM haven't been defined when Node.js was built,
+// it would return nullptr.


Suggestion: it returns instead of it would return.

helloshuangzi · 2018-05-02T16:58:49Z

Thanks for your quick review!
BTW, I just made the commit log confirm to the style guideline to my best knowledge. Please help modify it if anything required when you help squash and merge it.

BridgeAR · 2018-05-05T13:33:47Z

CI https://ci.nodejs.org/job/node-test-pull-request/14669/

addaleax · 2018-05-06T22:22:52Z

Landed in b00c34c 🎉

And congratulations on the 22,222nd commit to Node master :)

Add an API to get MultiIsolatePlatform used in node main thread. PR-URL: #20447 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable Changes: * console: - make console.table() use colored inspect (TSUYUSATO Kitsune) #20510 * fs: - move fs/promises to fs.promises (cjihrig) #20504 * http: - added aborted property to request (Robert Nagy) #20094 * n-api: - initialize a module via a special symbol (Gabriel Schulhof) #20161 * src: - add public API to expose the main V8 Platform (Allen Yonghuang Wang) #20447 PR-URL: Coming Soon

Add an API to get MultiIsolatePlatform used in node main thread. PR-URL: #20447 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable Changes: * console: - make console.table() use colored inspect (TSUYUSATO Kitsune) #20510 * fs: - move fs/promises to fs.promises (cjihrig) #20504 * http: - added aborted property to request (Robert Nagy) #20094 * n-api: - initialize a module via a special symbol (Gabriel Schulhof) #20161 * src: - add public API to expose the main V8 Platform (Allen Yonghuang Wang) #20447 PR-URL: #20606

Add an API to get MultiIsolatePlatform used in node main thread. PR-URL: #20447 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable Changes: * console: - make console.table() use colored inspect (TSUYUSATO Kitsune) #20510 * fs: - move fs/promises to fs.promises (cjihrig) #20504 * http: - added aborted property to request (Robert Nagy) #20094 * n-api: - initialize a module via a special symbol (Gabriel Schulhof) #20161 * src: - add public API to expose the main V8 Platform (Allen Yonghuang Wang) #20447 PR-URL: #20606

src: add public API to expose the MultiIsolatePlatform used in node m…

95b93f9

…ain thread

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 1, 2018

addaleax approved these changes May 1, 2018

View reviewed changes

bnoordhuis reviewed May 1, 2018

View reviewed changes

jasnell approved these changes May 1, 2018

View reviewed changes

helloshuangzi force-pushed the GetNodeMultiIsolatePlatform branch from e24fb6e to a115619 Compare May 1, 2018 17:17

src: give it a more accurate name and polish its comments.

c92af0b

helloshuangzi force-pushed the GetNodeMultiIsolatePlatform branch from a115619 to c92af0b Compare May 1, 2018 17:21

bnoordhuis approved these changes May 2, 2018

View reviewed changes

bnoordhuis reviewed May 2, 2018

View reviewed changes

src: polish comments

bb5c76c

danbev approved these changes May 3, 2018

View reviewed changes

cjihrig approved these changes May 3, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. embedding Issues and PRs related to embedding Node.js in another project. labels May 5, 2018

addaleax closed this May 6, 2018

helloshuangzi deleted the GetNodeMultiIsolatePlatform branch May 7, 2018 19:16

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: add API to get MultiIsolatePlatform used in node main thread#20447

src: add API to get MultiIsolatePlatform used in node main thread#20447
helloshuangzi wants to merge 3 commits intonodejs:masterfrom
helloshuangzi:GetNodeMultiIsolatePlatform

helloshuangzi commented May 1, 2018

Uh oh!

addaleax May 1, 2018

Uh oh!

bnoordhuis May 1, 2018

Uh oh!

bnoordhuis left a comment

Uh oh!

bnoordhuis May 1, 2018

Uh oh!

helloshuangzi May 1, 2018

Uh oh!

bnoordhuis May 1, 2018

Uh oh!

bnoordhuis left a comment

Uh oh!

bnoordhuis May 2, 2018

Uh oh!

helloshuangzi commented May 2, 2018

Uh oh!

BridgeAR commented May 5, 2018

Uh oh!

addaleax commented May 6, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

		}


		MultiIsolatePlatform* GetNodeMultiIsolatePlatform() {

Uh oh!

Conversation

helloshuangzi commented May 1, 2018

Checklist

Uh oh!

addaleax May 1, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 1, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 1, 2018

Choose a reason for hiding this comment

Uh oh!

helloshuangzi May 1, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 1, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 2, 2018

Choose a reason for hiding this comment

Uh oh!

helloshuangzi commented May 2, 2018

Uh oh!

BridgeAR commented May 5, 2018

Uh oh!

addaleax commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented May 6, 2018 •

edited

Loading