Repository navigation
Conversation
|
Preview build of 98c438b: |
e31304e to
172bb08
Compare
|
Design summary for a maintainer decision, since this overlaps #622 and replaces #337. The representation: a frozen, sealed or non-extensible array keeps its elements in the ArrayStorage vector and takes the SlowPutArrayStorage shape. Three Structure bits describe what the vector may not do: Overlap with #622: #337 is superseded: its gate widening and bulk seal / freeze of map entries are part of this commit, and its sparse-map |
172bb08 to
02ba7aa
Compare
…ArrayStorage vector Object.preventExtensions, Object.seal, Object.freeze and a read-only "length" all went through JSObject::enterDictionaryIndexingMode, which moved every element of the object into the SparseArrayValueMap (sparse mode, vector length 0). After that every a[i] read missed the JIT vector fast paths and did a hash lookup in C++, and the freeze itself cost two butterfly conversions, a map cell and one hash insert per element. Now enterDictionaryIndexingMode keeps the elements in the vector and only switches the object to the SlowPutArrayStorage shape. LLInt and the baseline JIT never store into a SlowPutArrayStorage vector inline, and DFG / FTL now check the structure before they do, so every write reaches the C++ paths. The attributes of the elements live on the Structure: vectorElementsAreNonConfigurable (Seal), vectorElementsAreReadOnly (Freeze) and arrayLengthIsReadOnly (Freeze and the new SetArrayLengthReadOnly transition, which replaces the LengthIsReadOnly flag of the sparse map). Entries of the sparse map keep their own attributes; elements only move into the map when a single index needs attributes of its own (Object.defineProperty on an element), and they carry the structure's attributes with them. A non-extensible object therefore never has the plain ArrayStorage shape (suggestedArrayStorageTransition), which is what lets the plain ArrayStorage paths stay as they were. The C++ vector writers and readers consult the bits: putByIndex, trySetIndexQuickly, putDirectIndex and the beyond-vector-length paths, deletePropertyByIndex, defineOwnIndexedProperty, getOwnPropertySlotByIndex, the prototype-chain put intercept, JSArray::setLength / pop / push and Array.prototype.reverse. An array with no elements (ArrayClass, such as Array.prototype) stays blank: the bits and isStructureExtensible() cover the later writes, so freezing Array.prototype keeps the array prototype chain watchpoint. A frozen object tells the objects that inherit from it about its read-only elements (notifyPresenceOfIndexedAccessors) when it becomes a prototype, not when it is frozen, so a frozen array that is no prototype keeps the builtin fast paths. The DFG folds a read of a frozen element from the vector as it folded one from the sparse map; JSObject::freeze fences the element stores before the structure store. ArrayAllocationProfile never records the SlowPutArrayStorage shape, so one frozen array does not make the arrays its allocation site creates next SlowPutArrayStorage. The baseline indexed load and "in" ICs accept the SlowPutArrayStorage shape, whose vector they already read. Object.isSealed / isFrozen answer from the structure for an object that JSObject::seal / freeze sealed or froze. Object.seal / Object.freeze take the JSObject::seal / freeze path for an exact JSArray and for a JSFinalObject with indexed properties instead of the generic per-index SetIntegrityLevel loop (from #337, which this supersedes); JSArray has no lazily reified own properties, so materializeLazyOwnProperties skips it. On the oven-sh/bun#44305 repro (300k arrays of 16 ints, x86_64 release jsc): freeze 970 ms -> 35-47 ms, reads from the frozen arrays 9.5x plain -> 1.0-1.2x plain. Supersedes #337. The ArrayClass handling and freeze-array-prototype.js come from #622. Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
02ba7aa to
2d9ab4f
Compare
…orage store, as clobberize does The in-bounds store takes a slow path that throws when the object is frozen; CFA asserts that the two agree.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (28)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 1 remain after this review. WalkthroughJavaScriptCore now records indexed-vector attributes and read-only array length in structure state. Runtime array and object operations, bytecode caches, and optimized JIT paths use that state. New stress tests and benchmarks cover frozen, sealed, and non-extensible arrays. ChangesArray integrity and indexed storage
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change keeps frozen, sealed and non-extensible array elements in the vector. No concrete merge-blocking defect was identified in the supplied context. Normal CI validation is still advisable. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, implementation summary, validation results, performance data, and limitations. However, it does not follow the required template because it lacks a Bugzilla URL, the required reviewer line, and the template-style changed-file and function list. Resolution Add the associated Bugzilla URL, include a "Reviewed by NOBODY (OOPS!)." line or the applicable reviewer information, and provide the required explanation followed by a list of changed paths and relevant functions. Ensure the PR is also linked from the associated Bugzilla issue as required by the repository template.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, two other spots were examined and ruled out: the compiler-thread read of a frozen vector in DFGAbstractInterpreterInlines.h (the structure flag is checked first, with a load-load fence paired against the store fence in JSObject::freeze, and the fold only applies when the structure already says read-only), and the new structure-answered Object.isFrozen / isSealed fast path in ObjectConstructor.cpp (it only short-circuits to true and still falls through to the generic TestIntegrityLevel loop otherwise).
Extended reasoning...
The change reworks how JSC stores elements of non-extensible, sealed and frozen objects (ArrayStorage vector instead of the sparse map) across the runtime, DFG, FTL and Repatch, with three new Structure bits and a new transition kind. It touches no injection, auth or data-exposure surface, but it changes JS-visible integrity-level semantics, and the hunt ended on the bug cap with several confirmed semantic regressions posted inline, so approval is not appropriate.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| case NonArray: | ||
| // No indexed properties to convert. Once the caller makes the structure | ||
| // non-extensible, indexingShouldBeSparse() lazily handles later indexed | ||
| // writes; staying blank also keeps for-in enumerator caching usable. | ||
| // JSArray code paths (e.g. setLengthWritable) assume this method | ||
| // allocated ArrayStorage, so do not skip for JSArray subclasses that | ||
| // use NonArray indexing (e.g. $vm RuntimeArray with DerivedArrayType). | ||
| if (!inherits<JSArray>()) [[likely]] | ||
| return; | ||
| [[fallthrough]]; | ||
| case ArrayClass: | ||
| // No elements to protect. The structure bits and isStructureExtensible() cover the later | ||
| // writes (putByIndexBeyondVectorLength, JSArray::push / pop / setLength), and staying blank | ||
| // keeps the for-in enumerator cache and the array prototype chain watchpoint usable. | ||
| return; |
There was a problem hiding this comment.
🔴 An empty array whose "length" was made non-writable now silently grows on a[0] = v or a.push(v), where the base throws a TypeError. JSObject.cpp:1218 leaves an ArrayClass array blank for JSArray::setLengthWritable, but the structure stays extensible, so indexingShouldBeSparse() at JSObject.cpp:3499 is false and the gate at JSObject.cpp:3503 that tests arrayLengthIsReadOnly() is skipped. The put reaches createInitialForValueAndSet at JSObject.cpp:3516, which allocates an Int32 butterfly with length 1; from then on no fast path (pushInline, setLength for Int32/Double/Contiguous) checks the read-only length. … [also at: Source/JavaScriptCore/runtime/JSObject.cpp:3516 - After merging, Array.prototype whose "length" was made non-writable can still gain elements and its length grows; the…; Source/JavaScriptCore/runtime/JSArray.cpp:1271 - After merging, Object.seal(Array.prototype) or Object.preventExtensions(Array.prototype) no longer stops new elements:…; +1 more]
Why this was flagged
…Fix: a blank object with arrayLengthIsReadOnly() must reject a put at or beyond length before any storage is allocated, on both putByIndexBeyondVectorLength (JSObject.cpp:3499) and putDirectIndexSlowOrBeyondVectorLength (JSObject.cpp:3676), e.g. test the bit outside the indexingShouldBeSparse() branch or give the array SlowPutArrayStorage in setLengthWritable as the…
Script: let a = Array.prototype; Object.defineProperty(a, "length", { writable: false }); a[0] = 1; (or a.push(1), strict or sloppy). JSArray::setLengthWritable (JSArray.cpp:203) calls enterDictionaryIndexingMode, which returns at JSObject.cpp:1218 for ArrayClass, then applies the SetArrayLengthReadOnly transition; the structure is still extensible.
Verification: normal (bounded: only Array.prototype, the sole ArrayClass object, is affected) — triggered when a script makes Array.prototype's "length" non-writable without also making the object non-extensible, then writes an index or pushes on it.
Mechanism verified, with the candidate's example corrected.
| // The read-only elements of a frozen object intercept the indexed puts of the objects that | ||
| // inherit from it. JSObject::freeze leaves this note to here so that a frozen array that is | ||
| // no prototype keeps its fast paths; nothing inherits from this object yet, so no bad time. | ||
| if (oldStructure->vectorElementsAreReadOnly() && hasIndexedElementsInArrayStorage()) [[unlikely]] { |
There was a problem hiding this comment.
🔴 Instances constructed with a frozen array as their direct prototype (Reflect.construct(Array, [], F) with F.prototype = Object.freeze([1,2,3])) can overwrite the frozen element: x[0] = 9 succeeds where the base branch threw TypeError. JSObject::freeze now defers notifyPresenceOfIndexedAccessors to didBecomePrototype, but StructureCache::emptyStructureForPrototypeFromBaseStructure (StructureCache.cpp:101) evaluates anyObjectInChainMayInterceptIndexedAccesses before createEmptyStructure calls didBecomePrototype, so the subclass structure keeps the plain Int32/Undecided shape. Fix: ensure the AddIndexedAccessors note is visible before any caller decides a child's indexing shape, or note read-only elements eagerly in freeze. [also at: Source/JavaScriptCore/runtime/JSObject.cpp:2958 - Arrays constructed with a frozen array as new.target.prototype can overwrite the frozen prototype's read-only elements in place, where the base rejects the write.; Source/JavaScriptCore/runtime/StructureCreateInlines.h:51 - An array constructed with a frozen array as its new.target prototype can overwrite the frozen prototype's read-only elements; the base throws a TypeError.]
Why this was flagged
Trigger: function F() {} F.prototype = Object.freeze([1, 2, 3]); let x = Reflect.construct(Array, [], F); "use strict"; x[0] = 9;. Object.freeze takes the new JSObject::freeze fast path; since the array is not yet a prototype, JSObject.cpp:2957 skips notifyPresenceOfIndexedAccessors, so its structure lacks MayHaveIndexedAccessors. StructureCache::emptyStructureForPrototypeFromBaseStructure tests prototype->anyObjectInChainMayInterceptIndexedAccesses() at StructureCache.cpp:101 before createEmptyStructure runs prototype->didBecomePrototype(vm), so the instance structure is created with the base ArrayWithUndecided shape. At runtime x[0] = 9 goes through JSObject::putByIndex and stores directly; trySetIndexQuickly and the contiguous paths never consult the prototype, so the write succeeds and x[0] reads 9. On the base branch, freezing the array ran defineOwnIndexedProperty with ReadOnly, which called notifyPresenceOfIndexedAccessors at freeze time, so the subclass structure was created with SlowPutArrayStorageShape and the put threw ReadonlyPropertyWriteError.
Verification: In JSObject::freeze (Source/JavaScriptCore/runtime/JSObject.cpp:2957-2958) notifyPresenceOfIndexedAccessors is only called if (mayBePrototype() && hasIndexedElementsInArrayStorage()). StructureCache::emptyStructureForPrototypeFromBaseStructure (StructureCache.cpp:98-105) evaluates prototype->anyObjectInChainMayInterceptIndexedAccesses() BEFORE calling createEmptyStructure, so indexingType stays ArrayWithUndecided.
| // Only a non-extensible object gets here with an index inside its vector: its elements | ||
| // stay in the vector with the attributes the structure records for them. | ||
| ASSERT(!isStructureExtensible()); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| WriteBarrier<Unknown>& valueSlot = storage->m_vector[i]; | ||
| if (valueSlot) { | ||
| if (structure()->vectorElementsAreReadOnly()) | ||
| return typeError(globalObject, scope, mode == PutDirectIndexShouldThrow, ReadonlyPropertyWriteError); | ||
| valueSlot.set(vm, this, value); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🔴 Builtins that must use CreateDataPropertyOrThrow silently overwrite elements of a sealed array instead of throwing, and the new in-vector branch at JSObject.cpp:3745-3755 is what accepts the write. The branch checks only vectorElementsAreReadOnly and then stores, never comparing the implied configurable:true descriptor against the non-configurable element. Array.from.call(() => sealed, [5, 6]) or a class field [0] = v on a constructor that returns a sealed array therefore mutate the sealed element; the spec and V8 throw a TypeError. Fix: in PutDirectIndexShouldThrow / ShouldNotThrow mode, reject the write (NonExtensibleObjectPropertyDefineError or an unconfigurable-change error) when the element is non-configurable, matching the descriptor validation the sparse-map path performs.
Why this was flagged
Trigger: s = Object.seal([1, 2]); Array.from.call(function () { return s; }, [5, 6]) (Array.from uses @ putByValDirect, strict builtin, so putDirectIndex with PutDirectIndexShouldThrow); or class A { constructor() { return Object.seal([1]); } } class B extends A { [0] = 9 } ; new B(). The JSObjectInlines.h:1601 fast check fails because the object is non-extensible, so control reaches putDirectIndexSlowOrBeyondVectorLength, ALL_ARRAY_STORAGE case: at JSObject.cpp:3749-3755 valueSlot exists, vectorElementsAreReadOnly is false for a sealed object, and the value is stored. CreateDataProperty supplies a descriptor with configurable:true; ValidateAndApplyPropertyDescriptor must return false for a non-configurable current property, so the spec result is a TypeError and the element stays 1. The base's SparseArrayValueMap::putDirect (SparseArrayValueMap.cpp:131-138) also accepted the write, but it also cleared DontDelete, and this PR rewrites the path with its own attribute model.
Verification: Object.seal([1,2]) now takes switchToSlowPutArrayStorage (JSObject.cpp:1227), so the elements stay in the vector with only vectorElementsAreNonConfigurable set on the structure. The new in-vector branch at JSObject.cpp:3745-3755 checks only vectorElementsAreReadOnly, so the write succeeds silently. Array.from.call(function(){ return Object.seal([1,2]); }, [5,6]) threw TypeError on the base and now returns [5,6].
| case ArrayWithSlowPutArrayStorage: | ||
| // The elements of a non-extensible object carry attributes (Structure::vectorElementAttributes). | ||
| return propertyName < butterfly()->vectorLength() && isStructureExtensible(); |
There was a problem hiding this comment.
🔴 An extensible array whose "length" is read-only can be grown by CreateDataProperty callers (Array.from, flat, map via Symbol.species) when spare vector capacity exists; the base throws before touching it. The putDirectIndex fast-path lambda at JSObjectInlines.h:1597 only requires isStructureExtensible() for SlowPutArrayStorage, so an index between length and vectorLength goes to setIndexQuickly, which stores and bumps length past the read-only value. Fix: for SlowPutArrayStorage the fast path must also refuse an index at or beyond storage length when structure()->arrayLengthIsReadOnly() (mode is not LikePutDirect there), so the slow path's ReadonlyPropertyWriteError at JSObject.cpp:3597 is reached.
Why this was flagged
Trigger: let a=[1,2,3]; a.push(4,5,6,7,8); a.length=3; Object.defineProperty(a,"length",{writable:false}); try { Array.from.call(function(){ return a; }, [0,0,0,0,0]); } catch {} a.length — Array.from's @ putByValDirect(result, k, v) reaches operationPutByValDirect (JITOperations.cpp:1696) → putDirectIndex with PutDirectIndexShouldThrow. The array is ArrayWithSlowPutArrayStorage, extensible, vectorLength 8, length 3. For k=3 the lambda at JSObjectInlines.h:1597 returns true, setIndexQuicklyForArrayStorageIndexingType stores and calls storage->setLength(4); same for k=4; the array ends with length 5 before the final Set("length") throws. Spec: CreateDataPropertyOrThrow on an array index >= length with non-writable length returns false and throws at k=3, leaving the array unchanged, which is what the base did (read-only length implied sparse mode with vectorLength 0, so the slow path hit map->lengthIsReadOnly()). The slow-path check at JSObject.cpp:3597 exists but is bypassed by the fast path.
Verification: normal — triggered when a JSArray whose "length" was made read-only via Object.defineProperty (not frozen/preventExtensions, so still extensible) has spare vector capacity and receives a CreateDataProperty-style put at an index >= length (Array.from / map / flat / filter result via new this() or Symbol.species, @ putByValDirect).
| { | ||
| if (!hasAnyArrayStorage(indexingType())) | ||
| return false; | ||
| const ArrayStorage* storage = butterfly()->arrayStorage(); | ||
| if (storage->m_numValuesInVector) | ||
| return true; |
There was a problem hiding this comment.
🟡 (optional) Every array in a realm can be pushed onto the SlowPutArrayStorage slow path after a frozen array with no elements becomes a prototype; the base does not do this. hasIndexedElementsInArrayStorage at JSObject.cpp:1248 trusts m_numValuesInVector, but enterDictionaryIndexingModeWhenArrayStorageAlreadyExists (JSObject.cpp:1191) copies that count into the new zero-length vector and never resets it, so an emptied map still counts as elements. JSObject::freeze (JSObject.cpp:2957) and didBecomePrototype (StructureCreateInlines.h:48) then call notifyPresenceOfIndexedAccessors, which is haveABadTime for the whole realm. Fix: zero m_numValuesInVector when the vector is emptied into the map (or count only vector slots that exist), so hasIndexedElementsInArrayStorage never reports phantom elements.
Why this was flagged
Trigger: a = [1,2,3]; Object.preventExtensions(a); Object.defineProperty(a, '0', {value: 1, enumerable: false}) moves the vector into the sparse map via enterDictionaryIndexingModeWhenArrayStorageAlreadyExists (JSObject.cpp:1167-1199); resizeArray at 1191 keeps m_numValuesInVector == 3 while vectorLength becomes 0. a.length = 0 (setLengthWithArrayStorage only touches map entries, usedVectorLength is 0) or delete a[i] empties the map; the stale count stays 3. Object.freeze(a) then Object.setPrototypeOf(o, a) or Object.create(a): didBecomePrototype (StructureCreateInlines.h:48) sees vectorElementsAreReadOnly and hasIndexedElementsInArrayStorage true (JSObject.cpp:1247-1248 returns on m_numValuesInVector), calls notifyPresenceOfIndexedAccessors, which transitions to AddIndexedAccessors and, once the object is a prototype, calls globalObject->haveABadTime (JSObject.cpp:1276). The same happens in JSObject::freeze at 2957 when the array is already a prototype. The base froze through the generic loop and only hit haveABadTime per ReadOnly index actually defined; with an empty map it defined none.
Verification: nit — triggered when an array that once entered sparse mode (e.g. Object.defineProperty on an index, or defineProperty after preventExtensions) is emptied (a.length = 0 / delete), then frozen and made a prototype (or frozen while already a prototype).
| // alone (JSObject::enterDictionaryIndexingMode). Only the header byte of lastArray is read: | ||
| // the array may be dead by now. | ||
| IndexingType lastArrayIndexingType = lastArray->indexingType(); | ||
| if (hasSlowPutArrayStorage(lastArrayIndexingType)) | ||
| lastArrayIndexingType = (lastArrayIndexingType & ~IndexingShapeMask) | ArrayStorageShape; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Code that freezes arrays from a hot literal or new Array site gets every later array from that site allocated as ArrayWithArrayStorage, losing Int32/Contiguous inline paths for arrays that are never frozen. ArrayAllocationProfile.cpp:68-70 maps a SlowPutArrayStorage lastArray to ArrayStorageShape and feeds it to leastUpperBoundOfIndexingTypes, so a single frozen array poisons the site's profile permanently. Freezing is now 25x cheaper and advertised as fast, so this runs at far higher rate than before. Fix: when lastArray is SlowPut because it is non-extensible (structure()->didPreventExtensions()), skip the upgrade and keep the site's current indexing type; only a genuinely bad-time array should upgrade.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Entry point: any allocation site whose most recent array (lastArray) was passed to Object.freeze / seal / preventExtensions before the next updateProfile call. ArrayAllocationProfile::updateProfile at ArrayAllocationProfile.cpp:66-70 reads lastArray->indexingType(), rewrites SlowPutArrayStorageShape to ArrayStorageShape, and computes the lub with the site's current type; the lub of Int32 and ArrayStorage is ArrayWithArrayStorage, so m_currentIndexingType becomes ArrayStorage. The profile is monotonic: it never goes back down. Every subsequent array created at that site, frozen or not, is ArrayWithArrayStorage and reads/writes through the ArrayStorage path instead of the Int32/Contiguous paths. The base froze arrays so slowly that the pattern was avoided; this change makes freeze cheap, so the population reaching this line grows by orders of magnitude while the new frozen-array-allocation-profile.js test only checks the frozen array itself. Remedy: in updateProfile, when lastArray's structure has didPreventExtensions(), leave the profile unchanged rather than upgrading to ArrayStorage.
Verification: pre-existing — the base already produces the identical profile outcome by the same route, so merging makes nothing worse. ArrayAllocationProfile.cpp:68-70 rewrites SlowPutArrayStorageShape to ArrayStorageShape and the profile only moves up. But base enterDictionaryIndexingMode (JSObject.cpp:1195-1221) calls ensureArrayStorageSlow, so the frozen array's indexingType was already ArrayWithArrayStorage at the base.
1 participant
Related to oven-sh/bun#44305 and oven-sh/bun#6360. Supersedes #337 (its gate widening and bulk attribute pass are part of this commit). Carries the ArrayClass handling and
freeze-array-prototype.jsfrom #622;arrayLengthIsReadOnlyhere is the generalisation of that PR'sdidFreezebit (it is also set byObject.defineProperty(a, "length", { writable: false })), so the two can merge in either order with a small conflict on Structure bit 10.Problem
Object.freezeon 300k arrays of 16 ints takes about 1 s in bun and 25 to 50 ms in node. Reads from the frozen arrays are 9 to 13x slower than from plain arrays (node: about 4x). The heap for those arrays grows from 53 MB to 172 MB.JSObject::enterDictionaryIndexingMode(runtime/JSObject.cpp) moves every element into theSparseArrayValueMap(sparse mode, vector length 0). Every producer goes through it:preventExtensions,seal,freezeandJSArray::setLengthWritable. After that eacha[i]read misses the JIT vector paths and does a hash lookup in C++.Fix
enterDictionaryIndexingModekeeps the elements in the ArrayStorage vector and only switches the object to theSlowPutArrayStorageshape. LLInt and baseline never store into such a vector inline, and DFG / FTL now test the structure before they do. So every write reaches C++.vectorElementsAreNonConfigurable(Seal),vectorElementsAreReadOnly(Freeze),arrayLengthIsReadOnly(Freeze and the newSetArrayLengthReadOnlytransition). They replace the map'sLengthIsReadOnlyflag. The C++ vector writers and readers consult them (putByIndex,trySetIndexQuickly,putDirectIndex,deletePropertyByIndex,defineOwnIndexedProperty,getOwnPropertySlotByIndex, the prototype-chain put intercept,JSArray::setLength/pop/push,Array.prototype.reverse).Object.definePropertyon an element) or when the object already owned a map; they carry the structure's attributes with them, so the existing map code validates the descriptor.ArrayClass, such asArray.prototype) stays blank, so freezingArray.prototypekeeps the array prototype chain watchpoint. A frozen object notes its read-only elements for the objects that inherit from it (notifyPresenceOfIndexedAccessors) when it becomes a prototype, not when it is frozen, so a frozen array that is no prototype keeps the builtin fast paths (slice,indexOf, spread).JSTests/stress/frozen-array-vector-resident.js(new, all tiers, lockdown, eager),frozen-array-allocation-profile.js(new),object-seal-freeze-array-fast-path.js(from Object.seal / Object.freeze: take the JSObject fast path for JSArray and JSFinalObject with indexed properties #337),freeze-array-prototype.js(from [JSC] Freezing built-in prototypes should not permanently disable fast paths #622), 6363 filtered stress runs, test262Object/{freeze,seal,preventExtensions,isFrozen,isSealed,defineProperty,...},Array/length,Array.prototypemutators,Reflect,Proxy(5514 pass, 3 pre-existing failures listed in expectations.yaml). The CI run of the first revision had one failure in the full suite (the allocation profile case, fixed byfrozen-array-allocation-profile.js).Background
ArrayStorageis the indexed storage with a vector plus an optionalSparseArrayValueMapfor attributed or far-away indices. "Sparse mode" means the vector is empty and the map holds every element.SlowPutArrayStorageis the shape JSC already uses when an indexed write cannot be done inline (indexed accessors on the prototype chain, "having a bad time"). Reads from its vector are inlined by every tier; the JITs never store into it without a C++ call, except the DFG / FTL in-bounds store, which now checksvectorElementsAreReadOnly.ArrayStorageshape (suggestedArrayStorageTransition), so the plainArrayStoragepaths are unchanged.Object.freezeonly, 22convertFromCopyOnWritecallers to audit), and a new indexing shape (spends the last free shape and twoArrayModesbits, touches 13 JIT files).Results (x86_64 release jsc, oven-sh/bun#44305 repro)
a[i]): 9.5x to 9.8x plain -> 0.9x to 1.2x plain. Builtins that read the array (slice,indexOf,join, spread) take their generic path for theSlowPutArrayStorageshape, as they do today for the sparse one.SparseArrayValueMapcells -> 0.size, 45,907,907 -> 45,922,123).Downsides
SlowPutArrayStorage, writable) pays one structure load and one branch more per store. PlainInt32/Double/Contiguous/ArrayStoragestores are unchanged.Object.definePropertyon one element of a sealed or frozen array still moves the whole array into the sparse map, as before. A sealed array that is then written througha[i] = vstays in the vector.Object.freezeof an array with elements that is already a prototype still callshaveABadTime, as defining a read-only index on it always did. Freezing it before it becomes a prototype no longer does.Fork-owned bits
TransitionKind::SetArrayLengthReadOnly = 18are taken by this change; upstream has them free at this base.Follow-ups noted
strings/rawarrays are built with per-indexputDirectIndex(ReadOnly | DontDelete)beforeobjectConstructorFreezeruns, so they are still sparse.Object.freezeonclass X extends Arrayinstances and on Proxies still runs the genericSetIntegrityLevelloop (the target array itself is vector-resident oncepreventExtensionsran).