Skip to content

ffi: preserve uint8 semantics for bool fast calls#64527

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-bool-fast-api
Open

ffi: preserve uint8 semantics for bool fast calls#64527
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-bool-fast-api

Conversation

@trivikr

@trivikr trivikr commented Jul 16, 2026

Copy link
Copy Markdown
Member

Fixes: #64526

node:ffi documents bool as an 8-bit unsigned integer, but the Fast
API path mapped it to V8's Boolean type. As a result, behavior changed
after JIT optimization:

  • return values changed from numbers to JavaScript Booleans;
  • true and false became accepted as arguments.

This change normalizes bool to the existing kUint8 Fast FFI type and
removes the now-unused kBool type. The regression test forces V8
optimization and verifies that numeric return values and argument
validation remain consistent.


Assisted-by: openai:gpt-5.6-sol

Normalize bool to kUint8 when creating Fast API metadata. This keeps
optimized calls consistent with generic FFI behavior, including numeric
return values and rejection of JavaScript Boolean values.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: openai:gpt-5.6-sol
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi
  • @nodejs/tsc

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 16, 2026
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2026
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.25%. Comparing base (f2ba101) to head (ae0ca8d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ffi/fast.cc 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64527      +/-   ##
==========================================
- Coverage   92.01%   90.25%   -1.77%     
==========================================
  Files         379      739     +360     
  Lines      170129   241649   +71520     
  Branches    26090    45542   +19452     
==========================================
+ Hits       156544   218098   +61554     
- Misses      13293    15093    +1800     
- Partials      292     8458    +8166     
Files with missing lines Coverage Δ
src/ffi/fast.h 100.00% <ø> (ø)
src/ffi/platforms/arm64.cc 86.90% <ø> (ø)
src/ffi/fast.cc 66.66% <0.00%> (ø)

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

@ShogunPanda

Copy link
Copy Markdown
Contributor

Can you please run comparison benchmarks to make sure nothing regresses?

@trivikr

trivikr commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Can you please run comparison benchmarks to make sure nothing regresses?

Did you mean benchmark-node-micro-benchmarks?

@ShogunPanda

Copy link
Copy Markdown
Contributor

No, I meant something like:

./node-new --no-warnings --experimental-ffi benchmark/compare.js --old ./node-old --new ./node-new --runs 30 --no-progress ffi > compare.csv

And then

npx node-benchmark-compare compare.csv

Note that you need to compile Node twice, once before and once after your changes.

@trivikr

trivikr commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Benchmark output for add-u8 which this PR touches

                         confidence improvement accuracy (*)   (**)  (***)
ffi/add-u8.js n=10000000                 0.75 %       ±1.47% ±1.95% ±2.54%

@ShogunPanda

Copy link
Copy Markdown
Contributor

Do you mind posting the entire benchmark? I just want to make sure there are not regressions elsewhere.

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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi: bool changes from uint8 to Boolean after Fast API optimization

3 participants