src: avoid env lookups and a global handle in InternalCallbackScope by nigrosimone · Pull Request #66316 · nodejs/node

nigrosimone · 2026-09-26T11:54:53Z

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:

                                confidence improvement accuracy (*)   (**)  (***)
napi/make_callback n=1000000           ***     27.55 %      ±5.39% ±7.21%  ±9.45%
napi/make_callback n=10000000          ***     33.68 %      ±2.70% ±3.60%  ±4.70%

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.

github-actions · 2026-09-26T11:55:12Z

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

the guide for first-time contributors
the contribution and automation policies
the pull request guide
the AI use policy
the Code of Conduct

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.

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>

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>

codecov · 2026-09-26T16:40:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.36%. Comparing base (66f26d3) to head (1221198).
⚠️ Report is 38 commits behind head on main.

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

Files with missing lines	Coverage Δ
src/api/callback.cc	`82.93% <100.00%> (-0.32%)`	⬇️
src/async_context_frame.cc	`100.00% <100.00%> (ø)`
src/env-inl.h	`93.99% <100.00%> (+0.02%)`	⬆️
src/env.h	`97.33% <ø> (ø)`

... and 28 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.

uNetworkingAB · 2026-09-26T18:03:24Z

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>

nodejs-github-bot · 2026-09-26T18:28:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/77972/

nigrosimone · 2026-09-26T20:53:42Z

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.

In my own addon (empty function called from a libuv timer, Linux x64, median of alternated runs):

	v8::Function::Call	node::MakeCallback	Node overhead
Node 22.6 (before AsyncContextFrame)	51 ns	131 ns	+160%
Node 26.3.0	50 ns	199-213 ns	+300%
Node 26.3.0 + this PR	50 ns	150-155 ns	+200%

With an empty callback the relative overhead is even higher than 100-200%. I will add a v8::Function::Call case to the benchmark in #66326, where I'm already extending it, so this PR keeps its approvals and CI.

uNetworkingAB · 2026-09-26T22:19:01Z

Node.js having a 3x cost with node::MakeCallback compared to what V8 gives you is exactly the kind of red flag you find when you ask questions. Bun and Deno do not have this kind of hot path performance sink.

nodejs-github-bot · 2026-09-26T22:26:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/77982/

nodejs-github-bot · 2026-09-28T11:59:55Z

Commit Queue failed

This pull request has multiple commits, but no landing policy was selected.

Add commit-queue-squash PRs the Commit Queue should land as one squashed commit. to land it as one commit, or commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. to land the commits separately.

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output

�[36m⠋�[39m Loading data for nodejs/node/pull/66316
�[36m⠋�[39m Loading data for nodejs/node/pull/66316
�[36m⠋�[39m Getting collaborator contacts from README of nodejs/node
�[36m⠋�[39m Getting PR from nodejs/node/pull/66316
�[36m⠋�[39m Getting reviews from nodejs/node/pull/66316
�[36m⠋�[39m Getting comments from nodejs/node/pull/66316
�[36m⠋�[39m Getting commits from nodejs/node/pull/66316
✔  Done loading data for nodejs/node/pull/66316
----------------------------------- PR info ------------------------------------
Title      src: avoid env lookups and a global handle in InternalCallbackScope (#66316)
Author     Nigro Simone <nigro.simone@gmail.com> (@nigrosimone, first-time contributor)
Branch     nigrosimone:callback-scope-cost -> nodejs:main
Labels     c++, lib / src, author ready, needs-ci, commit-queue
Commits    3
 - src: reduce InternalCallbackScope overhead
 - benchmark: add a node::MakeCallback benchmark
 - test: check frame restore in CallbackScope
Committers 1
 - Nigro Simone <nigro.simone@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/66316
Refs: https://github.com/nodejs/performance/issues/24
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/66316
Refs: https://github.com/nodejs/performance/issues/24
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 26 Sep 2026 11:54:53 GMT
   ✔  Approvals: 3
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/66316#pullrequestreview-5326168568
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/66316#pullrequestreview-5326655376
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/66316#pullrequestreview-5328346468
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-09-26T22:26:19Z: https://ci.nodejs.org/job/node-test-pull-request/77982/
�[36m⠙�[39m Querying data for job/node-test-pull-request/77982/
�[36m⠙�[39m Querying data for job/node-test-pull-request/77982/
�[36m⠙�[39m Querying API for job/node-test-pull-request/77982/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
�[36m⠹�[39m Querying API for job/node-test-pull-request/77982/
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
�[36m⠹�[39m Querying API for job/node-test-pull-request/77982/
�[36m⠹�[39m Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
�[36m⠸�[39m Downloading patch for 66316
�[36m⠸�[39m Downloading patch for 66316
From https://github.com/nodejs/node
 * branch                  refs/pull/66316/merge -> FETCH_HEAD
✔  Fetched commits as b59840b59306..12211986ec24
--------------------------------------------------------------------------------
[main 3fdbc95f0e] src: reduce InternalCallbackScope overhead
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 5 files changed, 28 insertions(+), 10 deletions(-)
[main 997a05e65d] benchmark: add a node::MakeCallback benchmark
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 4 files changed, 90 insertions(+)
 create mode 100644 benchmark/napi/make_callback/.gitignore
 create mode 100644 benchmark/napi/make_callback/binding.cc
 create mode 100644 benchmark/napi/make_callback/binding.gyp
 create mode 100644 benchmark/napi/make_callback/index.js
[main a92808b9f1] test: check frame restore in CallbackScope
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 1 file changed, 23 insertions(+)
 create mode 100644 test/addons/callback-scope/test-async-local-storage.js
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
(node:385) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/6)
Executing: git node land --amend --yes
   ⚠  Found Refs: https://github.com/nodejs/performance/issues/24, skipping..
--------------------------------- New Message ----------------------------------
src: reduce InternalCallbackScope overhead

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: https://github.com/nodejs/performance/issues/24
Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/66316
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD 18017b616e] src: reduce InternalCallbackScope overhead
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 5 files changed, 28 insertions(+), 10 deletions(-)
Rebasing (3/6)
Rebasing (4/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: add a node::MakeCallback benchmark

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>
PR-URL: https://github.com/nodejs/node/pull/66316
Refs: https://github.com/nodejs/performance/issues/24
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD 845673a44c] benchmark: add a node::MakeCallback benchmark
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 4 files changed, 90 insertions(+)
 create mode 100644 benchmark/napi/make_callback/.gitignore
 create mode 100644 benchmark/napi/make_callback/binding.cc
 create mode 100644 benchmark/napi/make_callback/binding.gyp
 create mode 100644 benchmark/napi/make_callback/index.js
Rebasing (5/6)
Rebasing (6/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: check frame restore in CallbackScope

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>
PR-URL: https://github.com/nodejs/node/pull/66316
Refs: https://github.com/nodejs/performance/issues/24
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD 03c0a5ebf6] test: check frame restore in CallbackScope
 Author: Nigro Simone <nigro.simone@gmail.com>
 Date: Sat Sep 26 14:59:55 2026 +0200
 1 file changed, 23 insertions(+)
 create mode 100644 test/addons/callback-scope/test-async-local-storage.js
Successfully rebased and updated refs/heads/main.
--------------------------------------------------------------------------------
   ℹ  Add `commit-queue-squash` label to land the PR as one commit, or `commit-queue-rebase` to land as separate commits.

View workflow run

nigrosimone · 2026-09-28T16:13:51Z

Commit Queue failed

This pull request has multiple commits, but no landing policy was selected.

Add commit-queue-squash PRs the Commit Queue should land as one squashed commit. to land it as one commit, or commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. to land the commits separately.

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output
View workflow run

The three commits are self-contained (src / benchmark / test), so commit-queue-rebase should work. Happy to squash if you prefer.

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 Sep 26, 2026

nigrosimone force-pushed the callback-scope-cost branch 2 times, most recently from b4d12f6 to e83b5b2 Compare September 26, 2026 12:27

nigrosimone force-pushed the callback-scope-cost branch from e83b5b2 to e9c75be Compare September 26, 2026 13:13

nigrosimone marked this pull request as ready for review September 26, 2026 13:17

nigrosimone mentioned this pull request Sep 26, 2026

MakeCallback is very slow nodejs/performance#24

Open

ronag requested review from anonrig, jasnell and joyeecheung September 26, 2026 13:56

anonrig approved these changes Sep 26, 2026

View reviewed changes

mcollina reviewed Sep 26, 2026

View reviewed changes

Comment thread benchmark/napi/make_callback/binding.gyp

mcollina 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 Sep 26, 2026

mcollina requested a review from Qard September 26, 2026 14:37

nigrosimone added 2 commits September 26, 2026 16:42

benchmark: add a node::MakeCallback benchmark

cad15a1

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>

test: check frame restore in CallbackScope

1221198

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>

nigrosimone force-pushed the callback-scope-cost branch from e9c75be to 1221198 Compare September 26, 2026 14:42

Qard approved these changes Sep 26, 2026

View reviewed changes

nigrosimone mentioned this pull request Sep 26, 2026

src: keep ALS store in AsyncResource::MakeCallback #66326

Draft

panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 26, 2026

This comment was marked as outdated.

Sign in to view

jasnell requested a review from addaleax September 27, 2026 00:50

jasnell reviewed Sep 27, 2026

View reviewed changes

Comment thread benchmark/napi/make_callback/binding.cc

jasnell reviewed Sep 27, 2026

View reviewed changes

Comment thread benchmark/napi/make_callback/index.js

jasnell approved these changes Sep 27, 2026

View reviewed changes

panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 27, 2026

nigrosimone mentioned this pull request Sep 27, 2026

src: do not shrink async resources on every pop #66344

Draft

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 28, 2026

Navigation Menu

Uh oh!

src: avoid env lookups and a global handle in InternalCallbackScope - #66316

src: avoid env lookups and a global handle in InternalCallbackScope#66316
nigrosimone wants to merge 3 commits into
nodejs:mainfrom
nigrosimone:callback-scope-cost

nigrosimone commented Sep 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 26, 2026

Uh oh!

Uh oh!

codecov Bot commented Sep 26, 2026 •

edited

Loading

Uh oh!

uNetworkingAB commented Sep 26, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Sep 26, 2026

Uh oh!

This comment was marked as outdated.

nigrosimone commented Sep 26, 2026

Uh oh!

uNetworkingAB commented Sep 26, 2026

Uh oh!

nodejs-github-bot commented Sep 26, 2026

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 28, 2026

Uh oh!

nigrosimone commented Sep 28, 2026

Commit Queue failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

nigrosimone commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 26, 2026

Uh oh!

Uh oh!

codecov Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

uNetworkingAB commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 26, 2026

Uh oh!

This comment was marked as outdated.

nigrosimone commented Sep 26, 2026

Uh oh!

uNetworkingAB commented Sep 26, 2026

Uh oh!

nodejs-github-bot commented Sep 26, 2026

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 28, 2026

Commit Queue failed

Uh oh!

nigrosimone commented Sep 28, 2026

Commit Queue failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nigrosimone commented Sep 26, 2026 •

edited

Loading

codecov Bot commented Sep 26, 2026 •

edited

Loading

uNetworkingAB commented Sep 26, 2026 •

edited

Loading