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 29 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.

@trivikr

trivikr commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Full ffi benchmarks (separate run)

                                                    confidence improvement accuracy (*)   (**)  (***)
ffi/add-64.js n=10000000                                           -0.16 %       ±0.55% ±0.73% ±0.95%
ffi/add-f32.js n=10000000                                          -0.23 %       ±1.33% ±1.77% ±2.31%
ffi/add-f64.js n=10000000                                          -0.60 %       ±4.27% ±5.68% ±7.41%
ffi/add-i16.js n=10000000                                           0.67 %       ±0.98% ±1.30% ±1.69%
ffi/add-i32.js n=10000000                                           0.57 %       ±5.02% ±6.68% ±8.71%
ffi/add-i64.js n=10000000                                           0.41 %       ±2.46% ±3.28% ±4.27%
ffi/add-i8.js n=10000000                                            0.19 %       ±1.52% ±2.03% ±2.66%
ffi/add-u16.js n=10000000                                          -0.14 %       ±4.44% ±5.92% ±7.71%
ffi/add-u64.js n=10000000                                          -1.19 %       ±2.27% ±3.04% ±4.01%
ffi/add-u8.js n=10000000                                           -0.54 %       ±1.31% ±1.75% ±2.29%
ffi/buffer-first-byte-direct.js n=10000000                         -0.00 %       ±0.25% ±0.33% ±0.43%
ffi/buffer-first-byte.js n=10000000                                 0.46 %       ±1.59% ±2.11% ±2.75%
ffi/buffer-sum-direct.js n=10000000                                 1.45 %       ±2.17% ±2.91% ±3.82%
ffi/buffer-sum.js n=10000000                                        0.67 %       ±2.53% ±3.38% ±4.42%
ffi/getpid.js n=10000000                                           -2.12 %       ±3.19% ±4.30% ±5.70%
ffi/identity-i32.js n=10000000                                      0.33 %       ±0.70% ±0.93% ±1.21%
ffi/many-args.js n=10000000                                        -0.04 %       ±1.01% ±1.34% ±1.75%
ffi/noop-void.js n=10000000                                        -0.97 %       ±4.20% ±5.62% ±7.40%
ffi/pointer-bigint.js n=10000000                                    1.13 %       ±2.46% ±3.31% ±4.38%
ffi/pointer-buffer-direct.js n=10000000                            -0.08 %       ±0.66% ±0.88% ±1.14%
ffi/pointer-buffer.js n=10000000                                    0.32 %       ±1.20% ±1.62% ±2.13%
ffi/pointer-null.js n=10000000                                      0.34 %       ±1.24% ±1.65% ±2.15%
ffi/string-equals-hello-buffer-direct.js n=10000000                -0.23 %       ±0.34% ±0.45% ±0.59%
ffi/string-equals-hello-buffer.js n=10000000                       -0.78 %       ±2.22% ±2.96% ±3.86%
ffi/string-first-char-buffer-direct.js n=10000000                  -0.41 %       ±0.88% ±1.17% ±1.54%
ffi/string-first-char-buffer.js n=10000000                         -1.13 %       ±1.18% ±1.57% ±2.04%
ffi/string-length-buffer-direct.js n=10000000                      -1.36 %       ±2.46% ±3.31% ±4.37%
ffi/string-length-buffer.js n=10000000                              0.00 %       ±0.35% ±0.46% ±0.60%
ffi/string-length-string-direct.js n=10000000                       0.04 %       ±0.11% ±0.15% ±0.20%
ffi/string-length-string.js n=10000000                             -1.15 %       ±1.92% ±2.55% ±3.33%
ffi/sum-3-i32.js n=10000000                                         0.53 %       ±0.81% ±1.07% ±1.40%
ffi/sum-5-i32.js n=10000000                                         0.13 %       ±0.83% ±1.10% ±1.44%
ffi/sum-8-i32.js n=10000000                                         0.24 %       ±0.35% ±0.46% ±0.60%
ffi/sum-buffer.js n=1000000 size=1024                              -1.05 %       ±2.21% ±2.96% ±3.89%
ffi/sum-buffer.js n=1000000 size=16384                             -0.13 %       ±1.25% ±1.66% ±2.17%
ffi/sum-buffer.js n=1000000 size=64                                 1.87 %       ±5.15% ±6.92% ±9.15%

@trivikr
trivikr requested a review from ShogunPanda July 18, 2026 07:15
@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

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