src: multi-isolate support for NodePlatform #16700

addaleax · 2017-11-02T23:59:13Z

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 · 2017-11-10T22:16:30Z

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

addaleax · 2017-11-12T13:15:50Z

Landed in c7ad729, 2cedff9

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>

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 · 2017-11-12T13:33:07Z

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

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>

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 · 2017-12-11T20:49:36Z

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

gibfahn · 2017-12-13T16:03:45Z

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 😁 )

- 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

* 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 · 2018-01-19T18:33:19Z

ping re: backport

* 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 · 2018-02-27T02:49:56Z

ping re: 8.x and 6.x

gibfahn · 2018-03-20T13:40:24Z

ping @addaleax

* 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 · 2018-05-22T14:56:14Z

ping @addaleax

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>

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>

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>

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>

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>

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 added 2 commits November 3, 2017 00:44

src: prepare v8 platform for multi-isolate support

e65b6ae

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>

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 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 mentioned this pull request Nov 2, 2017

When Node addon creates a new isolate, use it to execute JS code and let it triggers GC, it will crash #16658

Closed

jasnell approved these changes Nov 3, 2017

View reviewed changes

addaleax closed this Nov 12, 2017

addaleax deleted the platform-multiple-isolates branch November 12, 2017 13:15

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added lts-watch-v6.x labels Dec 19, 2017

addaleax removed the lts-watch-v6.x label May 22, 2018

addaleax mentioned this pull request May 22, 2018

[v8.x] backport some V8 platform changes #20901

Closed

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: multi-isolate support for NodePlatform #16700

src: multi-isolate support for NodePlatform #16700

addaleax commented Nov 2, 2017

addaleax commented Nov 10, 2017

addaleax commented Nov 12, 2017

refack commented Nov 12, 2017

MylesBorins commented Dec 11, 2017

gibfahn commented Dec 13, 2017

MylesBorins commented Jan 19, 2018

MylesBorins commented Feb 27, 2018

gibfahn commented Mar 20, 2018

MylesBorins commented May 22, 2018

src: multi-isolate support for NodePlatform #16700

src: multi-isolate support for NodePlatform #16700

Conversation

addaleax commented Nov 2, 2017

Checklist

Affected core subsystem(s)

addaleax commented Nov 10, 2017

addaleax commented Nov 12, 2017

refack commented Nov 12, 2017

MylesBorins commented Dec 11, 2017

gibfahn commented Dec 13, 2017

MylesBorins commented Jan 19, 2018

MylesBorins commented Feb 27, 2018

gibfahn commented Mar 20, 2018

MylesBorins commented May 22, 2018