Skip to content

node-api: find the Environment once - #66509

Open
nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:napi-make-callback-env
Open

nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:napi-make-callback-env

Conversation

@nigrosimone

@nigrosimone nigrosimone commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Without an async context, napi_make_callback goes through the public node::MakeCallback(), which looks up the Environment from the creation context of the callback on every call. The napi_env already knows it, so call into it directly, as the path with an async context does. node_napi_env__::node_env() also looked it up from the context on every call (also in can_call_into_js()): now it is found once, a napi_env lives as long as its Environment.

There was no benchmark for napi_make_callback, so this adds napi/make_callback_napi, a C addon calling it from a libuv timer like an I/O callback.

Alone on 26.3.0 + #66316 the gain is inside the noise (-3.0% and +2.1%, no stars). With #66395 and the V8 checkpoint fast path (#66500) underneath, both now on main, it is +15% (200 to 174 ns), compare.js, 30 runs, Linux x64, one core:

                                     confidence improvement accuracy (*)   (**)  (***)
napi/make_callback_napi n=1000000           ***     15.12 %       ±4.73% ±6.29% ±8.19%
napi/make_callback_napi n=10000000          ***     14.78 %       ±3.82% ±5.10% ±6.68%

The Node-API callback tests and the make-callback addon tests pass. Part of the plan in nodejs/performance#24.

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Oct 4, 2026
@legendecas legendecas added the node-api Issues and PRs related to Node-API. label Oct 9, 2026
@nigrosimone
nigrosimone force-pushed the napi-make-callback-env branch from b729030 to 1a639d6 Compare October 21, 2026 00:58
@ronag

ronag commented Nov 9, 2026

Copy link
Copy Markdown
Member

Why still draft?

@nigrosimone
nigrosimone marked this pull request as ready for review November 9, 2026 15:09
@nigrosimone

Copy link
Copy Markdown
Contributor Author

No reason now. The gain needed #66395 and the V8 checkpoint fast path underneath, both are on main now. Marked ready.

@nigrosimone
nigrosimone force-pushed the napi-make-callback-env branch from 1a639d6 to 64bedaa Compare November 9, 2026 15:26
@mcollina

mcollina commented Nov 9, 2026

Copy link
Copy Markdown
Member

CI is red

@nigrosimone
nigrosimone force-pushed the napi-make-callback-env branch 2 times, most recently from 5bcd171 to f911f82 Compare November 9, 2026 16:39
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 9, 2026
Without an async context, napi_make_callback went through the public
node::MakeCallback(), which looks up the Environment from the creation
context of the callback on every call. The napi_env already knows it,
so call into it directly, as the path with an async context does.
node_napi_env__::node_env() also looked it up from the context on every
call: find it once, a napi_env lives as long as its Environment.

Add a benchmark, napi/make_callback_napi.

Refs: nodejs/performance#24
Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
@nigrosimone
nigrosimone force-pushed the napi-make-callback-env branch from f911f82 to 4000c25 Compare November 9, 2026 17:22
@nigrosimone

Copy link
Copy Markdown
Contributor Author

@mcollina CI is green now.

@codecov

codecov Bot commented Nov 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (3e5c8af) to head (4000c25).
⚠️ Report is 261 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66509      +/-   ##
==========================================
- Coverage   92.86%   90.71%   -2.15%     
==========================================
  Files         428      798     +370     
  Lines      195100   280024   +84924     
  Branches    30147    53866   +23719     
==========================================
+ Hits       181175   254025   +72850     
- Misses      13597    16287    +2690     
- Partials      328     9712    +9384     
Files with missing lines Coverage Δ
src/api/callback.cc 80.54% <100.00%> (ø)
src/node_api_internals.h 100.00% <100.00%> (ø)
src/node_internals.h 80.35% <ø> (ø)
src/node_api.cc 77.18% <83.33%> (ø)

... and 545 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++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review.

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

5 participants