ffi: accept safe integer numbers for 64-bit arguments - #66197
HoonDongKang wants to merge 3 commits into
Conversation
|
Review requested:
|
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66197 +/- ##
=======================================
Coverage 90.36% 90.37%
=======================================
Files 792 792
Lines 275398 275461 +63
Branches 52776 52802 +26
=======================================
+ Hits 248877 248955 +78
- Misses 16937 16938 +1
+ Partials 9584 9568 -16
π New features to boost your workflow:
|
This comment was marked as spam.
This comment was marked as spam.
ac792ef to
b25043f
Compare
Allow safe integer numbers as int64 and uint64 arguments alongside bigint values. Reject negative numbers for uint64 and numbers outside the safe integer range. Keep 64-bit return values as bigint. Apply validation and conversion across the Fast API, shared-buffer, and generic argument conversion paths. Add coverage for Number and BigInt boundaries, invalid inputs, and single-argument calls before and after optimization. Signed-off-by: HoonDongKang <d159123@naver.com> Assisted-by: Codex:Astra-medium
Signed-off-by: HoonDongKang <d159123@naver.com> Assisted-by: Codex:Astra-medium
b25043f to
bf23996
Compare
|
Do you mind running benchmarks? |
Validate and convert integer arguments in a single helper to avoid repeated type metadata lookups and conversion checks. Handle safe integer Number inputs for 64-bit arguments separately, while preserving the existing type and range checks for BigInt inputs. Return other argument types unchanged for subsequent pointer conversion. Signed-off-by: HoonDongKang <d159123@naver.com> Assisted-by: Codex:Astra-medium
28b2721 to
b0a3f6d
Compare
|
I updated In the previous revision of this PR, validation and conversion were separate steps, each looking up the argumentβs type metadata. The combined helper performs that lookup once, validates and converts safe integer "number" inputs to "bigint", and preserves the existing type and range checks for "bigint" inputs. I measured two aspects of performance, with 30 runs per configuration: 1. Existing BigInt inputs: baseline vs. updated implementation confidence improvement accuracy (*) (**) (***)
ffi/add-i64.js n=10000000 *** +29.99 % Β±4.82% Β±6.41% Β±8.36%
ffi/add-u64.js n=10000000 *** +35.96 % Β±5.13% Β±6.83% Β±8.89%
-41.1% 0% +41.1%
ffi/add-i64.js n=10000000 |ββββββββββββββββ +29.99% ***
ffi/add-u64.js n=10000000 |βββββββββββββββββββ +35.96% ***2. BigInt vs. Number inputs in the updated implementation I added an
For the first comparison, both versions perform the same type and range checks for BigInt inputs, and the Number conversion branch is not taken. The improvement may therefore reflect differences in V8βs generated code rather than less validation work. The second comparison shows the cost of accepting Number inputs: throughput was approximately 30β33% lower than with pre-created BigInt inputs in these benchmarks. This measures the additional cost of internal validation and conversion when callers use the convenience provided by this PR. In absolute terms, this corresponds to approximately 15β17 ns of additional time per call in these benchmarks, which pass two 64-bit integer arguments. Do you think this overhead is acceptable given the convenience of passing Number values directly? == With the confidence improvement accuracy (*) (**) (***)
ffi/add-i64.js n=10000000 +0.49 % Β±1.57% Β±2.08% Β±2.71%
ffi/add-u64.js n=10000000 -0.10 % Β±1.59% Β±2.12% Β±2.76%
-2.1% 0% +2.1%
ffi/add-i64.js n=10000000 ββββββββββ|βββββββββββββββββββ +0.49%
ffi/add-u64.js n=10000000 ββββββββββββββββ|ββββββββββββββ -0.10% With TurboFan inlining disabled, both performance differences were below 1%. This suggests that the previously observed BigInt improvement is related to TurboFan inlining and its effects on the generated code, rather than reduced validation work. |
Summary
Accept safe integer numbers for int64/uint64 arguments alongside bigint values, allowing buffer lengths to be passed without explicit BigInt() conversions.
Changes
Tests
The tests check that number and bigint arguments produce the same results, including when used together. A new single-argument test covers Fast API conversion before and after requesting V8 optimization.
Separate nine-argument tests exercise SharedBuffer conversion and generic fallback. Both check Number and BigInt boundaries, invalid inputs, and error messages.
Validation
54 tests passed across the following files. Local lint and whitespace checks also passed.
Refs: #66198
Assisted-by: Codex:Astra-medium