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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

helloshuangzi
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 1, 2018
addaleax
addaleax approved these changes May 1, 2018
View reviewed changes
src/node.h Outdated
@@ -251,6 +251,10 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN void LoadEnvironment(Environment* env);
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bnoordhuis
bnoordhuis reviewed May 1, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #17859 and #17946.

src/node.cc Outdated
@@ -4425,6 +4425,11 @@ void FreeEnvironment(Environment* env) {
}


MultiIsolatePlatform* GetNodeMultiIsolatePlatform() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change it to GetMainThreadMultiIsolatePlatform as a more accurate name.

src/node.h Outdated
@@ -251,6 +251,10 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN void LoadEnvironment(Environment* env);
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@helloshuangzi helloshuangzi force-pushed the GetNodeMultiIsolatePlatform branch from e24fb6e to a115619 Compare May 1, 2018 17:17
@helloshuangzi helloshuangzi force-pushed the GetNodeMultiIsolatePlatform branch from a115619 to c92af0b Compare May 1, 2018 17:21
bnoordhuis
bnoordhuis approved these changes May 2, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bnoordhuis
bnoordhuis reviewed May 2, 2018
View reviewed changes
src/node.h Outdated
@@ -251,6 +251,11 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN void LoadEnvironment(Environment* env);
NODE_EXTERN void FreeEnvironment(Environment* env);

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: it returns instead of it would return.

@helloshuangzi
Copy link
Contributor Author

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
Copy link
Member

BridgeAR commented May 5, 2018

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

@BridgeAR 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 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
Copy link
Member

addaleax commented May 6, 2018 •
edited
Loading

Landed in b00c34c 🎉

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

@addaleax addaleax closed this May 6, 2018
addaleax pushed a commit that referenced this pull request May 6, 2018
@helloshuangzi @addaleax
src: add public API to expose the main V8 Platform
b00c34c 
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>
@helloshuangzi helloshuangzi deleted the GetNodeMultiIsolatePlatform branch May 7, 2018 19:16
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@helloshuangzi @MylesBorins
src: add public API to expose the main V8 Platform
ea6af3b 
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>
MylesBorins added a commit that referenced this pull request May 8, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
216acb1 
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
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@helloshuangzi @MylesBorins
src: add public API to expose the main V8 Platform
71f8f12 
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>
MylesBorins added a commit that referenced this pull request May 8, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
f8ed2a6 
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
MylesBorins pushed a commit that referenced this pull request May 9, 2018
@helloshuangzi @MylesBorins
src: add public API to expose the main V8 Platform
d357875 
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>
MylesBorins added a commit that referenced this pull request May 9, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
687867d 
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
MylesBorins added a commit that referenced this pull request May 9, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
2c0fc3b 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@helloshuangzi @BridgeAR @addaleax @bnoordhuis @danbev @jasnell @cjihrig @nodejs-github-bot