Skip to content

benchmark,tools: fix napi benchmarks on GHA - #66423

Open
nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:benchmark-gha-addons
Open

nigrosimone wants to merge 1 commit into
nodejs:mainfrom
nigrosimone:benchmark-gha-addons

Conversation

@nigrosimone

Copy link
Copy Markdown
Contributor

The GHA benchmark cannot run any napi/* benchmark: every run prints "Binding failed to load" (for example on #66395). Two reasons:

  • the workflow builds Node, but never the benchmark addons
  • the build uses --debug-node (the default of shell.nix), so process.features.debug is true and benchmark/common.js looks for the addon in build/Debug, while node-gyp builds it in build/Release

Now the workflow runs make bench-addons-build when the category has napi, and benchmark/common.js takes the build type from process.config, as test/common does.

Tested on my fork, napi/make_callback with the same code on both sides (x86_64-linux only): https://github.com/nigrosimone/node/actions/runs/36763821317

                              confidence improvement accuracy (*)   (**)  (***)
napi/make_callback n=1000000                  1.51 %       ±1.66% ±2.49% ±3.95%
napi/make_callback n=10000000          *      1.53 %       ±1.25% ±1.83% ±2.75%

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Sep 30, 2026
@nigrosimone
nigrosimone marked this pull request as ready for review October 1, 2026 02:00
@panva

panva commented Oct 1, 2026

Copy link
Copy Markdown
Member

This conflicts with #66351 (already in commit-queue PRs queued for automated landing through the Commit Queue. )

@nigrosimone

This comment was marked as resolved.

@nigrosimone
nigrosimone force-pushed the benchmark-gha-addons branch from d1e747a to e0dad8b Compare October 3, 2026 14:28
@nigrosimone

Copy link
Copy Markdown
Contributor Author

@panva rebased on #66351

Comment thread .github/workflows/benchmark.yml Outdated
The GHA benchmark never built the addons of benchmark/napi, and its
--debug-node build made benchmark/common.js look for them in
build/Debug. Build them when the napi category runs, and pick the
build type as test/common does.

Refs: nodejs#66395
Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
@nigrosimone
nigrosimone force-pushed the benchmark-gha-addons branch from e0dad8b to 235ee9b Compare October 4, 2026 06:20
@panva panva closed this Oct 10, 2026
@panva panva reopened this Oct 10, 2026
--arg ccache '(import <nixpkgs> {}).sccache' \
--run '
make build-ci -j4 V=1
make build-ci -j4 V=1 ${{ contains(format(' {0} ', inputs.category), ' napi ') && '&& make bench-addons-build' || '' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need two make calls, can't we merge them?

Suggested change
make build-ci -j4 V=1 ${{ contains(format(' {0} ', inputs.category), ' napi ') && '&& make bench-addons-build' || '' }}
make build-ci ${{ contains(format(' {0} ', inputs.category), ' napi ') && 'bench-addons-build' || '' }} -j4 V=1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can't be merged safely: with -j4 make builds both goals concurrently, and nothing orders bench-addons-build after build-ci.
On a clean checkout $(NODE_EXE) can hit config.gypi before configure runs, or race with build-ci's inner make on out/Release. && guarantees the full build finishes first.

@codecov

codecov Bot commented Oct 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (9067cc4) to head (235ee9b).
⚠️ Report is 146 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66423      +/-   ##
==========================================
+ Coverage   90.42%   93.23%   +2.80%     
==========================================
  Files         790      422     -368     
  Lines      275435   193715   -81720     
  Branches    52825    32844   -19981     
==========================================
- Hits       249074   180611   -68463     
+ Misses      16772    12796    -3976     
+ Partials     9589      308    -9281     

see 513 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

meta Issues and PRs related to the general management of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants