src: implement v8::TaskRunner API in NodePlatform #17134

addaleax · 2017-11-19T13:42:12Z

V8 is switching APIs for scheduling tasks. Implement the new APIs.

Fixes: nodejs/node-v8#24
Refs: v8/v8@c690f54

Note: This cherry-picks the API change itself (which is scheduled for V8 6.4) into this branch. We can wait for that release to perform the change if we like, but I’m not sure how we’d do node-canary coverage without this?

/cc @nodejs/v8 @gahaas

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (the WASM test fails without it, see the linked issue)
commit message follows commit guidelines

Affected core subsystem(s)

src/node_platform

~~CI: https://ci.nodejs.org/job/node-test-commit/14146/~~
CI: https://ci.nodejs.org/job/node-test-commit/14147/

gahaas

lgtm with nits. Thanks for implementing this so quickly!

gahaas · 2017-11-20T07:55:48Z

src/node_platform.cc

@@ -24,6 +24,46 @@ static void BackgroundRunner(void* data) {
  }
 }

+BackgroundTaskRunner::BackgroundTaskRunner(int thread_pool_size) {
+  for (int i = 0; i < thread_pool_size; i++) {
+    uv_thread_t* t = new uv_thread_t();


Make t already a std::unique_ptr<uv_thread_t>. Thereby you don't need the "delete" below, and it is more robust in the future.

gahaas · 2017-11-20T07:57:09Z

src/node_platform.cc

+void BackgroundTaskRunner::PostDelayedTask(std::unique_ptr<v8::Task> task,
+                                           double delay_in_seconds) {
+  // It's unclear whether this is required at all for background tasks.
+  PostTask(std::move(task));


I think it is better to leave this unimplemented than implementing it incorrectly.

@gahaas Is there any case in which V8 would call this method? What happens when the delay is not being matched?

I am okay with leaving it unimplemented but I’d like to understand better what the implications of each option are

@addaleax At the moment V8 does not call this method. The reason it exists is to share a common interface between the foreground task runner and the background task runner.

I think if a developer ever wants to use this method, the developer does not expect that the platform will just ignore the delay. Therefore posting the task without delay does not provide much value to the developer IMO. Leaving the method unimplemented at least tells the developer explicitly that no implementation with expected behavior exists.

bnoordhuis · 2017-11-20T13:01:45Z

src/node_platform.cc

+}
+
+void BackgroundTaskRunner::PostIdleTask(std::unique_ptr<v8::IdleTask> task) {
+  CHECK(false && "idle tasks not enabled");


UNREACHABLE()? Doesn't print a message although that could be added.

bnoordhuis · 2017-11-20T13:02:31Z

src/node_platform.cc

-void PerIsolatePlatformData::CallOnForegroundThread(
-    std::unique_ptr<Task> task) {
+void PerIsolatePlatformData::PostIdleTask(std::unique_ptr<v8::IdleTask> task) {
+  CHECK(false && "idle tasks not enabled");


Original commit message: [platform] Add TaskRunner to the platform API With the existing platform API it is not possible to post foreground tasks from background tasks. This is, however, required to implement asynchronous compilation for WebAssembly. With this CL we add the concept of a TaskRunner to the platform API. The TaskRunner contains all data needed to post a foreground task and can be used both from a foreground task and a background task. Eventually the TaskRunner should replace the existing API. In addition, this CL contains a default implementation of the TaskRunner. This implementation has tempory workaround for platforms which do not provide a TaskRunner implementation yet. This default implementation should be deleted again when all platforms provide a TaskRunner implementation. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802 Reviewed-on: https://chromium-review.googlesource.com/741588 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#49041} Refs: v8/v8@c690f54

Original commit message: [platform] Return task runners as shared_ptr At the moment, task runners are returned as unique_ptr. This is inconvenient, however. In all implementations I did, the platform holds a shared pointer of the task runner and wraps it in a wrapper class just to return it as a unique_ptr. With this CL the platform API is changed to return a shared_ptr directly. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c Reviewed-on: https://chromium-review.googlesource.com/768867 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#49366} Refs: v8/v8@98c40a4

V8 is switching APIs for scheduling tasks. Implement the new APIs. Fixes: nodejs/node-v8#24 Refs: v8/v8@c690f54

addaleax · 2017-11-21T07:50:08Z

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

Original commit message: [platform] Add TaskRunner to the platform API With the existing platform API it is not possible to post foreground tasks from background tasks. This is, however, required to implement asynchronous compilation for WebAssembly. With this CL we add the concept of a TaskRunner to the platform API. The TaskRunner contains all data needed to post a foreground task and can be used both from a foreground task and a background task. Eventually the TaskRunner should replace the existing API. In addition, this CL contains a default implementation of the TaskRunner. This implementation has tempory workaround for platforms which do not provide a TaskRunner implementation yet. This default implementation should be deleted again when all platforms provide a TaskRunner implementation. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802 Reviewed-on: https://chromium-review.googlesource.com/741588 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49041} Refs: v8/v8@c690f54 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Return task runners as shared_ptr At the moment, task runners are returned as unique_ptr. This is inconvenient, however. In all implementations I did, the platform holds a shared pointer of the task runner and wraps it in a wrapper class just to return it as a unique_ptr. With this CL the platform API is changed to return a shared_ptr directly. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c Reviewed-on: https://chromium-review.googlesource.com/768867 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49366} Refs: v8/v8@98c40a4 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

V8 is switching APIs for scheduling tasks. Implement the new APIs. Fixes: nodejs/node-v8#24 Refs: v8/v8@c690f54 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-11-21T15:40:23Z

Landed in 5d35a1a, 2540e3d, and f005656

targos · 2017-11-22T07:06:28Z

Each cherry-pick should have incremented the v8_embedder_string.

Original commit message: [platform] Add TaskRunner to the platform API With the existing platform API it is not possible to post foreground tasks from background tasks. This is, however, required to implement asynchronous compilation for WebAssembly. With this CL we add the concept of a TaskRunner to the platform API. The TaskRunner contains all data needed to post a foreground task and can be used both from a foreground task and a background task. Eventually the TaskRunner should replace the existing API. In addition, this CL contains a default implementation of the TaskRunner. This implementation has tempory workaround for platforms which do not provide a TaskRunner implementation yet. This default implementation should be deleted again when all platforms provide a TaskRunner implementation. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802 Reviewed-on: https://chromium-review.googlesource.com/741588 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#49041} Refs: v8/v8@c690f54 PR-URL: nodejs#17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Return task runners as shared_ptr At the moment, task runners are returned as unique_ptr. This is inconvenient, however. In all implementations I did, the platform holds a shared pointer of the task runner and wraps it in a wrapper class just to return it as a unique_ptr. With this CL the platform API is changed to return a shared_ptr directly. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c Reviewed-on: https://chromium-review.googlesource.com/768867 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#49366} Refs: v8/v8@98c40a4 PR-URL: nodejs#17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Add TaskRunner to the platform API With the existing platform API it is not possible to post foreground tasks from background tasks. This is, however, required to implement asynchronous compilation for WebAssembly. With this CL we add the concept of a TaskRunner to the platform API. The TaskRunner contains all data needed to post a foreground task and can be used both from a foreground task and a background task. Eventually the TaskRunner should replace the existing API. In addition, this CL contains a default implementation of the TaskRunner. This implementation has tempory workaround for platforms which do not provide a TaskRunner implementation yet. This default implementation should be deleted again when all platforms provide a TaskRunner implementation. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802 Reviewed-on: https://chromium-review.googlesource.com/741588 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49041} Refs: v8/v8@c690f54 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Return task runners as shared_ptr At the moment, task runners are returned as unique_ptr. This is inconvenient, however. In all implementations I did, the platform holds a shared pointer of the task runner and wraps it in a wrapper class just to return it as a unique_ptr. With this CL the platform API is changed to return a shared_ptr directly. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c Reviewed-on: https://chromium-review.googlesource.com/768867 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49366} Refs: v8/v8@98c40a4 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Add TaskRunner to the platform API With the existing platform API it is not possible to post foreground tasks from background tasks. This is, however, required to implement asynchronous compilation for WebAssembly. With this CL we add the concept of a TaskRunner to the platform API. The TaskRunner contains all data needed to post a foreground task and can be used both from a foreground task and a background task. Eventually the TaskRunner should replace the existing API. In addition, this CL contains a default implementation of the TaskRunner. This implementation has tempory workaround for platforms which do not provide a TaskRunner implementation yet. This default implementation should be deleted again when all platforms provide a TaskRunner implementation. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802 Reviewed-on: https://chromium-review.googlesource.com/741588 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49041} Refs: v8/v8@c690f54 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: [platform] Return task runners as shared_ptr At the moment, task runners are returned as unique_ptr. This is inconvenient, however. In all implementations I did, the platform holds a shared pointer of the task runner and wraps it in a wrapper class just to return it as a unique_ptr. With this CL the platform API is changed to return a shared_ptr directly. R=rmcilroy@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c Reviewed-on: https://chromium-review.googlesource.com/768867 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#49366} Refs: v8/v8@98c40a4 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

V8 is switching APIs for scheduling tasks. Implement the new APIs. Fixes: nodejs/node-v8#24 Refs: v8/v8@c690f54 PR-URL: #17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

V8 is switching APIs for scheduling tasks. Implement the new APIs. Fixes: nodejs/node-v8#24 Refs: v8/v8@c690f54 PR-URL: nodejs#17134 Fixes: nodejs/node-v8#24 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Nov 19, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Nov 19, 2017

addaleax mentioned this pull request Nov 19, 2017

broken test-wasm-simple nodejs/node-v8#24

Closed

addaleax force-pushed the fix-platform branch from 2f9c744 to c294f9a Compare November 19, 2017 14:03

gahaas approved these changes Nov 20, 2017

View reviewed changes

fhinkel approved these changes Nov 20, 2017

View reviewed changes

bnoordhuis approved these changes Nov 20, 2017

View reviewed changes

jasnell approved these changes Nov 20, 2017

View reviewed changes

addaleax added 5 commits November 21, 2017 08:44

src: implement v8::TaskRunner API in NodePlatform

8be600b

V8 is switching APIs for scheduling tasks. Implement the new APIs. Fixes: nodejs/node-v8#24 Refs: v8/v8@c690f54

[squash] gahaas nit

2cd135b

[squash] UNREACHABLE(); all the things

9bd517e

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017

addaleax force-pushed the fix-platform branch from 5a3abae to 9bd517e Compare November 21, 2017 07:45

jasnell closed this Nov 21, 2017

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017

addaleax deleted the fix-platform branch November 21, 2017 18:33

addaleax mentioned this pull request Nov 22, 2017

deps: update V8 to 6.3 #16271

Closed

2 tasks

targos added dont-land-on-v4.x labels Dec 6, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

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: implement v8::TaskRunner API in NodePlatform #17134

src: implement v8::TaskRunner API in NodePlatform #17134

addaleax commented Nov 19, 2017 •

edited

Loading

gahaas left a comment

gahaas Nov 20, 2017

gahaas Nov 20, 2017

addaleax Nov 20, 2017

gahaas Nov 20, 2017

addaleax Nov 21, 2017

bnoordhuis Nov 20, 2017

addaleax Nov 21, 2017

bnoordhuis Nov 20, 2017

addaleax commented Nov 21, 2017

jasnell commented Nov 21, 2017

targos commented Nov 22, 2017

src: implement v8::TaskRunner API in NodePlatform #17134

src: implement v8::TaskRunner API in NodePlatform #17134

Conversation

addaleax commented Nov 19, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gahaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 21, 2017

jasnell commented Nov 21, 2017

targos commented Nov 22, 2017

addaleax commented Nov 19, 2017 •

edited

Loading