test(ffi): speed up ffi.test.js and tighten its assertions - #39041
test(ffi): speed up ffi.test.js and tighten its assertions#39041robobun wants to merge 1 commit into
Conversation
… batch the subprocess tests ffi.test.js made 395k expect() calls, almost all of them in the twelve "integer identities work for all possible values" sweeps (32768 steps per type per runner). On the ASAN lane those sweeps were about 75% of the file's time, and on a loaded machine individual sweeps exceeded the default 5s per-test timeout. Each wider type now round-trips a fixed set of values a conversion can get wrong (range ends, 0/+-1, every power of two in range with its neighbours and negations, alternating bit patterns, a 16-step stride) through one toEqual, then cycles them through a 10k-iteration loop without per-call expect() so the call site is still exercised past the interpreter. The 8-bit types stay exhaustive. The toBuffer subprocess block now directly follows the three top-level subprocess tests and all of them are marked concurrent, so bun:test runs the six child processes (plus the in-process finalizer test) as one batch instead of serially. Assertions tightened while here: the threadsafe callback tests await the delivered value (the old expect() inside the callback ran after the test had finished) and assert the call was queued rather than run inline; a new test drives the previously unused cb_identity_* fixture functions through JS callbacks of every return type with one structured toEqual; i64_fast/u64_fast number-vs-BigInt return policy is asserted explicitly; "ffi print" checks the generated receiver source instead of length > 0; the dlopen failure test checks code/syscall/message; the 63-symbol test compares the symbol set, not its length; callbacks created by the roundtrip tests are closed.
|
Updated 11:59 AM PT - Aug 15th, 2026
❌ @robobun, your commit e78123c has some failures in 🧪 To try this PR locally: bunx bun-pr 39041That installs a local version of the PR into your bun-39041 --bun |
|
Warning Review limit reached
Next review available in: 6 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 (1)
Comment |
|
Status: ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup and assertion tightening; no coverage removed (151→154 tests), every changed assertion is strictly stronger.
What was reviewed:
valuesForboundary generator: BigInt arithmetic, min/max/stride bounds, unsigned filtering of negatives, and the normalize path for both fast and non-fast 64-bit — all round-trip cleanly.- New
cb_identity_*test againstffi-test.c: each fixture isreturn cb();, so the JSCallback return-value/expected pairs (including thebig()split for i64/u64 in fast vs non-fast) line up. - Concurrent regrouping: the moved
toBufferblock is byte-identical moduloit.concurrent.skipIf(used elsewhere in the suite); the in-process finalizer test's C static counter isn't read by any other concurrently-running test. - Threadsafe-callback rewrite now actually awaits delivery (
Bun.peek.statuspending →await promise) instead of the old fire-and-forgetawait 1.
Extended reasoning...
Overview
Single-file change to test/js/bun/ffi/ffi.test.js. Replaces twelve 32,768-step expect().toBe() sweeps with boundary-value sets plus a mismatch-recording 10k loop; regroups six subprocess tests (plus one in-process finalizer test) into one test.concurrent batch by moving the toBuffer describe block; tightens assertions in ffi print, threadsafe callback, dlopen error, JSCallback close, and the 63-symbol test; adds a new test driving the 13 previously-unused cb_identity_* fixture functions and a fast-int64 number-vs-BigInt policy test; wraps roundtrip callbacks in try/finally with .close().
Security risks
None. Test-only; no production code, no new external I/O, no network.
Level of scrutiny
Medium — it's a meaningful rewrite of test logic, not a mechanical tweak, so the risks are (a) silently dropping coverage and (b) new flakes. On (a): the 8-bit types remain exhaustive; the wider types now cover min/max/0 (which the old test asserted explicitly) plus every power-of-two boundary and alternating-bit pattern the old fixed stride skipped, and the 10k loop still exercises the JIT tier-up path. Every other assertion change is a strict strengthening (toBeGreaterThan(0) over != 0, error code/syscall/message over truthiness, sorted key-set equality over length, awaited promise over expect inside a callback that ran after the test passed). Test count goes up, nothing is skipped or deleted. On (b): checked that it.concurrent.skipIf is an established pattern in the suite, and that the finalizer test's shared C static (getDeallocatorCalledCount) is not touched by anything else in the concurrent group (the other six are subprocesses).
Other factors
The PR description is unusually thorough — profiled timings, per-lane CI breakdown, explicit "deliberately left alone" reasoning, and verified pass/skip counts with bun bd test. The changes directly implement several of REVIEW.md's own test rules (await the actual condition, release resources in try/finally before assertions, test.concurrent for independent subprocess tests, assert the strongest invariant). No CODEOWNERS entry covers this path. The bug-hunting pass found nothing.
1 participant
Problem
test/js/bun/ffi/ffi.test.jsis one of the slowest files in the suite: CI build #97275 took 23.4s on debian x64-asan and 19.6s on alpine x64, against 4.6s on debian x64, and alpine x64 is 18.1-19.6s in every build I looked at (97206, 97275, 97480, 97784), so it is not runner noise.expect()calls, almost all of them in the twelveinteger identities work for all possible valuestests (32768expect(fn(i)).toBe(i)per type per runner). The CI runner setsBUN_GARBAGE_COLLECTOR_LEVEL=1for every test process (scripts/runner.node.mjs,spawnBun), and at that level every matcher call ends inExpect::post_match->auto_garbage_collect()->mi_collect()plus a JSC collection request (src/runtime/test_runner/expect.rs:1735,src/jsc/VirtualMachine.rs:1257). So each of those 395k matcher calls was also a GC cycle: locally the old file goes from ~1.0s to 6.9s just by setting that variable (each sweep 4-20ms -> 300-660ms). Per lane that came to ~2.3s of test time on debian x64, ~14.6s on alpine x64 (the same path is ~6x more expensive per call on the musl build) and ~17s on the ASAN lane, whereexpect()itself is also instrumented bun code. With the debug build here the sweeps were 4-7s each, ~75% of the file, and 5-9 of them exceeded the default 5s per-test timeout on a loaded machine. The FFI calls themselves are not the cost: they run inside release JSC (the 400k-callCallFFItiering test in the same file takes 73ms in debug).cc -O2fixture compiles take 3.4-4.7s on that lane against ~2-2.5s on debian (other files that shell out toccshow the same lane-specific slowdown:addr32.test.ts0.2s vs 3.3s inexpected-durations.json,fs-stat-seccomp-linux.test.ts1.7s vs 9.6s andserve-file-slice-read-error.test.ts1.8s vs 6.5s in #97275). Nothing else about musl is slow: over the same build the per-file alpine/debian median is 1.03-1.06, and spawn-, GC- and JIT-heavy files are ~1x there (jsc-stress.test.tsruns 83 subprocesses in 0.69s on alpine). The compiles are unchanged by this PR.Workerstartup; 65ms each in release) ran serially, followed separately by the three concurrenttoBuffersubprocess tests.threadsafe callbacktests put theirexpect()inside the callback and thenawait 1, but a threadsafe callback is delivered as an event-loop task, so the assertion ran after the test had already passed;ffi printonly checkedlength > 0; the 13cb_identity_*fixture functions were dlopen'd and destructured but never called;dlopenfailure only checked truthiness.Fix
toEqual: both range ends and their neighbours, 0 and +-1, every power of two in range with its neighbours (and their negations for signed types), the0x5555.../0xAAAA...patterns masked to the width, and a 16-step stride. That is 64-394 values per type (106/202/394 for int16/32/64, 64/112/208 for uint16/32/64); it keeps the min/max/0 checks the old test made explicitly and adds the boundaries its stride did not land on (2^31 and 0xAAAAAAAA for u32, 2^32, 2^53-1/2^53 and 2^63 for the 64-bit types, each power of two and its negation for the signed types). The values are then cycled through a 10k-iteration loop that records mismatches instead of callingexpect()per iteration, so the call site is still exercised after it leaves the interpreter. Each test is now 0.4-1.3ms in release and 20-110ms in debug, and the file makes 1,452expect()calls.toEqualagainst BigInt inputs now also asserts thatint64_t/uint64_treturn BigInts for every value; the fast runner gets an explicit test thati64_fast/u64_fastreturn a number for safe integers and a BigInt beyond them (probes at 2^53-2 and 2^53;u64_fasthas returned a BigInt for exactly 2^53-1 since the TinyCC-eraUINT64_TO_JSVALUE, so the probes stay off that one value).toBufferblock now directly follows the three top-level subprocess tests and all of them (plus the in-process finalizer test, which is the only user of the fixture state it reads) areconcurrent, sobun:testruns the six child processes as one batch. The order is chosen so that the open PRs adding tests to this file (bun:ffi: keep ptr(typedArray) valid across DFG tier-up #32055, bun:ffi: handle negative byteOffset in read.* instead of panicking #32260, Tighten ffi pointer bounds, sparse archive extraction, and the Windows default trust store #31581 afterread; ffi: toArrayBuffer/toBuffer throw RangeError instead of aborting on a huge byteLength #33353 after the Windows dlopen test; bun:ffi: accept FFIType.buffer and FFIType.buffer_length as numeric type tags #38430 after theCFunctionblock and inside the engine-native block) still apply cleanly.Bun.peek.statusispendingright after the native call (queued, not inline, which the worker tests rely on) andawaitthe delivered value, then close the callback; newC code calls back into JS callbacks of every return typetest drives the 13cb_identity_*functions through aJSCallbackeach and compares one result object;ffi printchecks the generated receiver source (float not_a_callback(float arg0);,USES_FLOAT,HAS_ARGUMENTSpresent/absent) and that the callback "source" is signature-independent;dlopenfailure checkscode/syscall/messageas one object; the 63-symbol test compares the symbol name set instead of its length; pointer checks usetoBeGreaterThan/toBeNull; the roundtrip tests close the callbacks they create; the straydoneparameter on adescribeis gone.dlopens in the engine-native and ABI blocks (those tests are 0.04-0.6ms each in release, so hoisting buys nothing and those blocks are where other open PRs add tests); thesetIntervalin the terminate fixture (a keep-alive for a worker that is terminated from outside, not a wait); thelibPathgate for the libc tests, which is dead on every Linux lane becauseisGlibcVersionAtLeastalways returns false there (harness fix in test(harness): make isGlibcVersionAtLeast work with glibc's two-component version #39030).bun bd test test/js/bun/ffi/ffi.test.js149 pass / 5 skip, 154 tests. 8 of 9 debug runs passed on a box with a load average of 100-245; the one failure was the worker-teardown test reaching the local 5s default timeout at 5.00s during a load spike (it takes 3.1-4.6s per run here in debug; CI passes a much larger per-test timeout, and before this change 6-9 tests per run failed the same way on this box). The libc block was also run once locally with its gate removed, since the darwin lane that normally runs it did not get an agent in this build.Test count goes from 151 to 154 (the 13-function callback test in each runner, plus the fast-type policy test); nothing was removed or skipped.
Background
BUN_GARBAGE_COLLECTOR_LEVEL=1is a test-only knob: bun'sbun:testmatchers,Bun.serveand a few other host functions callauto_garbage_collect()after their work, which at level 1 asks mimalloc to release memory and JSC to start a collection. CI sets it so GC-ordering bugs surface; the side effect is that a matcher call costs microseconds instead of nanoseconds there, which is why expect()-dense tests show up as slow only in CI.bun:testruns consecutive tests markedconcurrentas one group (src/runtime/test_runner/Order.rs); any non-concurrent test (including a skipped one) between them starts a new group, which is why the blocks have to be adjacent.threadsafe: trueJSCallbacknever runs inside the native call: the engine hands the invocation to bun, which posts it as a task to the owning event loop (src/jsc/bindings/JSCFFIBridge.cpp), so the caller only sees it after anawaitthat yields to the loop.i64_fast/u64_fastare thebun:ffi64-bit types that return a JS number when the value fits in a safe integer and a BigInt otherwise;int64_t/uint64_talways return a BigInt.ffi-test.c,ffi-abi-fixture.c) are compiled once at module load with the system compiler viacompileFixturefrom the harness; that is now nearly all of the file's time on every lane.Timings
CI, wall clock of the file per lane from the Buildkite line timestamps ("load" is until the first test result is printed, i.e. module load including both fixture compiles; "tests" is the rest):
Alpine x64 in builds 97784 / 97480 / 97206 (old file): 19.0s / 18.1s / 19.0s total, 3.4s / 3.5s / 3.8s of it before the first test result.
Local release build (
USE_SYSTEM_BUN=1), old file vs new file: plain ~1.0-1.3s vs ~0.55-1.25s (both mostly the fixture compiles); with the CI runner'sBUN_GARBAGE_COLLECTOR_LEVEL=1 BUN_JSC_randomIntegrityAuditRate=1.08.1s vs 0.96s. With onlyBUN_GARBAGE_COLLECTOR_LEVEL=1the old file takes 6.9s and each sweep 300-660ms (4-20ms without it);BUN_JSC_randomIntegrityAuditRatealone makes no measurable difference.Local debug (ASAN) build, back to back on the same machine (12 cores, load average 95-245 during the runs, so absolute numbers are inflated):
bun bd test test/js/bun/ffi/ffi.test.jsharnessand compiling both fixturesinteger identitiestestsexpect()callsSlowest tests before (debug build): int64_t 4.8s, int16_t 4.7s, (fast) int32_t 4.4s, uint32_t 4.2s, uint16_t 4.1s, (fast) int16_t 4.0s, int32_t 3.9s, then the remaining sweeps at 5.2-7.4s once they started timing out; worker teardown 3.6s; worker terminate 3.4s; the three toBuffer subprocess tests 1.5s each (concurrent); (fast) primitives 1.1s; JSCallback exceptions subprocess 0.5s. Everything else was under 0.4s.