Object.seal/freeze: take the JSObject fast path for arrays and indexed objects - #35478
Object.seal/freeze: take the JSObject fast path for arrays and indexed objects#35478robobun wants to merge 1 commit into
Conversation
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Updated 3:30 AM PT - Jul 25th, 2026
⏳ @robobun, your commit 119953e is still building in
|
ba6af41 to
5d1300e
Compare
|
Warning Review limit reached
Next review available in: 24 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 |
|
Review on oven-sh/WebKit#337 caught a real regression: the new
Build 80407 on the rebased Merge order: oven-sh/WebKit#337 first, then this PR's |
There was a problem hiding this comment.
No issues with the one-line diff itself — prebuiltUrl() and prebuiltDestDir() both already branch on the autobuild- prefix, so the preview tag resolves correctly. Deferring because this is a JSC engine behavior change (Object.seal/freeze fast path + two other WebKit PRs in range) riding on an autobuild-preview-* tag that, per the merge-order note above, still needs to be swapped for the merged oven-sh/WebKit main SHA before this lands. That coordination step and the upstream engine change itself want a human sign-off.
Checked: the version-string handling in webkit.ts for non-40-hex tags; that no other file in the repo hardcodes the old WebKit SHA.
Extended reasoning...
Overview
The Bun-side diff is a single line: WEBKIT_VERSION in scripts/build/deps/webkit.ts moves from commit 54917009… to the preview tag autobuild-preview-pr-337-c00301cf. The substantive change lives in oven-sh/WebKit#337 (bulk seal/freeze on SparseArrayValueMap, widened fast-path gate in objectConstructorSeal/Freeze), plus two other WebKit PRs (#328 inspector throw-scope, #317 LiteralParser OOM) that fall in the bumped range.
Security risks
None introduced by the Bun diff. The upstream change touches JS-observable engine semantics for Object.seal/Object.freeze on arrays and indexed objects; the PR description reports test262 (built-ins/Object/{seal,freeze,isSealed,isFrozen,preventExtensions}) and JSTests stress coverage pass, and that Proxy/DerivedArray still take the observable generic path.
Level of scrutiny
Mechanically this is a version bump, but effectively it is a JavaScriptCore engine change to core language builtins — the class of change the repo's own guidance ("Dependencies & vendoring") flags for human attention. It also carries an explicit process dependency: robobun's own comment states oven-sh/WebKit#337 must merge first and then WEBKIT_VERSION here must be updated from the preview tag to the merged main SHA. As written, the PR is intentionally not in its final merge state.
Other factors
I confirmed the build script already handles non-SHA version strings: prebuiltUrl() skips re-prefixing when the value already starts with autobuild-, and prebuiltDestDir() strips the autobuild- prefix rather than truncating to 16 chars, so the cache key stays distinct. No other files reference the old SHA. CI on build 79977 is effectively green (one unrelated aarch64-musl runner timeout, four retry-green flakes). Nothing blocks beyond the upstream-merge → SHA-swap step, which a human should coordinate.
2eaaec7 to
cb814a4
Compare
cb814a4 to
119953e
Compare
Problem
Object.seal/Object.freezeon aJSArray(or a plain object with indexed properties) always fall through to JSC's genericSetIntegrityLevelloop, which per element allocates anIdentifier, dispatches through the method table, and runs the fulldefineOwnPropertydescriptor validation. For the #6360 benchmark (2000 arrays, avg 1000 elements) that costs ~460ms forsealand ~530ms forfreeze, vs ~3ms in Node/V8:Fix
oven-sh/WebKit#337 adds bulk
seal()/freeze()toSparseArrayValueMap(mirroringPropertyTable::seal/freeze), teachesJSObject::seal/freezeto apply it to the sparse map afterenterDictionaryIndexingMode, freezesJSArray"length" from the same place, and widens theobjectConstructorSeal/objectConstructorFreezefast-path gate fromis<JSFinalObject> && !hasIndexedPropertiestois<JSFinalObject> || isJSArray.isJSArraymatches only exactArrayType, soDerivedArrayTypesubclasses and every receiver with overridden[[DefineOwnProperty]]/[[PreventExtensions]](e.g.Proxy) still take the generic observable loop.Results (x86_64, release, local WebKit build)
2000 iterations,
new Array(i).fill(i):Object.seal(arr)Object.freeze(arr)preventExtensions(arr)The remaining ~90ms is
enterDictionaryIndexingModeitself (contiguous →ArrayStoragevector →SparseArrayValueMap); V8 keeps a contiguous sealed/frozen elements kind and so never pays for a sparse-map conversion. That is a larger change.Verification
The WebKit PR carries
JSTests/stress/object-seal-freeze-array-fast-path.jscovering descriptor attributes on dense / holey / accessor-bearing / named-prop-bearing arrays and plain objects,lengthwritability, seal-after-preventExtensions, freeze-after-seal, idempotence, strict-mode throw semantics, and that aProxytarget still observes thepreventExtensions/ownKeys/definePropertytraps.All 279 test262 tests under
built-ins/Object/{seal,freeze,isSealed,isFrozen,preventExtensions}and the pre-existingJSTests/stress/*{seal,freeze,frozen}*pass.There is no Bun-side test proof for this change: the diff is
WEBKIT_VERSIONonly (the fix lives in JavaScriptCore) and the new fast path is semantically identical to the generic loop, so a correctness test passes with and without the fix. Before/after numbers above are from a local--webkit=localrelease build.Also in this range (oven-sh/WebKit 54917009..e4f1d0d0)
Note
CI will fail until the
autobuild-preview-pr-337-e4f1d0d0artifacts finish building.Fixes #6360.
no test proof · iteration 4 · build/CI scripts only; test-proof not applicable