ffi: preserve uint8 semantics for bool fast calls#64527
Open
trivikr wants to merge 1 commit into
Open
Conversation
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
Collaborator
|
Review requested:
|
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
Contributor
|
Can you please run comparison benchmarks to make sure nothing regresses? |
Member
Author
Did you mean benchmark-node-micro-benchmarks? |
Contributor
|
No, I meant something like: And then
Note that you need to compile Node twice, once before and once after your changes. |
Member
Author
|
Benchmark output for confidence improvement accuracy (*) (**) (***)
ffi/add-u8.js n=10000000 0.75 % ±1.47% ±1.95% ±2.54% |
Contributor
|
Do you mind posting the entire benchmark? I just want to make sure there are not regressions elsewhere. |
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% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #64526
node:ffidocumentsboolas an 8-bit unsigned integer, but the FastAPI path mapped it to V8's Boolean type. As a result, behavior changed
after JIT optimization:
trueandfalsebecame accepted as arguments.This change normalizes
boolto the existingkUint8Fast FFI type andremoves the now-unused
kBooltype. The regression test forces V8optimization and verifies that numeric return values and argument
validation remain consistent.
Assisted-by: openai:gpt-5.6-sol