String.prototype.localeCompare: reuse collator for (string locale, no options) - #36103
String.prototype.localeCompare: reuse collator for (string locale, no options)#36103robobun wants to merge 9 commits into
Conversation
… options)
Passing an explicit string locale like a.localeCompare(b, "en") was
rebuilding a fresh ICU UCollator on every call, while the no-locale form
and new Intl.Collator("en").compare both reuse one. As a sort comparator
this made arr.sort((a,b)=>a.localeCompare(b,"en")) roughly 25x slower
than arr.sort(new Intl.Collator("en").compare), and ~60x slower than
Node/V8 which caches this shape.
JSC now keeps a single-entry per-global collator cache keyed on the raw
locales string, used when locales is a string and options is undefined;
see oven-sh/WebKit#360.
WEBKIT_VERSION points at the preview build for oven-sh/WebKit#360 and
must be bumped to the merge commit's autobuild-<sha> before this PR
merges.
WalkthroughThe PR updates the WebKit prebuilt version to a preview autobuild tag and adds ChangesWebKit prebuilt version
Intl localeCompare tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: verified locally against a source build of oven-sh/WebKit#360 (all 38 Diff is green. oven-sh/WebKit#360's preview release All review threads addressed in 3e35365, a6a0c31, ae3c386, e29fc0c, c4a909e and 0ce84b8. Ready for maintainer review. Before merging:
|
|
Updated 7:49 AM PT - Jul 27th, 2026
❌ @robobun, your commit 0ce84b8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36103That installs a local version of the PR into your bun-36103 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant from the temporary
autobuild-preview-pr-360-52f23e4f pin to the merged autobuild-<sha> identifier,
using the correct merged commit SHA so fresh builds resolve successfully.
In `@test/js/web/intl/intl.test.ts`:
- Around line 183-187: Reorder the assertions in the test “does not leak into
calls that pass options” so the default "en" localeCompare call runs first and
seeds the default cache, followed by the { sensitivity: "base" } call, while
retaining the final default-sensitivity assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 302e34e7-c3ea-4827-8ad0-c98518d51eac
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/web/intl/intl.test.ts
Makes the "does not leak into calls that pass options" test self-contained so it also catches an options call incorrectly reading the cache, without relying on earlier tests in the describe block having seeded it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Take the minimum of three runs per side so a single GC pause or scheduler hiccup on a debug+ASAN CI runner can't push the ratio past the threshold. N dropped to 1500 so six sorts still fit the default test timeout on debug+ASAN; the regression signal is unchanged (unpatched ratio ~5.4 debug / ~48 release vs ~1.0 patched).
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 `@test/js/web/intl/intl.test.ts`:
- Line 225: Update the benchmark setup around bestOf3 so the Intl.Collator("en")
instance is created once before the timed callback, then reuse its compare
method inside the callback passed for hoisted. Keep the array copy and sorting
within the timed operation while removing collator construction from each
iteration.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e40cc4fb-c839-4bd0-b8db-76870b8fe973
📒 Files selected for processing (1)
test/js/web/intl/intl.test.ts
The collator was already hoisted relative to the ~16k comparisons inside each sort, so the effect on the ratio is negligible, but this matches the test name and keeps the timed baseline to just the array copy and sort.
…e-throw poisoning
If the cache key were ever written before initializeCollator throws, the
slot would hold {key: <invalid>, collator: <stale>} and a second call
with the same invalid string would silently return a number instead of
throwing. The single-call version couldn't see that.
No-arg: assert the sign instead of typeof so NaN or a wrong-sign result fails. "a" < "b" holds under every ICU collation so this is default-locale-safe. GC: use "sv" so a GC-cleared slot that fell through to a default/root collator would return the opposite sign for "ä" vs "z" and fail the assertion, rather than just having to avoid crashing.
There was a problem hiding this comment.
Tests look good — all the earlier test-strength suggestions are now applied (seeded cache before the options check, repeated invalid-locale throw, best-of-3 timing with a hoisted baseline, sv in the GC test, .toBeLessThan(0) for the no-arg form). Deferring to a human since the actual behavior change lives in oven-sh/WebKit#360 and WEBKIT_VERSION is still on the preview pin per the PR note.
What was reviewed:
- Cache-invalidation coverage: locale-switch, options bypass (both directions), invalid-locale poisoning, array-locale/no-arg unaffected, GC survival.
- Perf assertion: best-of-3 ratio with hoisted
Intl.Collatorbaseline; checked that the 3x threshold vs. >5x/>20x unfixed leaves margin on both sides. WEBKIT_VERSIONhandling inscripts/build/deps/webkit.ts—autobuild-prefix is already handled byprebuiltUrl/prebuiltDestDir.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts to a preview build of oven-sh/WebKit#360 and adds a describe block to test/js/web/intl/intl.test.ts covering the new per-global collator cache for String.prototype.localeCompare(b, "<locale>") when options is undefined. The substantive runtime change — the cache in stringProtoFuncLocaleCompare, its WriteBarrier slot, and visitChildren wiring — lives entirely in the paired WebKit PR and is not visible in this diff.
Security risks
None identified. The change is a read-only performance cache keyed on a primitive string; no untrusted-input parsing, auth, or filesystem paths are touched. The tests are hermetic (no network, no external registries).
Level of scrutiny
Medium-high. In isolation this is just a version bump plus tests, but the paired JSC change adds GC-visible state to a global object. The Bun-side tests now exercise cache invalidation on locale switch, options bypass in both directions, invalid-locale error paths, GC survival with a locale-discriminating probe, and a ratio-based perf regression guard — but correctness of the WriteBarrier/visitChildrenImpl wiring and exception-scope handling in stringProtoFuncLocaleCompare can only be reviewed against the WebKit diff itself.
Other factors
All prior review feedback (CodeRabbit and my own inline nits across three rounds) has been applied and the threads are resolved. The remaining blocker is procedural and already called out in the PR description: WEBKIT_VERSION points at autobuild-preview-pr-360-52f23e4f, which must be replaced with the merge commit's autobuild-<sha> once oven-sh/WebKit#360 lands. That, plus the fact that the load-bearing C++ change is out-of-tree, makes this one for a human to sign off on rather than auto-approve.
options=null must throw at ToObject(null) per ECMA-402. This guards against a future .isUndefinedOrNull() gate (vs the correct .isUndefined()) letting a warm-cache hit swallow that TypeError.
There was a problem hiding this comment.
Tests look solid — all six rounds of strengthening are in and I have nothing further on this side. Deferring rather than approving because the actual behavior change lives in oven-sh/WebKit#360 (per-global WriteBarrier<IntlCollator> cache + exception-scope handling in stringProtoFuncLocaleCompare), which isn't visible from this diff, and WEBKIT_VERSION is still on the preview pin the description flags for a pre-merge bump.
What was reviewed:
- Cache-invalidation coverage: locale switching, invalid-locale repeat, options/
nullbypass, array-locale and no-arg forms — all assert locale-discriminating values. - GC-survival test uses
svso a cleared slot fails on sign, not just on crash. - Perf ratio assertion: best-of-3 with the baseline collator hoisted; considered flake risk on debug+ASAN — 3× threshold over ~1.0 true ratio should hold.
WEBKIT_VERSIONbump handled correctly byprebuiltUrl/prebuiltDestDir(both branch on theautobuild-prefix).
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION to autobuild-preview-pr-360-52f23e4f, and test/js/web/intl/intl.test.ts gains a ~95-line describe block covering the new per-global collator cache for String.prototype.localeCompare(b, "<locale>") with undefined options. The runtime change itself — the single-entry cache in stringProtoFuncLocaleCompare — lives entirely in oven-sh/WebKit#360; this PR only consumes its preview build and adds the Bun-side test net.
Security risks
None in this diff. No user-input parsing, no auth/crypto surface. The JSC change caches an IntlCollator keyed on a primitive locale string with no observable side effects per ECMA-402, so there's no spec-observable behavior change to leak. The memory-safety questions (is the WriteBarrier slot visited in visitChildrenImpl? is the key written only after initializeCollator succeeds? RETURN_IF_EXCEPTION after every fallible call?) belong to the WebKit PR review.
Level of scrutiny
The Bun-side changes are low risk: a version-pin string edit (whose autobuild- prefix is already handled by prebuiltUrl and prebuiltDestDir) and an additive test block. But a WebKit version bump pulls in native JSC changes to a GC-managed per-global cache — REVIEW.md's memory-safety section applies to that code, and I can't inspect it from here. That, plus the PR description's own note that WEBKIT_VERSION must be re-pinned to the merge sha once oven-sh/WebKit#360 lands, makes this unsuitable for bot approval.
Other factors
Every prior review nit (best-of-3 perf sampling, seed-cache-before-options, repeat-invalid-locale, hoisted baseline collator, sv for the GC test, .toBeLessThan(0) for the no-arg form, options=null → TypeError) has been applied and all threads are resolved. No human reviews on the thread. The bug-hunting system found nothing this run. The perf test's 3× ratio threshold with best-of-3 sampling over ~16k comparisons should be robust to CI jitter; if it does flake, the fix is widening the threshold rather than dropping the assertion.
2 participants
What
was rebuilding an ICU
UCollatoron every comparison. Only the no-locale forma.localeCompare(b)hit JSC's cached default collator; any explicitlocalesargument (even the string"en") took theIntlCollator::create+initializeCollatorpath per call.Repro
sort((a,b)=>a.localeCompare(b,"en"))sort(new Intl.Collator("en").compare)Fix
stringProtoFuncLocaleComparenow consults a single-entry per-global cache keyed on the rawlocalesstring whenlocalesis a primitive string andoptionsis undefined. ConstructingIntl.Collatorwith that shape has no observable side effects and its result is fully determined by the string, so reusing the collator is spec-equivalent. A different locale string overwrites the slot; an invalid locale that throws duringinitializeCollatoris never cached. V8 has the same fast path for this shape.The JSC change is oven-sh/WebKit#360; this PR bumps
WEBKIT_VERSIONto its preview build and adds tests intest/js/web/intl/intl.test.tscovering:new Intl.Collator(locale).comparefor several localesRangeErrorand do not poison the cacheoptionsare unaffectedlocaleCompare(b, "en")as a sort comparator is within 3x of a hoistedIntl.Collator("en").compare(previously 25x+)Note
WEBKIT_VERSIONcurrently points atautobuild-preview-pr-360-52f23e4f. Before merging, bump it to theautobuild-<sha>of the merge commit once oven-sh/WebKit#360 lands.no test proof · iteration 6 · Platform-specific test-only change; deferring to CI.