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

addaleax · 2018-05-22T22:57:19Z

Backport #16700 + #16970 + #17083 (the latter two depend on the first one)

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 <[email protected]> PR-URL: nodejs#16700 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: nodejs#16700 Reviewed-By: James M Snell <[email protected]>

Replace raw pointers in task queues with std::unique_ptr. This makes ownership obvious. PR-URL: nodejs#16970 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Use std::unique_ptr for delayed tasks in the scheduled delayed tasks vector. This makes it clear that the vector has ownership of the delayed tasks and is responsible for deleting them. Use a custom deleter for the pointers because libuv needs to close the handle and then delete the data. Provide the handle when creating the pointer instead of invoking the special delete action everytime an element is removed from the vector. PR-URL: nodejs#17083 Reviewed-By: Anna Henningsen <[email protected]>

danbev · 2018-05-23T09:29:25Z

src/node_platform.cc

-  flush_tasks_.data = static_cast<void*>(this);
-  uv_unref(reinterpret_cast<uv_handle_t*>(&flush_tasks_));
+PerIsolatePlatformData::PerIsolatePlatformData(
+    v8::Isolate* isolate, uv_loop_t* loop)


Nit: Perhaps use an unqualified name for the Isolate type here?

Since this is a backport is it safe to assume this is what already landed upstream

Yeah, I’d prefer to avoid stylistic changes outside of master :)

MylesBorins · 2018-05-23T17:51:40Z

Could you also include #17133?

addaleax · 2018-05-23T17:55:54Z

@MylesBorins Already included that as part of the backporting process (in order to make make test pass locally), so I think there’s nothing to do about that one

Or are you seeing errors because of that?

MylesBorins · 2018-05-23T18:11:33Z

@addaleax didn't know it was included because not documented. Was just going through the backlog 😄

MylesBorins · 2018-06-14T20:27:24Z

landed in 35055a1...beb45ac

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 <[email protected]> PR-URL: #16700 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: #16700 Reviewed-By: James M Snell <[email protected]>

Replace raw pointers in task queues with std::unique_ptr. This makes ownership obvious. Backport-PR-URL: #20901 PR-URL: #16970 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Use std::unique_ptr for delayed tasks in the scheduled delayed tasks vector. This makes it clear that the vector has ownership of the delayed tasks and is responsible for deleting them. Use a custom deleter for the pointers because libuv needs to close the handle and then delete the data. Provide the handle when creating the pointer instead of invoking the special delete action everytime an element is removed from the vector. Backport-PR-URL: #20901 PR-URL: #17083 Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> PR-URL: #16700 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: #16700 Reviewed-By: James M Snell <[email protected]>

Replace raw pointers in task queues with std::unique_ptr. This makes ownership obvious. Backport-PR-URL: #20901 PR-URL: #16970 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Use std::unique_ptr for delayed tasks in the scheduled delayed tasks vector. This makes it clear that the vector has ownership of the delayed tasks and is responsible for deleting them. Use a custom deleter for the pointers because libuv needs to close the handle and then delete the data. Provide the handle when creating the pointer instead of invoking the special delete action everytime an element is removed from the vector. Backport-PR-URL: #20901 PR-URL: #17083 Reviewed-By: Anna Henningsen <[email protected]>

addaleax and others added 4 commits May 23, 2018 00:42

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. v8.x labels May 22, 2018

addaleax mentioned this pull request May 22, 2018

src: use unique_ptr for scheduled delayed tasks #17083

Closed

2 tasks

TimothyGu approved these changes May 23, 2018

View reviewed changes

danbev approved these changes May 23, 2018

View reviewed changes

MylesBorins closed this Jun 14, 2018

MylesBorins force-pushed the v8.x-staging branch from 084ef60 to 859dc64 Compare June 14, 2018 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented May 22, 2018

danbev May 23, 2018

MylesBorins May 23, 2018

addaleax May 23, 2018

MylesBorins commented May 23, 2018

addaleax commented May 23, 2018

MylesBorins commented May 23, 2018

MylesBorins commented Jun 14, 2018

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

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

Conversation

addaleax commented May 22, 2018

danbev May 23, 2018

Choose a reason for hiding this comment

MylesBorins May 23, 2018

Choose a reason for hiding this comment

addaleax May 23, 2018

Choose a reason for hiding this comment

MylesBorins commented May 23, 2018

addaleax commented May 23, 2018

MylesBorins commented May 23, 2018

MylesBorins commented Jun 14, 2018