Add DFG/FTL intrinsics for DataView BigInt64/BigUint64 accessors#294
Add DFG/FTL intrinsics for DataView BigInt64/BigUint64 accessors#294robobun wants to merge 1 commit into
Conversation
getBigInt64/getBigUint64/setBigInt64/setBigUint64 were plain host calls, making them ~25x slower than the equivalent pair of Uint32 DataView accesses, which are inlined by the JITs. This wires the four methods into the existing DataViewGetInt/DataViewSet DFG nodes as the byteSize == 8 integer case: - set: the value child is speculated as a heap BigInt and unboxed inline (low digit with the sign applied, which is exactly the required modulo-2^64 wrap). The store is fully inline in both DFG and FTL. - get: the 64-bit load and byte swap are inline; the BigInt result is boxed via operationInt64ToBigInt / a new operationUint64ToBigInt in the DFG tier, while the FTL inline-allocates a one-digit heap BigInt (zero stays canonical with length 0) and only calls out on allocator slow paths. The JSBigInt length/digit abstract heaps become Mutable since the FTL fast path now stores to them, and JSBigInt grows an offsetOfHash() accessor so the inline allocation can zero m_hash. On a 1 MiB byte-swap microbenchmark (setBigUint64(o, getBigUint64(o, false), true) over the whole buffer), throughput goes from ~200 MiB/s to ~1.7 GiB/s on x64, and a set-only loop goes from ~420 MiB/s to ~5.2 GiB/s. See oven-sh/bun#34231.
WalkthroughChangesDataView BigInt64 and BigUint64 intrinsics are registered and integrated into DFG and FTL JIT lowering. The implementation adds BigInt conversion and storage paths with endian handling, updates BigInt heap metadata, and introduces extensive stress and correctness coverage. DataView BigInt JIT support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JSTests/stress/dataview-jit-bigint64.js`:
- Around line 160-172: Update the offset-coercion coverage block around oobGet
and oobSet to invoke the BigInt64 DataView accessors with non-plain-integer
offsets, such as a fractional number, numeric string, and object implementing
valueOf, while preserving the detached-buffer assertions. Ensure these cases
exercise ToIndex coercion for both get and set rather than only passing integer
offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc63bfb8-d569-4ec0-82fd-375ac43e7d02
📒 Files selected for processing (13)
JSTests/stress/dataview-jit-bigint64.jsSource/JavaScriptCore/b3/B3AbstractHeapRepository.hSource/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.hSource/JavaScriptCore/dfg/DFGByteCodeParser.cppSource/JavaScriptCore/dfg/DFGDoesGC.cppSource/JavaScriptCore/dfg/DFGFixupPhase.cppSource/JavaScriptCore/dfg/DFGOperations.cppSource/JavaScriptCore/dfg/DFGOperations.hSource/JavaScriptCore/dfg/DFGSpeculativeJIT64.cppSource/JavaScriptCore/ftl/FTLLowerDFGToB3.cppSource/JavaScriptCore/runtime/Intrinsic.hSource/JavaScriptCore/runtime/JSBigInt.hSource/JavaScriptCore/runtime/JSDataViewPrototype.cpp
| // --- number value to get offset coercion & detached buffer --- | ||
| { | ||
| const buf = new ArrayBuffer(16); | ||
| const v = new DataView(buf); | ||
| v.setBigUint64(0, 0x1122334455667788n, true); | ||
| transferArrayBuffer(buf); | ||
| let threw = false; | ||
| try { oobGet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | ||
| assert(threw, "get on detached buffer should throw"); | ||
| threw = false; | ||
| try { oobSet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | ||
| assert(threw, "set on detached buffer should throw"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Comment promises offset-coercion coverage that isn't tested.
The block comment says "number value to get offset coercion & detached buffer" but only exercises the detached-buffer path; offset/byteOffset is always a plain integer everywhere in this file (main loop offsets, OOB offsets, resizable-buffer offsets). No test passes a non-Int32 offset (a float like 16.0, a numeric string, or an object with valueOf) to getBigInt64/setBigUint64, so ToIndex coercion on the offset operand of the new intrinsic is untested. Given this PR adds new DFG/FTL type speculation for the offset operand of DataViewGetInt/DataViewSet, an offset that isn't a plain small integer is exactly the kind of input likely to hit a different type-check/OSR-exit path.
Suggested addition
{
const buf = new ArrayBuffer(16);
const v = new DataView(buf);
v.setBigUint64(0, 0x1122334455667788n, true);
+ // offset coercion via ToIndex
+ assert(v.getBigUint64(0.0, true) === 0x1122334455667788n, "float offset coerces");
+ assert(v.getBigUint64("0", true) === 0x1122334455667788n, "string offset coerces");
+ assert(v.getBigUint64({ valueOf() { return 0; } }, true) === 0x1122334455667788n, "object offset coerces via valueOf");
transferArrayBuffer(buf);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // --- number value to get offset coercion & detached buffer --- | |
| { | |
| const buf = new ArrayBuffer(16); | |
| const v = new DataView(buf); | |
| v.setBigUint64(0, 0x1122334455667788n, true); | |
| transferArrayBuffer(buf); | |
| let threw = false; | |
| try { oobGet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | |
| assert(threw, "get on detached buffer should throw"); | |
| threw = false; | |
| try { oobSet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | |
| assert(threw, "set on detached buffer should throw"); | |
| } | |
| // --- number value to get offset coercion & detached buffer --- | |
| { | |
| const buf = new ArrayBuffer(16); | |
| const v = new DataView(buf); | |
| v.setBigUint64(0, 0x1122334455667788n, true); | |
| // offset coercion via ToIndex | |
| assert(v.getBigUint64(0.0, true) === 0x1122334455667788n, "float offset coerces"); | |
| assert(v.getBigUint64("0", true) === 0x1122334455667788n, "string offset coerces"); | |
| assert(v.getBigUint64({ valueOf() { return 0; } }, true) === 0x1122334455667788n, "object offset coerces via valueOf"); | |
| transferArrayBuffer(buf); | |
| let threw = false; | |
| try { oobGet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | |
| assert(threw, "get on detached buffer should throw"); | |
| threw = false; | |
| try { oobSet(v, 0); } catch (e) { threw = e instanceof TypeError || e instanceof RangeError; } | |
| assert(threw, "set on detached buffer should throw"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@JSTests/stress/dataview-jit-bigint64.js` around lines 160 - 172, Update the
offset-coercion coverage block around oobGet and oobSet to invoke the BigInt64
DataView accessors with non-plain-integer offsets, such as a fractional number,
numeric string, and object implementing valueOf, while preserving the
detached-buffer assertions. Ensure these cases exercise ToIndex coercion for
both get and set rather than only passing integer offsets.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this adds new DFG/FTL codegen paths including inline JSBigInt heap allocation in FTL and flips the JSBigInt_length abstract heap to Mutable — JIT/GC-correctness-critical enough that it warrants a human reviewer familiar with the JIT tiers.
What was reviewed:
- FTL inline BigInt allocation: cell header/sign bit/
m_length/m_hash/digit init,mutatorFence(), and theallocateCellslow-path fallback tooperationInt64ToBigInt/operationUint64ToBigInt. doesGCnow returns true forDataViewGetIntatbyteSize == 8;DataViewSetstays non-GC (set path is a pure store).- Set path uses the existing
toBigInt64/lowHeapBigInthelpers (low digit with sign applied → correct mod-2^64 wrap, zero-length case handled). - Abstract-interpreter/fixup result types for the new 8-byte case, and the
USE(BIGINT32)opt-out for the set intrinsic.
Extended reasoning...
Overview
Adds DFG/FTL intrinsics for DataView.prototype.{get,set}Big{Int,Uint}64 by extending the existing DataViewGetInt/DataViewSet nodes with a byteSize == 8 integer case. Touches 13 files across the intrinsic table, bytecode parser, abstract interpreter, fixup, doesGC, DFG SpeculativeJIT64 codegen, FTL lowering (including a new inline one-digit JSBigInt allocation fast path), the B3 abstract-heap repository, and JSBigInt.h. Includes a comprehensive stress test.
Security risks
JIT codegen and inline GC-cell allocation are security-sensitive. Specific hazards here: (a) manually constructing a JSBigInt cell in FTL — wrong field initialization or missing fence could yield a malformed cell visible to the GC/other code; (b) the JSBigInt_length abstract heap changed from Immutable to Mutable, which globally affects B3 alias analysis for BigInt length loads; (c) doesGC modeling — a wrong false here can corrupt the heap. None of these looked wrong on inspection, but they are exactly the class of change that benefits from a second pair of expert eyes.
Level of scrutiny
High. This is hand-written JIT lowering across two tiers plus GC-interaction metadata, not a mechanical or config change. Bugs in this area tend to be subtle, tier-specific, and can manifest as memory corruption rather than test failures.
Other factors
The PR is well-structured, follows existing patterns (compileMakeRope-style inline allocation, existing toBigInt64/lowHeapBigInt helpers, existing DataView byte-size dispatch), and ships a thorough stress test covering endianness variants, boundary values, multi-digit wrap, OOB/detached/resizable buffers, type-error OSR exits, and a GC loop. The bug-hunting pass found nothing. Still, the scope and criticality put this outside the bar for auto-approval.
Preview Builds
|
1 participant
Problem
getBigInt64/getBigUint64/setBigInt64/setBigUint64onDataViewhave no JIT intrinsics, so every call is a C++ host call that also heap-allocates aJSBigInt. That makes them ~25x slower than the equivalent pair of inlinedUint32DataView accesses. Reported in oven-sh/bun#34231.Change
Wires the four methods into the existing
DataViewGetInt/DataViewSetDFG nodes as thebyteSize == 8integer case:HeapBigIntUse) and unboxed inline: low digit with the sign applied, which is exactly the modulo-2^64 wrap the spec requires, for any digit count. Fully inline in both DFG and FTL.operationInt64ToBigInt/ a newoperationUint64ToBigInt; the FTL inline-allocates a one-digit heap BigInt (zero stays canonical with length 0,m_hashzeroed, sign via the per-cell bit) and only calls out on allocator slow paths, following thecompileMakeRopepattern.JSBigInt_length/ newJSBigInt_data0abstract heaps areMutablesince the FTL fast path now stores to them;JSBigIntgrowsoffsetOfHash().USE(BIGINT32)(heap-BigInt speculation would always exit for small BigInts).Results
1 MiB byte-swap microbenchmark from the bun issue (x64 Linux, release JSC):
setBigUint64(o, getBigUint64(o, false), true)loopTesting
JSTests/stress/dataview-jit-bigint64.js: round-trips interesting values (int64/uint64 boundaries, multi-digit BigInts wrapping mod 2^64, zero canonicalization) across all endianness shapes (constant true/false, omitted, variable), checks OOBRangeError,TypeError/SyntaxErrorcoercion paths after optimization, detached and resizable ArrayBuffers, and a GC stress loop. Passes on the ASAN debug build with--useConcurrentJIT=0,--jitPolicyScale=0 --validateGraph=1, and default options.JSTests/stress/dataview-*.jsandv8-dataview-*.jspass on the ASAN debug build.