node-api: faster threadsafe_function by indutny · Pull Request #38506 · nodejs/node · GitHub

Invoke threadsafe_function during the same tick and avoid marshalling
costs between threads and/or churning event loop if either:

There's a queued call already
Push() is called while the main thread was running
threadsafe_function

Sorry, this is not yet ready for review. I've realized that the removal of uv_idle_t is no longer justified now that the iteration count is limited. Give me a minute to address this.

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function

Now fixed. PTAL @addaleax

I've swapped the order since uv_async_send() error is treated as a hard failure and it is more logical to push before the notification.

Looks stupid, but there is an assert in the binding that the queue block at least once, and I didn't want to change .c code 😂

/me sets a reminder to land this after 48 hours.

CI: https://ci.nodejs.org/job/node-test-pull-request/37919/

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: #38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 7abc7e4. Thanks for reviews!

@jasnell @addaleax what's our process for backports? Do I need to submit a PR for 14.x? I assume this should land cleanly there since the code didn't change.

Opened a backport PR: #38543 . Let me know if I messed this up!

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: nodejs#38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Here's a PR against v16.x-staging: #38547

Backports: nodejs/node#38506

Backports: nodejs/node#38506 Co-authored-by: Fedor Indutny <fedor@indutny.com>

We have a persistent failure in the node-addon-api testing which started around this time. nodejs/node-addon-api#994

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: #38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs/node#38506

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: #38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs/node#38506

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: #38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs/node#38506

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function PR-URL: #38506 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

indutny requested a review from addaleax May 2, 2021 19:15

github-actions Bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 2, 2021

node-api: faster threadsafe_function

42ccd43

Invoke threadsafe_function during the same tick and avoid marshalling costs between threads and/or churning event loop if either: 1. There's a queued call already 2. `Push()` is called while the main thread was running threadsafe_function

indutny force-pushed the feature/faster-threadsafe-function branch from 37b10a7 to 42ccd43 Compare May 2, 2021 19:21

indutny mentioned this pull request May 2, 2021

fix: allow Node.js to manage microtasks queue electron/electron#28957

Merged

5 tasks

indutny commented May 2, 2021

View reviewed changes

Comment thread test/node-api/test_threadsafe_function/test.js Outdated

addaleax approved these changes May 2, 2021

View reviewed changes

Comment thread src/node_api.cc

addaleax added the node-api Issues and PRs related to the Node-API. label May 2, 2021

simplify

9835c84

indutny commented May 2, 2021

View reviewed changes

addaleax approved these changes May 2, 2021

View reviewed changes

Trott approved these changes May 3, 2021

View reviewed changes

jasnell approved these changes May 4, 2021

View reviewed changes

jasnell added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels May 4, 2021

indutny closed this May 5, 2021

indutny deleted the feature/faster-threadsafe-function branch May 5, 2021 01:41

indutny mentioned this pull request May 5, 2021

v14.17.0 proposal #38507

Merged

indutny mentioned this pull request May 5, 2021

[v14.x backport] node-api: faster threadsafe_function #38543

Closed

indutny added the backport-open-v14.x label May 5, 2021

indutny mentioned this pull request May 5, 2021

[v16.x backport] node-api: faster threadsafe_function #38547

Closed

indutny-signal pushed a commit to indutny/electron that referenced this pull request May 5, 2021

chore: cherry-pick 7abc7e45b2 from node

74a6476

Backports: nodejs/node#38506

indutny added a commit to indutny/electron that referenced this pull request May 5, 2021

chore: cherry-pick 7abc7e45b2 from node

5240c07

Backports: nodejs/node#38506

indutny mentioned this pull request May 5, 2021

chore: cherry-pick 7abc7e45b2 from node electron/electron#29021

Merged

5 tasks

jrose-signal mentioned this pull request May 6, 2021

signal-neon-futures: Only run Rust futures on an existing EventQueue signalapp/libsignal#290

Merged

deepak1556 pushed a commit to electron/electron that referenced this pull request May 6, 2021

chore: cherry-pick 7abc7e45b2 from node (#29021 )

ad4def9

Backports: nodejs/node#38506

trop Bot pushed a commit to electron/electron that referenced this pull request May 6, 2021

chore: cherry-pick 7abc7e45b2 from node

37c5a19

Backports: nodejs/node#38506

trop Bot pushed a commit to electron/electron that referenced this pull request May 6, 2021

chore: cherry-pick 7abc7e45b2 from node

e9a185c

Backports: nodejs/node#38506

codebytere pushed a commit to electron/electron that referenced this pull request May 6, 2021

chore: cherry-pick 7abc7e45b2 from node

481c4ec

Backports: nodejs/node#38506

MarshallOfSound pushed a commit to electron/electron that referenced this pull request May 7, 2021

chore: cherry-pick 7abc7e45b2 from node (#29048 )

ed0b654

Backports: nodejs/node#38506 Co-authored-by: Fedor Indutny <fedor@indutny.com>

MarshallOfSound pushed a commit to electron/electron that referenced this pull request May 7, 2021

chore: cherry-pick 7abc7e45b2 from node (#29047 )

47431a5

Backports: nodejs/node#38506 Co-authored-by: Fedor Indutny <fedor@indutny.com>

jrose-signal mentioned this pull request May 7, 2021

Optimization: share a single napi_threadsafe_function for all EventQueues neon-bindings/neon#727

Closed

targos mentioned this pull request May 18, 2021

v16.2.0 release proposal #38719

Merged

codebytere added a commit to electron/electron that referenced this pull request May 20, 2021

node-api: faster threadsafe_function

1b2b073

nodejs/node#38506

targos removed the backport-open-v14.x label May 30, 2021

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

node-api: faster threadsafe_function

f2b13f5

nodejs/node#38506

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

node-api: faster threadsafe_function

25dbafb

nodejs/node#38506

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021

node-api: faster threadsafe_function

fb45c97

nodejs/node#38506

codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021

node-api: faster threadsafe_function

673d324

nodejs/node#38506

codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021

node-api: faster threadsafe_function

aab0e27

nodejs/node#38506

kjvalencik mentioned this pull request Jun 29, 2021

Performance regression in Channel (EventQueue) for some usage patterns neon-bindings/neon#762

Open

+              // `kMaxIterationCount` in `src/node_api.cc`
+              .then(() => testWithJSMarshaller({
+                threadStarter: 'StartThreadNonblocking',
+                maxQueueSize: binding.ARRAY_LENGTH >>> 1,

node-api: faster threadsafe_function by indutny · Pull Request #38506 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: faster threadsafe_function - #38506

node-api: faster threadsafe_function#38506
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:feature/faster-threadsafe-function

indutny commented May 2, 2021

Uh oh!

indutny commented May 2, 2021

Uh oh!

indutny commented May 2, 2021

Uh oh!

Uh oh!

Uh oh!

indutny May 2, 2021 •
edited
Loading

Uh oh!

Uh oh!

indutny May 2, 2021

Uh oh!

indutny commented May 2, 2021

Uh oh!

nodejs-github-bot commented May 4, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

mhdawson commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

-                      return napi_generic_failure;
-                    }
                     queue.push(data);
+                    Send();

node-api: faster threadsafe_function by indutny · Pull Request #38506 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: faster threadsafe_function - #38506

node-api: faster threadsafe_function#38506indutny wants to merge 2 commits intonodejs:masternodejs/node:masterfrom indutny:feature/faster-threadsafe-functionindutny/io.js:feature/faster-threadsafe-functionCopy head branch name to clipboard

Conversation

indutny commented May 2, 2021

Uh oh!

indutny commented May 2, 2021

Uh oh!

indutny commented May 2, 2021

Uh oh!

Uh oh!

Uh oh!

indutny May 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny May 2, 2021

Choose a reason for hiding this comment

Uh oh!

indutny commented May 2, 2021

Uh oh!

nodejs-github-bot commented May 4, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

indutny commented May 5, 2021

Uh oh!

mhdawson commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

node-api: faster threadsafe_function#38506
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:feature/faster-threadsafe-function

indutny May 2, 2021 •
edited
Loading