Repository navigation
Conversation
https://bugs.webkit.org/show_bug.cgi?id=325781 Reviewed by NOBODY (OOPS!). Freezing an array turns it into ArrayStorage. After that, two profiles start to turn ordinary arrays into ArrayStorage, and those arrays miss most of the array fast paths. 1. ArrayMode::fromObserved picks ArrayStorage with Array::Convert for a site that has seen both Contiguous and ArrayStorage. Arrayify then converts every array that comes to the site. 2. ArrayAllocationProfile picks ArrayStorage for a site when the last array allocated there has become ArrayStorage. The array builtins share their profiles in a realm. Passing one frozen array to `every`, or freezing one result of `map`, affects every caller. This patch stops that. 1. ArrayMode::fromObserved ignores ArrayStorage when the site has seen Undecided, Int32, Double or Contiguous too. If an ArrayStorage array then fails the check in optimized code, ArrayProfile remembers it and the next compilation uses Array::Generic. 2. ArrayAllocationProfile ignores the last array when it has become ArrayStorage. The site keeps allocating arrays as before, and only the arrays that need ArrayStorage are converted later. 3. Arrayify exits with SparseIndex instead of Uncountable when the index is sparse, and ArrayMode::refine uses Array::Generic for the site in the next compilation. A site that writes to a sparse index did not reach this exit when Arrayify converted the array to ArrayStorage first. The other sites already kept exiting here. Both profiles still pick SlowPutArrayStorage as before. Every array has it when having a bad time. Some JetStream 3 tests hit this. Counts in one run, before -> after: Arrayify to ArrayStorage Allocated as ArrayStorage proxy-mobx 23306 -> 0 219027 -> 0 babel-wtb 53886 -> 0 21265 -> 0 jsdom-d3-startup 3011 -> 0 20834 -> 0 These tests were neutral when I ran them locally. Tests: JSTests/stress/array-allocation-profile-should-not-select-array-storage.js JSTests/stress/array-should-not-be-converted-to-array-storage-by-frequent-frozen-arrays.js JSTests/stress/array-should-not-be-converted-to-array-storage-by-frozen-array.js JSTests/stress/array-without-elements-should-not-be-converted-to-array-storage-by-frozen-array.js JSTests/stress/arrayify-should-not-keep-exiting-for-sparse-index.js * JSTests/stress/array-allocation-profile-should-not-select-array-storage.js: Added. (shouldBe): (createWithPush): (createWithLiteral): (createWithNewArray): (createWithMap): (createDoubleWithPush): (createInt32WithNewArray): (createInt32WithConstantLiteral): (createEmpty): (test): * JSTests/stress/array-should-not-be-converted-to-array-storage-by-frequent-frozen-arrays.js: Added. (shouldBe): (create): (createFrozen): (isObject): (every): (some): (filter): (forEach): (at): (indexed): (forOf): (destructure): (store): (push): (test): * JSTests/stress/array-should-not-be-converted-to-array-storage-by-frozen-array.js: Added. (shouldBe): (create): (createFrozen): (isObject): (every): (some): (filter): (forEach): (at): (indexed): (forOf): (destructure): (store): (push): (test): * JSTests/stress/array-without-elements-should-not-be-converted-to-array-storage-by-frozen-array.js: Added. (shouldBe): (shouldNotBeCompiledRepeatedly): (first): (indexed): (forOf): (destructure): (store): (push): (test): * JSTests/stress/arrayify-should-not-keep-exiting-for-sparse-index.js: Added. (shouldBe): (shouldNotBeCompiledRepeatedly): (putAfterOptimized): (putOncePerArray): (putToArrayBecomingSparse): (read): * Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp: (JSC::ArrayAllocationProfile::updateProfile): * Source/JavaScriptCore/bytecode/ArrayProfile.cpp: (JSC::ArrayProfile::computeUpdatedPrediction): * Source/JavaScriptCore/bytecode/ArrayProfile.h: (JSC::ArrayProfile::removeObservedArrayModes): (JSC::ArrayProfile::speculationFailedOnArrayStorage const): * Source/JavaScriptCore/bytecode/ExitKind.h: * Source/JavaScriptCore/dfg/DFGArrayMode.cpp: (JSC::DFG::ArrayMode::fromObserved): (JSC::DFG::ArrayMode::refine const): * Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h: * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::handleIteratorNext): * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::compileArrayify):
|
Status: draft. CI is red: Verified so far
How this was reproduced (Release function isObject(value) { return typeof value === "object"; }
function every(array) { return array.every(isObject); }
noInline(every);
let frozen = Object.freeze([{}, {}, {}]);
for (let i = 0; i < 10000; ++i) {
let array = [{}, {}, {}];
every(array);
if ($vm.indexingMode(array) !== "ArrayWithContiguous")
throw new Error("call " + i + ": " + $vm.indexingMode(array));
if (!(i % 100))
every(frozen);
}
|
|
Preview build of 76c588a: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (13)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe change updates JavaScriptCore array profiling and DFG handling for ArrayStorage and sparse indices. It adds stress tests for allocation-profile modes, frozen-array operations, empty arrays, and sparse-index reads and writes. ChangesArray indexing behavior
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No confirmed change-related issue blocks merge. The reported Linux LTO failure still needs identification before the draft is ready. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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, I also checked whether the new ExitKind::SparseIndex needs entries elsewhere: this fork's Source/JavaScriptCore/bytecode/ExitKind.cpp has no name/string table and exitKindMayJettison falls through to default: return true, so the new value is counted and jettisons without further table edits. I also checked that the fromObserved recursion in Source/JavaScriptCore/dfg/DFGArrayMode.cpp terminates and does not mutate the CodeBlock's profile: the profile is a by-value copy and the ArrayStorage bits are cleared before the single recursive call, so the new branch cannot be re-entered.
Extended reasoning...
The change is a cherry-pick touching JSC array profiling (ArrayProfile, ArrayAllocationProfile), DFG ArrayMode selection, the DFG and FTL Arrayify slow paths, a new ExitKind, and the for-of iterator-next lowering, plus five new stress tests; it touches no security-sensitive surface. Four verified findings are posted inline about performance-tier regressions (permanent Generic fallback and repeated recompiles), so a human should weigh those; this note only records the two extra checks that were ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| if (arrayMode.type() == Array::Generic) { | ||
| auto* data = m_graph.m_getByIdData.add(GetByIdData { CacheableIdentifier::createFromImmortalIdentifier(m_vm->propertyNames->length.impl()), CacheType::GetByIdSelf }); | ||
| length = addToGraph(GetById, OpInfo(data), OpInfo(SpecInt32Only), iteratedObject); | ||
| } else { | ||
| Node* butterfly = addToGraph(GetButterfly, iteratedObject); | ||
| length = addToGraph(GetArrayLength, OpInfo(arrayMode.asWord()), Edge(iteratedObject), Edge(butterfly, KnownStorageUse)); |
There was a problem hiding this comment.
🔴 After a single sparse-index exit inside a for-of loop, every later DFG/FTL compile of that function force-exits at the loop, so the loop never runs optimized again. handleIteratorNext (DFGByteCodeParser.cpp:12830) still emits GetArrayLength unless fromObserved returned Generic, but refine (DFGArrayMode.cpp:233) now makes that node Generic whenever a SparseIndex exit site exists at the same bytecode, and DFGFixupPhase.cpp:2834 turns a Generic GetArrayLength into ForceExit. Fix: take the GetById length path whenever the exit profile has SparseIndex at m_currentIndex (as is done for Generic), or exclude GetArrayLength from the SparseIndex rule in refine, so the recompiled loop runs generically instead of exiting.
Why this was flagged
Trigger: a for-of over a JSArray with at least MIN_SPARSE_ARRAY_INDEX (100000) elements whose op_iterator_next iterable profile saw two shapes (e.g. Int32 and Contiguous, giving Contiguous with Array::Convert), entered mid-loop by OSR entry at an index >= 100000 while the array still has the lesser shape; the Arrayify inserted for the GetByVal (DFGFixupPhase.cpp:4798, index edge at 4790) hits the index check and exits with SparseIndex (DFGArrayifySlowPathGenerator.h:63, FTLLowerDFGToB3.cpp:4437). Any later jettison records every compiled exit stub as a frequent exit site (CodeBlock.cpp:2903 and 3755) into the UnlinkedCodeBlock exit profile (DFGGraph.h:610), which persists. On the recompile the GetArrayLength at DFGByteCodeParser.cpp:12835 has the same origin.semantic as the GetByVal (both emitted at startIndex); its fixup calls refine (DFGFixupPhase.cpp:2832), which returns Array::Generic at DFGArrayMode.cpp:233; line 2834 maps that to ForceExit and blessArrayOperation (DFGFixupPhase.cpp:4910) inserts ForceOSRExit.
Verification: normal — triggered when a for-of site whose iterable profile saw two non-ArrayStorage shapes (e.g. Int32 + Contiguous, giving Contiguous/Array::Convert from DFGArrayMode.cpp:185-203) is entered mid-loop (baseline->DFG OSR entry, or an iterator advanced elsewhere) at an index >= MIN_SPARSE_ARRAY_INDEX while the array still has the lesser shape; after that the loop can never run optimized again in that function.
| case Array::Double: | ||
| case Array::Contiguous: | ||
| m_badPropertyJump = jit->speculationCheck(Uncountable, JSValueSource(), nullptr); | ||
| m_badPropertyJump = jit->speculationCheck(SparseIndex, JSValueSource(), nullptr); |
There was a problem hiding this comment.
🟡 (optional) Hot array stores can be permanently downgraded to the generic IC path after one in-bounds store that needed a storage conversion. DFGArrayifySlowPathGenerator.h:63 (and FTLLowerDFGToB3.cpp:4437) now exit with SparseIndex for any index >= MIN_SPARSE_ARRAY_INDEX on the Arrayify slow path, including an in-bounds index into a dense array of 100,000+ elements. One such exit plus any later jettison records the site, and DFGArrayMode.cpp:233 then returns Array::Generic for every later compile, with no recovery. Fix: raise SparseIndex only when the index is actually beyond the array's current length or vector (truly sparse), keeping Uncountable for in-bounds large indices, in both the DFG and FTL Arrayify paths.
Why this was flagged
Trigger: a DFG/FTL-compiled put_by_val (or get_by_val) site whose profile needs Array::Convert, reached with a dense array longer than 100000 whose first store of the new element type lands at an index >= 100000. The index is inside the butterfly vector, so the baseline store just converts Int32 to Double in place; nothing is sparse. In the Arrayify slow path the diff changes the check at DFGArrayifySlowPathGenerator.h:84-86 from an Uncountable exit to a SparseIndex exit without narrowing its condition. CodeBlock::tallyFrequentExitSites (CodeBlock.cpp:3755) adds every stub as a FrequentExitSite at the next jettison for any reason except old age or VM traps (CodeBlock.cpp:2902), into the UnlinkedCodeBlock exit profile, which is never cleared. ArrayMode::refine at DFGArrayMode.cpp:233 then returns Array::Generic in every subsequent compile, so all stores at the site go through the JITPutByValGenerator IC path. On the base branch the same event is an Uncountable exit that nothing consults, so the recompiled code keeps its fast path.
Verification: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:63 and :84-86 now exit with SparseIndex (was Uncountable). The compare is on the raw index, not against the vector length. DFGArrayMode.cpp:233-234 then returns Array::Generic for every later DFG/FTL compile of that function. On the base the identical check exits with Uncountable, which is not consulted by refine(), so the permanent Generic downgrade is new with this change.
| if (profile.speculationFailedOnArrayStorage()) | ||
| return ArrayMode(Array::Generic, nonArray, Array::AsIs, action).withProfile(profile, makeSafe); | ||
| profile.removeObservedArrayModes(arrayStorageModes); | ||
| ASSERT(!(profile.observedArrayModes() & arrayStorageModes)); | ||
| return fromObserved(profile, action, makeSafe); |
There was a problem hiding this comment.
🟡 (optional) Hot functions that read one array at several sites and sometimes see a frozen array now jettison and recompile once per site, up to K+1 DFG compiles with doubling thresholds, not 2 as on the base. DFGArrayMode.cpp:170-174 decides per ArrayProfile; only the exited site's profile gets the flag, and the AI folds later checks on the same base, so one site learns per compile. Fix: once one site fails on ArrayStorage, make every profile in that CodeBlock that observed ArrayStorage with other shapes Generic on the next compile, e.g. propagate SpeculationFailedOnArrayStorage to sibling profiles at jettison. The PR notes this downside.
Why this was flagged
A function reads a[0] .. a[K-1] from the same base and is called with ordinary Contiguous arrays plus an occasional Object.freeze'd one, so each get_by_val's ArrayProfile observes ArrayWithContiguous | ArrayWithArrayStorage. DFGArrayMode.cpp:165-174 strips ArrayStorage from each profile that lacks SpeculationFailedOnArrayStorage and returns Contiguous; DFGAbstractInterpreterInlines.h:4814 (alreadyChecked) lets constant folding remove every check after the first on the same value. When a frozen array arrives only the first site's CheckArray exits; DFGOSRExit.cpp:436-476 stores the failure structure only in that site's profile and ArrayProfile.cpp:132-133 sets the flag only there. The next compile makes that one site Generic, the next site now carries the live check and exits in turn, so K+1 DFG compiles are needed. Each jettison runs countReoptimization (CodeBlock.cpp:3070); the optimize threshold is shifted by 2^counter (CodeBlock.cpp:3197), so the function spends about 1000*2^(K+1) calls in baseline. The base branch returned ArrayStorage+Convert for every site (DFGArrayMode.cpp:181-203) and settled in 2 compiles.
Verification: The PR description's "Downsides" states "K of them in one function need up to K + 2 DFG compiles". Source/JavaScriptCore/dfg/DFGArrayMode.cpp:165-174 decides per ArrayProfile; the OSR exit stub writes the failure structure only to the exiting site's profile (dfg/DFGOSRExit.cpp:446-448,476). On the base, the same profile hits shouldUseFastArrayStorage at line 183, no exits, 2 compiles.
| if (auto structureID = std::exchange(m_speculationFailureStructureID, StructureID())) { | ||
| Structure* structure = structureID.decode(); | ||
| if (hasArrayStorage(structure->indexingType())) | ||
| m_arrayProfileFlags.add(ArrayProfileFlag::SpeculationFailedOnArrayStorage); |
There was a problem hiding this comment.
🟡 (optional) Sites that mostly read frozen arrays and rarely see an ordinary one lose their ArrayStorage fast path for good after merging; every read there goes through the Generic IC (the PR's own numbers: 33 ns vs 22 ns). ArrayProfile.cpp:133 sets SpeculationFailedOnArrayStorage after a single BadIndexingType exit on an ArrayStorage array, and UnlinkedArrayProfile::update (ArrayProfile.h:292-295) copies it into the unlinked profile, so every later CodeBlock of that function starts Generic at DFGArrayMode.cpp:171 and nothing ever clears it. Fix: make the Generic fallback proportional, e.g. only after repeated ArrayStorage failures, and clear or age the flag rather than persisting it in UnlinkedArrayProfile.
Why this was flagged
Trigger: a hot read or write site whose arrays are almost all frozen or sealed (ArrayStorage), plus one ordinary array passing through once. On the base branch observed = ArrayWithContiguous|ArrayWithArrayStorage gives ArrayMode(ArrayStorage, Convert): the one ordinary array is converted and the frozen majority keeps a CheckArray plus direct vector access. With the change DFGArrayMode.cpp:172-174 strips the ArrayStorage bits and compiles Contiguous Convert; the first frozen array hits the Arrayify slow path, fails to convert, exits BadIndexingType and stores its structure in m_speculationFailureStructureID. The next computeUpdatedPrediction (ArrayProfile.cpp:130-135) sets SpeculationFailedOnArrayStorage; DFGArrayMode.cpp:170-171 then returns Array::Generic. UnlinkedArrayProfile::update at ArrayProfile.h:292-295 propagates the flag into the UnlinkedCodeBlock, so every CodeBlock later linked from it, for every closure of the function, is Generic from the first compile; ArrayProfile::clear is never called on that path.
Verification: The PR description lists under "Downsides" that "A site that often gets both kinds is generic: with half its arrays frozen, a read takes 33 ns, was 22 ns"; the bound is understated, because the code needs only ONE ordinary array observation and ONE exit, and the resulting state is permanent and shared across all CodeBlocks of the function. Nothing removes the flag (only ArrayProfile::clear(), ArrayProfile.h:221-227).
2 participants
Cherry-pick of WebKit#75376 (
67fc02ae76dc, https://bugs.webkit.org/show_bug.cgi?id=325781), open upstream, not reviewed yet. It applies cleanly. Author and message are unchanged.Problem
ArrayStorage: 1,897 to 2,000 of 2,000 arrays that pass througheveryafter it saw frozen arrays.ArrayMode::fromObserved(dfg/DFGArrayMode.cpp:168) picksArrayStoragewithArray::Convertfor a site that saw both shapes.ArrayAllocationProfile::updateProfile(bytecode/ArrayAllocationProfile.cpp:63) does it for allocation sites.Fix
fromObservedignoresArrayStoragenext to Undecided, Int32, Double or Contiguous. If optimized code then fails on anArrayStoragearray, the site becomesArray::Generic.updateProfileignores a last array that hasArrayStorage.Arrayifyexits with the new counted kindSparseIndex, so a sparse store site becomes generic and stops exiting.JSTests/stresstests (49 of 50 runs fail onmain, 50 pass), and 1,726 runs of related tests.Background
ArrayStorageis the shape of a frozen, sealed or sparse array. It has fewer JIT and builtin fast paths.ArrayProfilerecords the shapes one site saw.fromObservedmakes the DFG'sArrayModefrom it.Array::Convertconverts the array in place.Downsides
main: 2), about 1,024 × 2^K calls.Notes
The port
git applyof the upstream patch onmain(7012d42e55): no conflict, offsets of 218 lines (DFGByteCodeParser.cpp) and 34 lines (FTLLowerDFGToB3.cpp). The added and removed lines are the same as upstream's.ArrayProfile.hdiffers in one#include. InDFGByteCodeParser.cpp,handleIteratorOpenandhandleIteratorNextare identical to upstream's. The fork's own changes to that file and toFTLLowerDFGToB3.cppare elsewhere.UnlinkedArrayProfilehere (CodeBlock::updateAllArrayProfilePredictions). So the newSpeculationFailedOnArrayStorageflag of a builtin is lost when itsCodeBlockgoes away. The site then learns it again with one more compile. Probe: six rounds of$vm.deleteAllCodeWhenIdle()between 10,000 calls each. DFG compiles per round stay at 2 (everycaller), 2 to 3 (index loop) and 1 (for...of), and no ordinary array changes shape.ArrayProfileof a call site lives inCallSiteDatahere.getArrayModeand the OSR exit stubs reach it throughCodeBlock::getArrayProfileas before.BufferAccessorIntrinsic(fork only) reads two flags from its array mode and builds its own mode. It does not emitArrayify.Reproduce (
jsc --useDollarVM=1 --useConcurrentJIT=0)main:Error: call 5198: ArrayWithArrayStorage. With the change: no error.function create() { return [{}, {}, {}]; }, thenObject.freeze(create()); fullGC();. Onmainthe nextcreate()returnsArrayWithArrayStorage. With the change it returnsArrayWithContiguous.Measurements (x64, Release
jsc: thebun-webkit-linux-amd64build ofmainand a local build of this branch with the same flags. Five interleaved runs each, median, range in parentheses.)maineveryafter frozen ones: arrays left asArrayStorageArrayStorageObject.freeze([i, i + 1, i + 2, i + 3])at one site: ns per arraya = []; a[0] = i; a[100008] = iat one site: ns per arraya[0]toa[K-1], half of its arrays frozen (--useConcurrentJIT=0, counts are the same on each run):mainThe DFG removes the array check of the later reads because the first read already checked. So only the first speculating site fails in each compile, and the reoptimization thresholds double each time. On
mainthe first site converts every ordinary array and nothing exits.What it adds for code that never sees
ArrayStoragehasExitSitelookup for each array access node (ArrayMode::refine).updateProfile. One branch incomputeUpdatedPredictionwhen a speculation failure was recorded.ExitKindvalue and oneArrayProfileFlagbit.sizeof(ArrayProfile)stays 16.Tests run
run-javascriptcore-testsin all modes:main49 of 50 runs fail (the run that passes isarray-allocation-profile-should-not-select-array-storage.js.ftl-no-cjit-small-pool), this branch 0 of 50.run-javascriptcore-tests --filter 'array-storage|arraystorage|arrayify|frozen|freeze|seal|sparse|prevent-extensions|preventExtensions|array-profile|arrayprofile|allocation-profile|indexing': 1,726 runs, 0 failures.Related
SlowPutArrayStorageshape and edits the lines after the one this change edits inupdateProfile. This change leavesSlowPutArrayStoragealone, as upstream does. From a read of that diff: with both merged,fromObservedwould pickSlowPutArrayStoragewithArray::Convertagain for a site that saw a frozen array. That needs a follow-up in whichever lands second.