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

@indutny 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 indutny commented May 2, 2021

Now fixed. PTAL @addaleax

@indutny indutny force-pushed the feature/faster-threadsafe-function branch from 37b10a7 to 42ccd43 May 2, 2021
src/node_api.cc Show resolved Hide resolved
queue.push(data);
Send();
Copy link
Member Author

@indutny indutny May 2, 2021

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

@indutny indutny May 2, 2021

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 indutny commented May 2, 2021

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

Trott
Trott approved these changes May 3, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 4, 2021

jasnell
jasnell approved these changes May 4, 2021
indutny added a commit that referenced this issue 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 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
@indutny indutny mentioned this pull request May 5, 2021
indutny added a commit to indutny/io.js that referenced this issue 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 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 issue 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 indutny commented May 5, 2021

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

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

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

@mhdawson mhdawson commented May 10, 2021

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

targos added a commit that referenced this issue 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 issue May 20, 2021
targos added a commit that referenced this issue 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 issue May 31, 2021
codebytere added a commit to electron/electron that referenced this issue May 31, 2021
targos added a commit that referenced this issue 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 added a commit that referenced this issue 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 issue Jun 8, 2021
codebytere added a commit to electron/electron that referenced this issue Jun 9, 2021
codebytere added a commit to electron/electron that referenced this issue Jun 10, 2021
targos added a commit that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants