Skip to content
This repository has been archived by the owner on Aug 31, 2018. It is now read-only.

src: prepare v8 platform for multi-isolate support (workers preparation) #89

Merged
merged 1 commit into from
Oct 7, 2017

Conversation

addaleax
Copy link
Contributor

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

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

@Qard
Copy link
Member

Qard commented Sep 30, 2017

I see a bunch of NodePlatform method signatures changing. Do you think it'd make sense to keep no-isolate versions of those which just iterate over all the isolates and call the isolate versions of those methods to reduce change impact on embedders?

@addaleax
Copy link
Contributor Author

to reduce change impact on embedders?

@Qard I think that’s just FlushForegroundTasks() and DrainBackgroundTasks(), right? For those two that’s not going to be possible because they basically need to be called from the thread where JS is executing, i.e. the one that is currently responsible for a given Isolate

src/node.cc Outdated
@@ -259,28 +259,32 @@ static v8::Isolate* node_isolate;

node::DebugOptions debug_options;

#ifdef NODE_USE_V8_PLATFORM
NodePlatform* NodePlatform::platform = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, we don't actually need to switch this to a static field. It's initialized the same way in the Initialize() method, it's just pointing at NodePlatform::platform rather than _platform. It doesn't seem like NodePlatform itself actually makes any use of that reference. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a new approach :)

@addaleax addaleax force-pushed the prepare-v8-platform branch 2 times, most recently from 9cd5c32 to 995afd6 Compare October 4, 2017 22:43
@addaleax
Copy link
Contributor Author

addaleax commented Oct 4, 2017

@ayojs/core I’d appreciate some advice on how to make reviewing this easier – I realize this is a pretty niché topic within Node/V8…

src/node.h Outdated
};

// If `platform` is passed, it will be used to register new worker instances.
// It can be `nullptr`, in which case creating new workers will not work.
Copy link
Member

Choose a reason for hiding this comment

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

The word "creating new workers" is vague to me. Creating new workers in the isolate for which isolate data are created? or creating new workers whose isolate is the isolate for which isolate data are created?

static void FlushTasks(uv_async_t* handle) {
NodePlatform* platform = static_cast<NodePlatform*>(handle->data);
platform->FlushForegroundTasksInternal();
void PerIsolatePlatformData::FlushTasks(uv_async_t* handle) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move this function to where all the other PerIsolatePlatformData methods are?

@@ -3,13 +3,18 @@

#include <queue>
#include <vector>
#include <unordered_map>
Copy link
Member

Choose a reason for hiding this comment

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

order :)

src/node.h Outdated
@@ -209,8 +210,26 @@ NODE_EXTERN void Init(int* argc,
class IsolateData;
class Environment;

NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate,
struct uv_loop_s* loop);
class WorkerSupportingPlatform : public v8::Platform {
Copy link
Member

Choose a reason for hiding this comment

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

Can the name be made more generic, like maybe MultiIsolatePlatform? After all, this is a public API.

@addaleax
Copy link
Contributor Author

addaleax commented Oct 7, 2017

@TimothyGu @Qard Fixed review nits, please take another look if you can :)

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#89
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@addaleax addaleax merged commit 2f47b5b into ayojs:latest Oct 7, 2017
@addaleax addaleax deleted the prepare-v8-platform branch October 7, 2017 19:01
@addaleax addaleax added the worker label Oct 8, 2017
addaleax added a commit to addaleax/node that referenced this pull request Nov 2, 2017
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 a commit to nodejs/node 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>
MylesBorins pushed a commit to nodejs/node 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>
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>
MylesBorins pushed a commit to nodejs/node 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>
rvagg pushed a commit to nodejs/node 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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants