Add JIT intrinsics for DataView BigInt64/BigUint64 accessors (WebKit upgrade) - #34238
Add JIT intrinsics for DataView BigInt64/BigUint64 accessors (WebKit upgrade)#34238robobun wants to merge 2 commits into
Conversation
getBigInt64/getBigUint64/setBigInt64/setBigUint64 on DataView were plain host calls in JSC, roughly 25x slower than the equivalent pair of inlined Uint32 DataView accesses. oven-sh/WebKit#294 adds DFG/FTL intrinsics for them; this bumps WEBKIT_VERSION to pick that up and adds a JIT round-trip test. Fixes #34231
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 11:01 AM PT - Jul 15th, 2026
❌ @robobun, your commit 57a07a0 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34238That installs a local version of the PR into your bun-34238 --bun |
Decouple endianness from the value-index parity, add byte-level and cross-endianness known-answer checks inside the hot loop, route the error-path and GC-stress assertions through the tiered helpers so the compiled intrinsic is what gets tested, and force a final GC before checking the retained values.
There was a problem hiding this comment.
Thanks — the test now addresses all three earlier nits (endianness decoupled from value index, byte-layout + cross-endian known-answer checks inside the hot loop via the tiered helpers, error paths routed through roundtripUnsigned/getUnsigned/roundtripSigned, and the extra Bun.gc(true) before the final value check). No further findings on the Bun side.
Not auto-approving because the substantive change is a JSC DFG/FTL codegen change living in oven-sh/WebKit#294 (not reviewable from this diff), and WEBKIT_VERSION is still pinned to the preview build pending that PR's merge — a human should coordinate the final version bump and sign off on the engine change.
Extended reasoning...
Overview
Two files: a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts (currently to autobuild-preview-pr-294-92ed0b7e) and a new JIT round-trip test at test/js/bun/jsc/dataview-bigint64-jit.test.ts. The actual behavioral change — DFG/FTL intrinsics for DataView BigInt64/BigUint64 accessors, including FTL inline allocation of one-digit JSBigInts — is implemented in oven-sh/WebKit#294 and only pulled in here via the version pin.
Since the previous review pass, the test was strengthened (commit 57a07a0) to address all three nits I raised: le = ((i >> 1) & 1) === 0 decouples endianness from the value index; the byte-layout known-answer check and a cross-endianness read now run inside the 50k-iteration hot loop through the already-tiered roundtripUnsigned/getUnsigned helpers; the TypeError/RangeError checks and post-exit sanity check likewise go through the hot helpers; and a final Bun.gc(true) was added so the retained BigInts are value-checked after surviving a full collection. No new issues found in the revised test.
Security risks
None on the Bun side — a version-constant edit and a test file. The WebKit-side change adds JIT lowerings that perform bounds-checked 8-byte loads/stores and heap-BigInt unboxing; a bug there could be memory-safety-relevant, but that code is not in this diff.
Level of scrutiny
High. This is an engine upgrade that changes optimizing-JIT codegen for a Web/Node-standard API, and per the PR's own IMPORTANT note it is not yet mergeable as-is: WEBKIT_VERSION points at a preview artifact and must be re-bumped to the merged autobuild sha once oven-sh/WebKit#294 lands. That coordination step, plus review of the DFG/FTL implementation itself, is squarely a human call.
Other factors
The bug-hunting system found nothing this run. The WebKit PR carries its own JSTests/stress/dataview-jit-bigint64.js covering OOB/detached/resizable buffers under the JIT, and the Bun-side test now provides a reasonable in-tree guard. Given the prior nits are resolved and the remaining blocker is process (WebKit merge + final version pin) rather than a code defect here, deferring with acknowledgement rather than staying silent.
|
Closing: these intrinsics landed upstream as WebKit 317392@main (oven-sh/WebKit commit e164bce2e888, "[JSC] Handle DataView BigInt64 accessors in DFG / FTL") and have been in the WebKit build main pins since #34373, which is why #34231 was closed. This PR's pin still points at the preview build of oven-sh/WebKit#294, a separate implementation of the same change that is now superseded. The upstream commit's stress tests are being ported into test/js/bun/jsc-stress in #38117, which covers everything the test here did. |
What does this PR do?
Fixes #34231.
DataView.prototype.getBigInt64/getBigUint64/setBigInt64/setBigUint64had no JIT intrinsics in JavaScriptCore: every call was a C++ host call that also heap-allocates aJSBigInt. That made them ~27x slower than the equivalent pair ofUint32DataView accesses, which are inlined by the DFG/FTL.oven-sh/WebKit#294 adds DFG/FTL intrinsics for the four methods, extending the existing
DataViewGetInt/DataViewSetnodes to the byteSize == 8 integer case:This PR bumps
WEBKIT_VERSIONto pick that up and adds a JIT round-trip test.Important
WEBKIT_VERSIONcurrently points at the preview buildautobuild-preview-pr-294-92ed0b7e. Before merging this PR, oven-sh/WebKit#294 must be merged andWEBKIT_VERSIONbumped to the resultingautobuild-<merge sha>release.Benchmark
Reproduction from the issue (1 MiB buffer byte-swap, linux x64, release build against the new WebKit):
view64(setBigUint64(o, getBigUint64(o, false), true))optimizedView64(two Uint32 ops)The remaining gap to V8 on the combined loop is the per-
BigIntallocation + GC cost on the get side, which is structural in JSC.Testing
test/js/bun/jsc/dataview-bigint64-jit.test.ts(new): 50k-iteration JIT round-trip over int64/uint64 boundary values, multi-digit BigInts (mod 2^64 wrap), both endiannesses, error paths (TypeError/RangeError) after tier-up, and a GC stress loop. Passes against a debug ASAN build with the new WebKit.JSTests/stress/dataview-jit-bigint64.js) covering constant/variable endianness, OOB, detached and resizable ArrayBuffers; all existingdataview-*stress tests pass on the ASAN debug build.Note for the fail-before gate: this is an engine performance fix delivered via a WebKit version bump (
scripts/build/deps/webkit.ts), so there is no src/ diff to stash and the correctness test passes on stock Bun too. The behavioral proof is the benchmark above plus the WebKit-side stress test.[decide:webkit] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The slowdown occurred because JSC's DFG and FTL tiers only recognized DataView get and set intrinsics for byte sizes up to 4, so getBigUint64 and setBigUint64 always fell back to a generic call into the C++ host function, paying call overhead and BigInt boxing on every access. The fix extends the DataView intrinsic lowering to handle the 8-byte BigInt variants, emitting inlined 64-bit loads and stores with bounds checks, value speculation, and a native byte swap for the non-native endianness case, so tiered-up code never leaves JIT code for these operations. This brings BigUint64 and BigInt…