src: avoid env lookups and a global handle in InternalCallbackScope - #66316
nigrosimone wants to merge 3 commits into
Conversation
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
b4d12f6 to
e83b5b2
Compare
InternalCallbackScope looks up the Environment from the isolate two times per call, inside async_context_frame::exchange, and it keeps the prior async context frame in a v8::Global also when there is no frame, that is the common case. Every call from native code into JS pays this: MakeCallback, CallbackScope, AsyncWrap, Node-API. Now the scope passes the Environment it already has, the option is read with an inline accessor instead of copying the shared_ptr, and the global handle is created only when the prior frame is not undefined. benchmark/napi/make_callback, Node 26.3.0 built with and without this change, Linux x64, 30 runs: from 202-208 ns to 155-159 ns per call. Refs: nodejs/performance#24 Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
e83b5b2 to
e9c75be
Compare
The addon calls into JS with node::MakeCallback from a libuv timer, so every call opens a top-level callback scope, like an I/O callback does. Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
A CallbackScope must restore the async context frame that was active before it, when there was none and when there was one. Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
e9c75be to
1221198
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66316 +/- ##
==========================================
- Coverage 90.38% 90.36% -0.02%
==========================================
Files 790 790
Lines 274497 274522 +25
Branches 52557 52567 +10
==========================================
- Hits 248100 248076 -24
- Misses 16879 16927 +48
- Partials 9518 9519 +1
🚀 New features to boost your workflow:
|
|
If the benchmark would (also) compare against a vanilla v8::Function::Call it would show the Node.js mandated (relative) overhead compared to core V8 that is added to all V8 addons that touch Node.js for event delivery. In my own tests, Node.js tend to add 100-200% overhead. |
AsyncResource saves the async context frame when it is created, and MakeCallback() enters it with async_context_frame::Scope. Then node::MakeCallback() opens the callback scope with an undefined frame, so the callback never runs in the saved one. Since AsyncContextFrame is the default, AsyncLocalStorage loses its store in these callbacks. Pass the saved frame to InternalMakeCallback(), as the Node-API AsyncContext already does. This also removes the Scope from every call, with its two Environment lookups and its global handle. Refs: nodejs#66316 Refs: nodejs#43038 Refs: nodejs/performance#24 Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
InternalCallbackScope looks up the Environment from the isolate two times per call, inside async_context_frame::exchange, and it keeps the prior async context frame in a v8::Global also when there is no frame, that is the common case. Every call from native code into JS pays this: MakeCallback, CallbackScope, AsyncWrap, Node-API.
Now the scope passes the Environment it already has, the option is read with an inline accessor instead of copying the shared_ptr, and the global handle is created only when the prior frame is not undefined.
Benchmark: benchmark/napi/make_callback (added here) with benchmark/compare.js, Node 26.3.0 built from source with and without this change, Linux x64, 30 runs per binary. From 202-208 ns to 155-159 ns per call:
Side note: async_context_frame::Scope, used by AsyncResource::MakeCallback, still looks up the Environment twice and always creates a global handle. I would leave it for a separate PR.
Refs: nodejs/performance#24
Disclosure: I used Fable 5.1 (Max) as coding assistant. I built Node with and without the change, ran the benchmark and the tests myself.