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

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

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

addaleax and others added 4 commits May 23, 2018 00:42
@addaleax
src: prepare v8 platform for multi-isolate support
9065001 
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
src: cancel pending delayed platform tasks on exit
e56610b 
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>
@fhinkel @addaleax
src: use unique_ptr in platform implementation
916fa14 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

PR-URL: nodejs#16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@fhinkel @addaleax
src: use unique_ptr for scheduled delayed tasks
4c88ddb 
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 <anna@addaleax.net>
@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. v8.x labels May 22, 2018
@addaleax addaleax mentioned this pull request May 22, 2018
Closed
2 tasks
danbev
danbev approved these changes May 23, 2018
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@MylesBorins
Copy link
Contributor

Could you also include #17133?

@addaleax
Copy link
Member Author

@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
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

landed in 35055a1...beb45ac

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@addaleax @MylesBorins
src: prepare v8 platform for multi-isolate support
68ad21b 
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
@addaleax @MylesBorins
src: cancel pending delayed platform tasks on exit
8b65512 
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 pushed a commit that referenced this pull request Jun 14, 2018
@fhinkel @MylesBorins
src: use unique_ptr in platform implementation
becb2df 
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@fhinkel @MylesBorins
src: use unique_ptr for scheduled delayed tasks
beb45ac 
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 <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@addaleax @rvagg
src: prepare v8 platform for multi-isolate support
bf8068e 
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
@addaleax @rvagg
src: cancel pending delayed platform tasks on exit
e932754 
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>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@fhinkel @rvagg
src: use unique_ptr in platform implementation
2148b19 
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@fhinkel @rvagg
src: use unique_ptr for scheduled delayed tasks
2cf2635 
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 <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MylesBorins MylesBorins MylesBorins left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @MylesBorins @danbev @TimothyGu @nodejs-github-bot @fhinkel