String.prototype.localeCompare: cache IntlCollator for (string locale, no options) - #360
String.prototype.localeCompare: cache IntlCollator for (string locale, no options)#360robobun wants to merge 1 commit into
Conversation
…, no options) stringProtoFuncLocaleCompare already reuses the default collator when both locales and options are undefined. Passing any explicit locales argument, even the plain string "en", fell through to IntlCollator::create + initializeCollator on every call, so the idiomatic sort comparator arr.sort((a, b) => a.localeCompare(b, "en")) rebuilt a UCollator for every comparison. Add a single-entry cache on JSGlobalObject, keyed on the raw locales string, used when locales is a primitive string and options is undefined. Constructing Intl.Collator with that shape has no observable side effects and its result is fully determined by the string, so reusing the collator is spec-equivalent. The cache is invalidated simply by overwriting the slot when the string changes; invalid locales that throw during initializeCollator are never cached. The DFG/FTL StringLocaleCompare intrinsic is untouched (it only handles the 1-argument form). Guarded by USE(BUN_JSC_ADDITIONS).
|
Warning Review limit reached
Next review available in: 10 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but I'd like a human to sign off on the spec-equivalence claim for caching IntlCollator on the raw locales string — it looks correct, but it's a semantic optimization in a core runtime path.
What was reviewed:
- GC:
m_cachedLocaleCompareCollatoris aWriteBarrierand is appended invisitChildrenImpl, so the cached collator is kept alive. - Exception path: an invalid locale that throws in
initializeCollatorreturns beforesetCachedLocaleCompareCollator, so a half-initialized collator is never cached. - Cache key: keyed on the raw string, so
"de"vs"de-u-co-phonebk"are separate entries; the cached object is never exposed to JS, so it can't be mutated between calls.
Extended reasoning...
Overview
Adds a single-entry IntlCollator cache on JSGlobalObject, keyed by the raw locales string, for the shape String.prototype.localeCompare(that, <string>) with options === undefined. Touches three files: two new WriteBarrier/String members plus accessor methods on JSGlobalObject (with a matching visitChildrenImpl append), and a new else if branch in stringProtoFuncLocaleCompare. All new code is behind USE(BUN_JSC_ADDITIONS).
Security risks
None identified. No new untrusted-input parsing beyond what initializeCollator already does; the cached object is created via the same path as the existing slow path and is never exposed to user JS.
Level of scrutiny
Medium-high. While the diff is small and mechanically clean (WriteBarrier + visit + accessor follows the established pattern used throughout this file), the correctness of the optimization rests on the claim that new Intl.Collator(<primitive string>, undefined) is side-effect-free and fully determined by the string. That claim looks right — canonicalizeLocaleList on a primitive string runs no user code, options coercion is skipped for undefined, and the resulting collator has no mutable state — but it's an ECMA-402 spec-equivalence argument in a hot runtime path, not a mechanical refactor. Someone with Intl/JSC ownership should confirm they agree with the reasoning (e.g., that locale fallback to the default locale for an unmatched tag doesn't need cache invalidation, mirroring the existing m_defaultCollator assumption).
Other factors
The GC integration is correct (WriteBarrier<IntlCollator> + visitor.append in the existing BUN_JSC_ADDITIONS block of visitChildrenImpl). The cache-miss path is byte-identical to the previous slow path plus a store, so behavior on first call is unchanged. The localeString is resolved via asString(locales)->value(globalObject) after locales.isString(), which cannot re-enter user code. No tests are added in this PR; the existing ECMA-402 test suite in JSTests should exercise the path but won't specifically stress cache reuse across differing locale strings.
Preview Builds
|
1 participant
stringProtoFuncLocaleComparealready reusesglobalObject->defaultCollator()when bothlocalesandoptionsare undefined. Passing any explicitlocalesargument, even the plain string"en", fell through toIntlCollator::create+initializeCollatoron every call, so the idiomatic sort comparatorrebuilt a
UCollatorfor every comparison. On a 200k-element sort this is ~25x slower thanarr.sort(new Intl.Collator("en").compare)and ~60x slower than Node/V8, which caches this shape.This adds a single-entry cache on
JSGlobalObject, keyed on the rawlocalesstring, used whenlocalesis a primitive string andoptionsis undefined. ConstructingIntl.Collatorwith a string locale and undefined options 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. The DFG/FTLStringLocaleCompareintrinsic is untouched (it only applies to the 1-argument form).All new state and fast-path code is guarded by
USE(BUN_JSC_ADDITIONS).Covers the same fast path V8 has for this shape; see https://github.com/oven-sh/bun/issues with the
localeCompareperf report.Bench (release, 200k-element sort):
sort((a,b)=>a.localeCompare(b,"en"))sort(new Intl.Collator("en").compare)localeCompare(b,"en")