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.

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

node-api: faster threadsafe_function #38506

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented May 2, 2021

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 indutny requested a review from addaleax May 2, 2021 19:15
@github-actions 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
@indutny
Copy link
Member Author

indutny commented May 2, 2021

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
@indutny
Copy link
Member Author

indutny commented May 2, 2021

Now fixed. PTAL @addaleax

src/node_api.cc Show resolved Hide resolved
@addaleax addaleax added the node-api Issues and PRs related to the Node-API. label May 2, 2021
queue.push(data);
Send();
Copy link
Member Author

@indutny indutny May 2, 2021

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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 😂

@indutny
Copy link
Member Author

indutny commented May 2, 2021

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

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels May 4, 2021
indutny added a commit that referenced this pull request May 5, 2021
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
Copy link
Member Author

indutny commented May 5, 2021

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.

@indutny indutny closed this May 5, 2021
@indutny indutny deleted the feature/faster-threadsafe-function branch May 5, 2021 01:41
@indutny indutny mentioned this pull request May 5, 2021
indutny added a commit to indutny/io.js that referenced this pull request May 5, 2021
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

Backport-PR-URL: nodejs#38506
@indutny
Copy link
Member Author

indutny commented May 5, 2021

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

indutny added a commit to indutny/io.js that referenced this pull request May 5, 2021
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>
@indutny
Copy link
Member Author

indutny commented May 5, 2021

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

indutny-signal pushed a commit to indutny/electron that referenced this pull request May 5, 2021
indutny added a commit to indutny/electron that referenced this pull request May 5, 2021
deepak1556 pushed a commit to electron/electron that referenced this pull request May 6, 2021
trop bot pushed a commit to electron/electron that referenced this pull request May 6, 2021
trop bot pushed a commit to electron/electron that referenced this pull request May 6, 2021
codebytere pushed a commit to electron/electron that referenced this pull request May 6, 2021
MarshallOfSound pushed a commit to electron/electron that referenced this pull request May 7, 2021
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
Backports: nodejs/node#38506

Co-authored-by: Fedor Indutny <fedor@indutny.com>
@mhdawson
Copy link
Member

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

targos pushed a commit that referenced this pull request May 17, 2021
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>
codebytere added a commit to electron/electron that referenced this pull request May 20, 2021
targos pushed a commit that referenced this pull request May 30, 2021
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>
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
targos pushed a commit that referenced this pull request Jun 5, 2021
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>
targos pushed a commit that referenced this pull request Jun 5, 2021
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>
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants