-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
worker: add Notification API #59691
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
base: main
Are you sure you want to change the base?
worker: add Notification API #59691
Conversation
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/17355601584 |
|
Can you mark it as experimental? |
|
@mcollina Done |
|
What's the performance gap between this and postMessage? I know it might sound hard work, but is there any optimization potential in postMessage? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59691 +/- ##
========================================
Coverage 88.27% 88.28%
========================================
Files 701 701
Lines 206640 206764 +124
Branches 39739 39748 +9
========================================
+ Hits 182406 182535 +129
+ Misses 16277 16269 -8
- Partials 7957 7960 +3
🚀 New features to boost your workflow:
|
addaleax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API purposely does not allow to attach any data to the notifications. The two threads must use SharedArrayBuffers to share data.
Yeah, I'd say you're on the right track here – if you want the fastest possible low-level communication between threads, SharedArrayBuffers and Atomics is the best way to do this. To be completely honest, it seems like this API here would not add value on top of that, so the fact that it brings in additional complexity that would need to be maintained would make me suggest not to add it.
For this PR specifically, the current implementation breaks with a number of contracts and conventions for Node.js internals; if you want to pursue adding this API, I'd highly encourage looking at our src/README.md file for information about how to work with the C++ side of Node.js internals, and look at other places where we integrate with e.g. libuv handles and asynchronous messaging between threads.
I've measured the approach with SharedArrayBuffers and Atomics, and unfortunately it's slower than our |
@mcollina That's very surprising, yes. Have we to talked to the V8 team about this? Atomics being slower than regular asynchronous communication seems like something that would be a mistake, honestly. |
6445ba9 to
a870d24
Compare
|
Thanks a huge lot for your thorough review. @jasnell For the record, when I say "anything", I even include flipping Also, it will provide a node-ish natural way to synchronize work without blocking threads with @mcollina I'll try to provide benchmarks in the new few days. |
I think this is actually pretty good from an implementation perspective, aside from the lack of async tracking.
Yeah, I think the problem is that you're mostly describing how this API differs from the others, but not why that would serve real-world use cases. It seems like you're saying "this is better than
Why is it bad to expect a user to change a byte in shared memory for communication purposes?
So, to be clear, the big issues here are:
|
| v8::Local<v8::Function> callback = | ||
| notifications_callbacks_[id].Get(isolate); | ||
| MakeCallback(isolate, process, callback, 0, nullptr, {0, 0}) | ||
| .ToLocalChecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using ToLocalChecked if possible, if the callbacks throw it will crash the process. You'll want to make sure the error gets appropriately propagated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that. But I see no difference in how the error is propagated, I still get:
node:internal/event_target:1101
process.nextTick(() => { throw err; });
^
Error: kaboom
at process.<anonymous> (/Volumes/DATI/Users/Shogun/Programmazione/OSS/nodejs/test/parallel/test-process-notifications-error.js:22:11)
Node.js v25.0.0-pre
Am I missing anything?
This PR adds a new notification API to the
workermodule made of the following methods:registerNotificationsendNotificationunregisterNotificationunregisterNotificationsgetNotificationsThe idea behind it is being able to trigger a callback in another module without having to go throughout all workers messaging and event handling.
The API purposely does not allow to attach any data to the notifications. The two threads must use
SharedArrayBuffers to share data.The usecase is threads that must transfer data back and forth the quickest way possible. For instance to simulate a socket via threads.
This is the typical usage: