Bump WebKit (oven-sh/WebKit#458 preview): Math.sumPrecise rounds negative sums to nearest - #39455
Bump WebKit (oven-sh/WebKit#458 preview): Math.sumPrecise rounds negative sums to nearest#39455robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (3)
Comment |
|
Status
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs in the Bun-side changes. Deferring to a human because the PR description states WEBKIT_VERSION must be repointed from the autobuild-preview-pr-458-* tag to a main sha (and coordinated with #39371) before landing — that sequencing decision needs a maintainer.
What was reviewed:
prebuiltUrl/prebuiltDestDirinwebkit.tshandle theautobuild-prefix correctly, so the preview tag resolves and caches without collision.- The new fixture follows the existing
jsc-stresspattern (// @bunheader, registered injsFixtures, throws on mismatch so exit code drives the assertion). - Spot-checked several expected values (
[-2, ULP/2]tie-to-even →-2,[-MAX, -(2**970)]→-Infinity,[-1, 1]→+0viaObject.is) — they match IEEE-754 round-to-nearest-even.
Extended reasoning...
Overview
The PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts from sha c6cfe90c… to the preview tag autobuild-preview-pr-458-7051f3b5, adds a 123-line JSC stress fixture exercising Math.sumPrecise rounding of negative sums, and registers it in jsc-stress.test.ts. The actual fix lives in oven-sh/WebKit#458; the Bun-side change is the pin plus the regression test.
Security risks
None. The change is a dependency version pin to a build produced by Bun's own oven-sh/WebKit CI, plus a pure-computation test fixture. No user input handling, auth, crypto, or network paths are touched.
Level of scrutiny
Medium-high. While the Bun-side diff is mechanically simple (a one-line pin change, a fixture list append, and a self-contained test file), a WebKit pin bump swaps the entire JS engine underneath Bun. Per the repo's "Dependencies & vendoring" guidance, dep bumps warrant maintainer review. More importantly, the PR description is explicit that this pin is temporary: oven-sh/WebKit#458 must merge first, then WEBKIT_VERSION must be repointed at the resulting main sha, and landing order must be coordinated with #39371 (which carries the Bun-side changes for the post-upgrade WebKit main). Landing the preview tag as-is would ship a non-main WebKit build.
Other factors
- The build script already special-cases
autobuild-prefixed versions in bothprebuiltUrl(uses the tag verbatim) andprebuiltDestDir(strips the prefix for the cache key), so the preview tag is handled correctly for CI purposes. - The test fixture is thorough: 58 base cases covering exact/inexact negative sums, quarter/half/three-quarter-ulp rounding, ties on even and odd mantissas, power-of-two boundaries, the
MAX_VALUE/-Infinityedge, positive mirrors, and the+0cancellation case — each run through bothXsumSmall(array + generator) andXsumLarge(padded past 1000 elements). Assertions useObject.isso-0/+0andNaNare compared correctly. - The fixture matches the harness convention exactly (throws on failure → nonzero exit →
expect(exitCode).toBe(0)fails with stdout/stderr logged). - No prior reviewer comments to address; the only timeline entry is a CodeRabbit rate-limit notice.
|
Confirming the sequencing for whoever lands this: the preview tag is not meant to be merged. Once oven-sh/WebKit#458 merges I will push the repoint of |
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
387bd73 to
3b4cb6e
Compare
|
Rebased onto main (3b4cb6e). #39371 moved main's |
There was a problem hiding this comment.
I reviewed this PR and didn't find any issues in the Bun-side changes. Since WEBKIT_VERSION is currently pointed at a preview tag that (per the description) must be repointed to a merged oven-sh/WebKit main sha before landing, and the engine change itself lives in oven-sh/WebKit#458, a maintainer should coordinate the landing sequence rather than this being auto-approved.
What was reviewed:
- The new fixture's expected values (round-to-nearest-even at ±half-ulp, ties on even/odd mantissas, power-of-two boundaries,
-MAX_VALUEoverflow tie,+0on cancellation viaObject.is) — spot-checked against IEEE 754 semantics. - The >1000-element padding and generator variants correctly exercise the
XsumLargeand forced-XsumSmallpaths;cases.slice()avoids mutating during iteration. prebuiltUrl()/prebuiltDestDir()inwebkit.tsalready handle theautobuild-prefix, so the preview pin resolves correctly.
Extended reasoning...
Overview
This PR changes three files: (1) scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from the eeab0404… sha to the preview tag autobuild-preview-pr-458-ffe26339; (2) a new 123-line JSC stress fixture math-sum-precise-negative-rounding.js covering Math.sumPrecise rounding of negative sums; (3) a two-line addition registering that fixture in jsc-stress.test.ts. The actual behavioral fix is in oven-sh/WebKit#458 (the xsum negative-branch rounding table in WTF::Xsum::XsumSmall::compute), not in this repo.
Security risks
None. The change is a dependency pin plus a test fixture. The fixture is plain arithmetic with no I/O, network, or filesystem access; it runs in a spawned subprocess like every other jsc-stress fixture.
Level of scrutiny
High — not because the Bun-side diff is complex (it isn't; the fixture and registration are straightforward and follow the existing jsc-stress conventions exactly), but because:
WEBKIT_VERSIONcontrols which JavaScriptCore engine every Bun build links. Bumping it is effectively a dependency upgrade of the JS engine.- The pin is currently at a preview tag. The PR description and the robobun status comment both explicitly state the preview tag is not meant to be merged and must be repointed to the resulting oven-sh/WebKit main sha once #458 lands there.
- The correctness of the engine change (the xsum rounding-table fix) can only be reviewed in oven-sh/WebKit#458, not here.
Other factors
The fixture itself is well-constructed: it uses Object.is so -0/+0 and NaN are distinguished, covers each rounding outcome for negative sums plus positive mirrors, exercises both accumulator sizes (array vs. >1000-element padded array vs. generator), and the expected values I spot-checked are consistent with IEEE 754 round-to-nearest-even. The registration in jsc-stress.test.ts matches the neighboring entries. webkit.ts already special-cases autobuild--prefixed versions in both prebuiltUrl() and prebuiltDestDir(), so the preview tag is handled by existing machinery. No bugs were found by the bug-hunting system. Given the explicit landing-order dependency on an unmerged upstream PR and that WebKit bumps are maintainer-coordinated, deferring rather than auto-approving.
|
Agreed, this stays on the preview tag until oven-sh/WebKit#458 merges; I will repoint |
|
Updated 10:34 PM PT - Aug 17th, 2026
✅ @robobun, your commit 3b4cb6e1eeb6ab768f4a913035090d04ae2884bc passed in 🧪 To try this PR locally: bunx bun-pr 39455That installs a local version of the PR into your bun-39455 --bun |
Problem
Math.sumPrecisereturns every exactly representable negative sum one ulp too large in magnitude:Math.sumPrecise([-1])is-1.0000000000000002,[-0.5]is-0.5000000000000001,[1, -2]is-1.0000000000000002,[-Number.MAX_VALUE]is-Infinity. A negative sum just inside a power of two is rounded onto it too ([-2, 2 ** -52]gives-2instead of-1.9999999999999998). Positive sums and inexact negative sums are right. Same on Bun 1.3.14, so not a 1.4 regression; found by a differential fuzz run against an exact reference, where all 468 of 10,000 mismatching arrays fell into this one bucket.WTF::Xsum::XsumSmall::compute()(Source/WTF/wtf/PreciseSum.cpp, the port of xsum thatMath.sumPrecisesums into; arrays longer than 1000 elements useXsumLarge, which is rounded by the same function). Its negative branch rounds away from zero whenever no bit below the two guard bits is set, where the original xsum only does so for guard bits11, or10on an odd mantissa (a tie). An exactly representable sum has no bits set below the mantissa, so it always took that path. Upstream WebKit has the same code.Fix
c6cfe90cprebuilt WTF and comparing it bit for bit with Radford Neal'sxsum.con 3.75 million generated inputs (805,856 with a negative, exactly representable sum; the unmodified file mismatches on 20% of them), plus a 30,000 input sample against exact rational arithmetic.WEBKIT_VERSIONat that PR's preview build,autobuild-preview-pr-458-ffe26339, so CI runs Bun against it. The WebKit branch sits on current fork main (eeab0404, which is also what main pins since Upgrade WebKit to 47f7250137c6 #39371), so the preview is exactly the current pin plus this fix.WEBKIT_VERSIONhas to be repointed at the resulting main sha (the build prints that instruction itself once the preview release disappears); I will push that repoint when the merge happens. If another bump that already contains next always using default port for stylesheet, even when running on another port #458 lands first, this PR reduces to the fixture and its registration.test/js/bun/jsc-stress/fixtures/math-sum-precise-negative-rounding.js, theJSTests/stressfile from the WebKit PR with the usual// @bunfirst line, registered injsc-stress.test.ts. It checks 58 cases: the sums from the report, each rounding outcome for a negative sum (quarter ulp, three quarters, ties on even and odd mantissas, the same ties with one subnormal added or removed so the scan of the lower accumulator chunks decides, magnitudes just inside a power of two, the-MAX_VALUEneighbourhood including the-Infinitytie) and the positive mirror images. Each case runs as an array, as the same array padded past the 1000 elementXsumLargethreshold with cancelling pairs, and as a generator (alwaysXsumSmall), plus three plain 1001 element arrays. All 119 expected values were checked against exact rational arithmetic independently of any engine.eeab0404,process.versions.webkitchecked),bun bd test test/js/bun/jsc-stress/jsc-stress.test.ts -t math-sum-precisefails at the fixture's first case withMath.sumPrecise([-1]) returned -1.0000000000000002, expected -1; the same happens on the previousc6cfe90cpin and on the releasedbun, and 92 of the fixture's 238 assertions fail on those engines.process.versions.webkitreportspreview-pr-458-ffe26339), the fixture passes and the wholejsc-stress.test.tspasses its 116 fixtures (debug + ASAN build; the new fixture takes about 1.2 s there and 30 ms on the releasejscshell);test/js/bun/jsc/temporal-global.test.tspasses as well.Background
scripts/build/deps/webkit.tspins which build. An engine fix lands as a WebKit PR plus a pin bump here, and theautobuild-preview-pr-*releases let the bump PR run Bun's suite against the WebKit PR before it merges.test/js/bun/jsc-stress/runs files taken verbatim from WebKit'sJSTests/stressunderbun, so the engine test is shared with the WebKit PR as is.Earlier revision of this PR
The first revision pinned
autobuild-preview-pr-458-7051f3b5, built from a WebKit branch based just before the 47f7250137c6 upstream upgrade so that it would build against Bun main before #39371 landed. #39371 has landed (main now pinseeab0404), so the WebKit branch was rebased onto current fork main (same three files, identical content) and this PR was rebased onto main; the only conflict was theWEBKIT_VERSIONline, resolved to the new preview tag.Repro
[decide:webkit] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file