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

src: multi-isolate support for NodePlatform #16700

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 2, 2017

These commits are taken from Ayo, where they are used for Worker support in the NodePlatform implementation. Since #16658 was opened on this repo, I decided I might as well PR them here as well since I’m pretty certain it would help resolve that issue. No hard feelings if this is rejected.

(NB: These already have been reviewed by Node.js collaborators. This should still not be landed without an explicit +1 from a collaborator in this pretty different context.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

fyi @matthewloring @jasnell

CI: https://ci.nodejs.org/job/node-test-commit/13691/

This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

PR-URL: ayojs/ayo#89
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

PR-URL: ayojs/ayo#120
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency. labels Nov 2, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 2, 2017
@addaleax
Copy link
Member Author

Kicking off CI again because the last one is inaccessible: https://ci.nodejs.org/job/node-test-commit/13919/

@addaleax
Copy link
Member Author

Landed in c7ad729, 2cedff9

@addaleax addaleax closed this Nov 12, 2017
@addaleax addaleax deleted the platform-multiple-isolates branch November 12, 2017 13:15
addaleax added a commit that referenced this pull request Nov 12, 2017
This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

Original-PR-URL: ayojs/ayo#89
Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Nov 12, 2017
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Original-PR-URL: ayojs/ayo#120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Nov 12, 2017

I was thinking of way to benchmark this... did a very native benchmark jenkins job on the child_process category.
But as I assumed, it didn't not show anything with statistical significance...

child-process-exec-stdout.jslen=1024                            0.43 %            0.27
child-process-exec-stdout.jslen=256                            -0.16 %            0.36
child-process-exec-stdout.jslen=32768                          -0.00 %            0.98
child-process-exec-stdout.jslen=4096                           -0.32 %            0.62
child-process-exec-stdout.jslen=64                              0.00 %            0.38
child-process-params.js params=1 methodName="execFile"         -1.03 %            0.16
child-process-params.js params=1 methodName="execFileSync"     -1.36 %            0.20
child-process-params.js params=1 methodName="exec"              0.17 %            0.74
child-process-params.js params=1 methodName="execSync"         -1.27 %          * 0.02
child-process-params.js params=1 methodName="spawn"            -1.77 %            0.18
child-process-params.js params=1 methodName="spawnSync"        -0.36 %            0.68
child-process-params.js params=2 methodName="execFile"         -0.68 %            0.25
child-process-params.js params=2 methodName="execFileSync"     -1.72 %            0.17
child-process-params.js params=2 methodName="exec"             -0.86 %            0.19
child-process-params.js params=2 methodName="execSync"         -0.27 %            0.80
child-process-params.js params=2 methodName="spawn"             1.87 %            0.13
child-process-params.js params=2 methodName="spawnSync"        -0.42 %            0.72
child-process-params.js params=3 methodName="execFile"         -1.52 %            0.05
child-process-params.js params=3 methodName="execFileSync"      1.08 %            0.35
child-process-params.js params=3 methodName="exec"             -0.26 %            0.76
child-process-params.js params=3 methodName="spawn"            -0.46 %            0.72
child-process-params.js params=3 methodName="spawnSync"         0.06 %            0.96
child-process-params.js params=4 methodName="execFile"         -1.14 %            0.12

MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

Original-PR-URL: ayojs/ayo#89
Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Original-PR-URL: ayojs/ayo#120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed this on 9.x and manually resolved conflicts with headers. LMK if this should be pulled out of the release. Also curious about 8.x and 6.x, I'm assuming we shouldn't land. please add appropriate labels

121245f...c2431d5

@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 13, 2017

Also curious about 8.x and 6.x, I'm assuming we shouldn't land.

ping @addaleax (I know it's only been 2 days 😁 )

deepak1556 pushed a commit to electron/electron that referenced this pull request Jan 6, 2018
- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
deepak1556 pushed a commit to electron/electron that referenced this pull request Jan 6, 2018
- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
codebytere added a commit to electron/electron that referenced this pull request Jan 6, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
@MylesBorins
Copy link
Contributor

ping re: backport

zcbenz pushed a commit to electron/electron that referenced this pull request Jan 31, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 21, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 22, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
@MylesBorins
Copy link
Contributor

ping re: 8.x and 6.x

@gibfahn
Copy link
Member

gibfahn commented Mar 20, 2018

ping @addaleax

sethlu pushed a commit to sethlu/electron that referenced this pull request May 3, 2018
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
@MylesBorins
Copy link
Contributor

ping @addaleax

addaleax added a commit to addaleax/node that referenced this pull request May 22, 2018
This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

Original-PR-URL: ayojs/ayo#89
Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#16700
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request May 22, 2018
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Original-PR-URL: ayojs/ayo#120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: nodejs#16700
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

Backport-PR-URL: #20901
Original-PR-URL: ayojs/ayo#89
Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Backport-PR-URL: #20901
Original-PR-URL: ayojs/ayo#120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
This splits the task queue used for asynchronous tasks scheduled
by V8 in per-isolate queues, so that multiple threads can be supported.

Backport-PR-URL: #20901
Original-PR-URL: ayojs/ayo#89
Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.

To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.

It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.

Backport-PR-URL: #20901
Original-PR-URL: ayojs/ayo#120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #16700
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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 this pull request may close these issues.

7 participants