test(jsc-stress): port the DataView JIT stress tests from JSTests - #38117
test(jsc-stress): port the DataView JIT stress tests from JSTests#38117robobun wants to merge 2 commits into
Conversation
Adds the six JSTests/stress fixtures that cover the DFG/FTL DataView intrinsics, including the BigInt64/BigUint64 accessors that gained intrinsics in WebKit 317392@main (in Bun since the WebKit upgrade in #34373). Bun's suite had no test exercising any DataView accessor after tier-up. The fixtures are verbatim upstream apart from the `// @bun` pragma the other fixtures in this directory carry. dataview-jit-bigint64.js uses the jsc shell's transferArrayBuffer(), so preload.js gains a polyfill built on ArrayBuffer.prototype.transfer(). Co-authored-by: Sosuke Suzuki <sosuke@bun.sh>
|
Warning Review limit reached
Next review available in: 46 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 (12)
Comment |
|
Updated 1:09 PM PT - Aug 13th, 2026
✅ @robobun, your commit 26d63780471462074eb63611573b7da1f0abc20d passed in 🧪 To try this PR locally: bunx bun-pr 38117That installs a local version of the PR into your bun-38117 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only port of upstream JSTests DataView JIT fixtures following the existing jsc-stress pattern.
What was reviewed:
- Six new fixtures are registered in
jsFixturesand carry the// @bunheader like their siblings; they use only globals the preload already provides plus the newtransferArrayBuffer. transferArrayBufferpolyfill viaArrayBuffer.prototype.transfer()correctly detaches; existing FFI fixtures that probe for it already fall back to the same.transfer()call, so no behavior change there.dataview-jit-unaligned-accesses.jsusesMath.random()but the index is clamped to[0, size-8]so all 8-byte accesses stay in bounds — not a flake source.- Reported per-fixture timings (≤5.2s debug/ASAN) fit within the existing 180s debug
fixtureTimeout.
Extended reasoning...
Overview
This PR ports six DataView JIT stress test fixtures from WebKit's JSTests/stress into test/js/bun/jsc-stress/fixtures/, registers them in the jsFixtures array of jsc-stress.test.ts, and adds a transferArrayBuffer polyfill to preload.js. The fixtures exercise the DFG/FTL intrinsics for DataView.prototype.{get,set}Big{Int,Uint}64 that landed in WebKit 317392@main. No src/ changes.
Security risks
None. Test-only additions running vetted upstream WebKit test code in isolated subprocesses via the existing harness. No network, no external I/O, no auth/crypto surface.
Level of scrutiny
Low. The change follows the exact established pattern of the jsc-stress directory (verbatim JSTests fixtures with a // @bun header, driven by --preload preload.js, pass = exit 0). The only non-fixture code is 8 lines appending to an array literal and a 3-line polyfill that wraps ArrayBuffer.prototype.transfer(). The fixtures themselves are upstream code covered by the directory's existing BSD-2-Clause LICENSE.
Other factors
- I checked the side effect of defining
globalThis.transferArrayBuffer: five existing FFI fixtures probe for it, but each either checksbuffer.transferfirst or falls back tobuffer.transfer()— semantically identical to the new polyfill — so no observable change to their behavior. dataview-jit-unaligned-accesses.jscontainsMath.random(), which the repo review guide flags in tests, but here the random index is clamped so every access (max 8 bytes) fits within the 16 KiB buffer; this is verbatim upstream and cannot produce a bounds-dependent flake.- The PR notes the tests pass under
USE_SYSTEM_BUN=1; that is expected and appropriate for this directory — these are JIT-correctness regression guards against future WebKit bumps, not tests of a same-PRsrc/change, matching every other fixture here. - Reported run: 121 pass / 0 fail on debug+ASAN with per-fixture timings well under the 180 s debug timeout, and <150 ms each on release. No timeout or concurrency changes were made to the test file.
|
Status: test-only follow-up to the triage of #34238 and oven-sh/WebKit#294, both now closed as superseded by upstream WebKit 317392@main (in Bun since #34373; details in the collapsed section of the description). Second push (26d6378) adds the four remaining upstream DataView JIT tests ( Verified locally with On the review note about the new |
dataview-jit-get.js, dataview-jit-set.js, dataview-jit-neuter.js and dataview-get-cse.js cover the non-BigInt DataView accessors through the same DataViewGetInt / DataViewGetFloat / DataViewSet nodes, which the BigInt64 change also touched in the shared fixup, abstract interpreter and CSE code. dataview-jit-neuter.js is the second user of the transferArrayBuffer() polyfill. Verbatim upstream apart from the `// @bun` line, like the others.
There was a problem hiding this comment.
LGTM — test-only port of upstream JSTests DataView fixtures into the existing jsc-stress harness.
What was reviewed:
- Fixture registration in
jsFixturesand the// @bunheader match the directory's existing conventions; fixtures are verbatim upstream copies covered by the existing LICENSE. - The new
transferArrayBufferglobal inpreload.js: the five ffi fixtures that feature-detect it either try.transfer()first or fall back to.transfer(), so their behavior is unchanged now that the global is defined. - Checked that no
src/code is touched and no CODEOWNERS path applies.
Extended reasoning...
Overview
This PR adds ten DataView JIT stress fixtures from WebKit's JSTests/stress into test/js/bun/jsc-stress/fixtures/, registers them in the jsFixtures array in jsc-stress.test.ts, and adds a transferArrayBuffer polyfill to preload.js (implemented via ArrayBuffer.prototype.transfer()). No runtime (src/) code is modified. The fixtures are verbatim upstream apart from the leading // @bun line that all existing fixtures in this directory carry.
Security risks
None. This is test-only; the fixtures run in spawned subprocesses via the existing harness, do not touch the network, and add no new API surface. The one new global lives in a test preload script and only wraps a standard ES method.
Level of scrutiny
Low. The change follows the established pattern for this directory precisely: drop the upstream fixture in fixtures/, add its filename to the jsFixtures list, and polyfill any jsc-shell-only globals in preload.js. The fixture bodies themselves were authored and reviewed upstream in WebKit, so the review here is about wiring, not test logic. The jsc-stress.test.ts edit is a 13-line append with a comment header matching neighboring blocks, and the preload edit is a 4-line polyfill.
Other factors
- I grepped for existing
transferArrayBufferusage: five ffi fixtures reference it, and each either checkstypeof buf.transfer === "function"first (so takes that branch, unchanged) or hasbuffer.transfer()as its else branch (so the newly-defined polyfill produces identical behavior). The author's status comment already notes all 121 tests pass with the change. - Per-fixture timing (44-133ms release, 0.5-5.2s debug/ASAN) is in line with the rest of this concurrently-run suite and well under the debug
fixtureTimeout. - No CODEOWNERS entry covers
test/js/bun/jsc-stress/. - The bug-hunting pass found nothing; there are no outstanding human review comments on the thread.
2 participants
Problem
DataView.prototype.getBigInt64/getBigUint64/setBigInt64/setBigUint64gained DFG/FTL intrinsics in WebKit 317392@main (oven-sh/WebKit commit e164bce2e888), which Bun has shipped since the WebKit upgrade in Upgrade WebKit to 2603e9eb41f0 #34373. That is what closed DataView BigUint64 byte swapping is ~26x slower than two Uint32 operations #34231.ftl-library-inlining-loops.js, which hitssetInt32/setInt8/getInt8in a single loop. Nothing exercises the BigInt64 accessors, the float accessors, the endianness argument, or the bounds, detach, unaligned and CSE paths after tier-up, so a future WebKit bump that breaks the lowering would only be caught by WebKit's own test runs, which do not cover every platform Bun ships on.Fix
JSTests/stressintotest/js/bun/jsc-stress/fixtures/and registers them injsFixturesinjsc-stress.test.ts:dataview-jit-bigint64.js,dataview-jit-bigint64-byte-offset.js,dataview-jit-bigint64-cse-and-aliasing.js,dataview-jit-bigint64-type-check-failures.js, plusdataview-jit-bounds-checks.jsanddataview-jit-unaligned-accesses.js, which it extended with 64-bit cases;DataViewGetInt/DataViewGetFloat/DataViewSetnodes whose shared fixup, abstract interpreter and CSE handling the commit also touched:dataview-jit-get.js,dataview-jit-set.js,dataview-jit-neuter.js,dataview-get-cse.js.LICENSEin that directory) apart from the// @bunfirst line that the other fixtures carry, so the source reaches JSC untranspiled.preload.jsgainstransferArrayBuffer(), the jsc shell helperdataview-jit-bigint64.jsanddataview-jit-neuter.jsuse to test the detached-buffer path; it is implemented withArrayBuffer.prototype.transfer(), which detaches the source buffer. Five ffi fixtures already feature-detect this global and fall back to.transfer()themselves, so they behave the same either way.scripts/verify-baseline.tspicks up everyfixtures/*.jsautomatically, so these also run under the no-AVX baseline emulation on WebKit changes. Porting the upstream tests keeps the coverage identical to what the intrinsics were reviewed against upstream.bun:jsc'snumberOfDFGCompiles, thegetVar/setVarhelpers fromdataview-jit-bigint64.jsreport 1 compile each after the main loop, both on a release build (testLoopCount= 10000) and on the debug build (testLoopCount= 1000).bun bd test test/js/bun/jsc-stress/jsc-stress.test.ts: 125 pass, 0 fail (debug/ASAN at the current WebKit pin; the ten new fixtures take 0.5s to 4.2s each there and 30ms to 90ms on a release build).USE_SYSTEM_BUN=1 bun test test/js/bun/jsc-stress/jsc-stress.test.ts -t dataview: 10 pass on the current canary.Background
DataViewGetInt/DataViewGetFloat/DataViewSetnodes; the upstream change extended those nodes to the byteSize == 8 BigInt case.testLoopCount(set inpreload.js, 10000 in release and 1000 in debug builds) is the iteration count the JSTests harness uses to force that tier-up;noInline()keeps each helper a separate compilation unit so its own tier-up is what gets exercised.test/js/bun/jsc-stress/runs verbatim copies of WebKitJSTestswithbun --preload preload.js <fixture>; the preload supplies the jsc shell globals the tests expect (noInline,testLoopCount,fullGC, and nowtransferArrayBuffer). A fixture passes when the process exits 0.Triage notes: how #34238 and oven-sh/WebKit#294 were confirmed obsolete
scripts/build/deps/webkit.tson main pinscaad865eb1a6e5ca4427f5ea1f066140b11953e7. At that sha,Source/JavaScriptCore/runtime/Intrinsic.handJSDataViewPrototype.cppdeclareDataViewGetBigInt64/DataViewGetBigUint64/DataViewSetBigInt64/DataViewSetBigUint64,DFGByteCodeParser.cpphandles them, andDFGSpeculativeJIT64.cpp/FTLLowerDFGToB3.cppcontain the 64-bit lowering (emitAllocateJSBigInt64,allocateHeapBigInt64,operationUInt64ToBigInt).e164bce2e888("[JSC] HandleDataViewBigInt64 accessors in DFG / FTL", WebKit 317392@main, 2026-07-17). The GitHub compare API reports it as an ancestor ofa0e65bf29849(the sha Upgrade WebKit to 2603e9eb41f0 #34373 pinned on 2026-07-20) and of the current pin, and not an ancestor of4895f45dfbd0(the sha Add JIT intrinsics for DataView BigInt64/BigUint64 accessors (WebKit upgrade) #34238 was based on).operationUint64ToBigInt,JSBigInt::offsetOfHash); those identifiers are absent at the pinned sha, so it was never merged and is superseded.view641.4 to 1.55 GiB/s, 2.5x to 3.1x behind the two-Uint32version, compared with the 26x gap reported in the issue.Earlier revision of this description
The first push ported only the six fixtures touched by the intrinsics commit and claimed Bun had no DataView accessor coverage after tier-up. Self-review turned up
ftl-library-inlining-loops.js, which does cover three of the integer accessors, and the four remaining upstream DataView JIT tests; the second commit adds those and this description was corrected accordingly.