Skip to content

src: cut small costs in node::MakeCallback - #66395

Open
nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:callback-context-scope
Open

nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:callback-context-scope

Conversation

@nigrosimone

Copy link
Copy Markdown
Contributor

After #66316, node::MakeCallback still does some work that the common case does not need:

  • it enters the Environment's context also when it is already the entered and current one
  • it sets the async context frame also when it does not change (usually undefined before and after)
  • push_async_context() takes the resource variant by value
  • the native resource stack uses resize() where push_back() and pop_back() are enough

Each one is 1-5 ns, together about 10-14 ns per call. type=Call does not use this code. benchmark/compare.js, 30 runs, Linux x64:

                                                    confidence improvement accuracy (*)   (**)  (***)
napi/make_callback n=1000000 type='AsyncResource'           ***      8.76 %  ±3.98% ±5.30% ±6.90%
napi/make_callback n=1000000 type='Call'                            -3.08 %  ±5.30% ±7.06% ±9.19%
napi/make_callback n=1000000 type='MakeCallback'             **      7.92 %  ±5.71% ±7.60% ±9.90%
napi/make_callback n=10000000 type='AsyncResource'            *      5.20 %  ±4.02% ±5.35% ±6.96%
napi/make_callback n=10000000 type='Call'                           -0.04 %  ±5.44% ±7.24% ±9.42%
napi/make_callback n=10000000 type='MakeCallback'           ***     11.15 %  ±3.05% ±4.06% ±5.28%

Refs: nodejs/performance#24

Disclosure: I used Opus 5.5 (Max) as coding assistant

Enter the Environment's context only when it is not already the
entered and current one, set the async context frame only when it
changes, pass the resource variant to push_async_context() by const
reference, and grow and shrink the native resource stack with
push_back() and pop_back() instead of resize().

Refs: nodejs/performance#24
Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
@nodejs-github-bot nodejs-github-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 Sep 29, 2026
@nigrosimone
nigrosimone marked this pull request as ready for review September 29, 2026 14:08
@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.37%. Comparing base (1f26576) to head (4667f95).

Files with missing lines Patch % Lines
src/api/callback.cc 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66395      +/-   ##
==========================================
+ Coverage   90.35%   90.37%   +0.02%     
==========================================
  Files         792      792              
  Lines      275500   275515      +15     
  Branches    52791    52808      +17     
==========================================
+ Hits       248932   249003      +71     
+ Misses      16992    16931      -61     
- Partials     9576     9581       +5     
Files with missing lines Coverage Δ
src/env.cc 82.24% <100.00%> (+0.01%) ⬆️
src/env.h 97.33% <ø> (ø)
src/api/callback.cc 82.64% <70.00%> (-0.77%) ⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants