Repository navigation
[JSC] The slow path of a run-time-length array allocation takes the butterfly from the size class of its inline path - #767
Conversation
…utterfly from the size class of its inline path DFG and FTL allocate the butterfly of an array with a run-time length inline (SpeculativeJIT::emitAllocateButterfly, FTL allocateJSArray). They ask vm.auxiliarySpace() for sizeof(IndexingHeader) + 8 * length bytes. For a length of 0 or 1 that is size class 16. Their slow path, operationNewArrayWithSize, called JSArray::tryCreate. That applies the minimum capacity of the runtime: 5 elements (48 bytes) for length 0 and 3 elements (32 bytes) for length 1. So the slow path never ran LocalAllocator::allocateSlowCase on the allocator whose free list the inline path reads. Each collection empties that free list. After that, each allocation of such an array called the slow path again, until other code allocated a 16-byte auxiliary cell. Rest parameters with 0 or 1 arguments, new Array(n), slice and map of short arrays are on this path. operationNewArrayWithSizeAfterInlineAllocation is now the slow path of these inline attempts. With no butterfly and a contiguous structure, it takes the butterfly from the size class that the inline path asked for. Lengths of 2 and more get the same butterfly as before. The two DFG slow path generators for a run-time length call the operation by name. FTL allocateJSArray calls it when the vector length is not static. A static vector length already has the capacity of the runtime, so that arm keeps operationNewArrayWithSize. The inline code does not change. * JSTests/stress/runtime-length-array-slow-path-refills-inline-allocator.js: Added. * JSTests/microbenchmarks/runtime-length-small-array-then-grow.js: Added. * Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h: * Source/JavaScriptCore/dfg/DFGOperations.cpp: * Source/JavaScriptCore/dfg/DFGOperations.h: * Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp: * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
|
Status How this was reproduced, with plain JavaScript in function rest(...values) { return values; }
noInline(rest);
const keep = new Array(200000);
for (let i = 0; i < 200000; ++i)
keep[i] = rest(i);
One decision is open for a maintainer. It is in the Notes of the description ("For a maintainer to decide"). |
|
Preview build of d184a84: |
![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 two things in tryCreateArrayInSizeClassOfInlineAllocation and ruled them out: the raw butterfly held across JSArray::createWithButterfly is a conservative stack root, the same pattern JSArray::tryCreate already uses; and the length > MAX_STORAGE_VECTOR_LENGTH nullptr return is unreachable from the DFG/FTL callers with a contiguous structure, since they switch to the ArrayStorage structure at MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH before taking this slow path.
Extended reasoning...
The change adds a new DFG/FTL slow-path JIT operation in Source/JavaScriptCore/dfg/DFGOperations.cpp and rewires the two DFG slow-path generators and the FTL lazy slow path to it, plus a stress test and a microbenchmark; it touches allocator/GC-adjacent code but no auth, injection, or data-exposure surface. Verified findings on the test and on the capacity trade-off are posted inline, so a human review is already signalled; this note only records the two additional concerns that were examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| static ALWAYS_INLINE JSArray* tryCreateArrayInSizeClassOfInlineAllocation(VM& vm, Structure* structure, unsigned length) | ||
| { | ||
| ASSERT(!hasAnyArrayStorage(structure->indexingType())); | ||
| if (length > MAX_STORAGE_VECTOR_LENGTH) [[unlikely]] | ||
| return nullptr; | ||
|
|
||
| unsigned vectorLength = Butterfly::availableContiguousVectorLength(structure, length); |
There was a problem hiding this comment.
🟡 (optional) Programs that grow a rest-parameter or new Array(n) array right after creating it now pay a reallocation on every call, where the base often gave spare room. tryCreateArrayInSizeClassOfInlineAllocation at DFGOperations.cpp:2902 sizes a run-time length of 0 or 1 to one element, and the inline path it now keeps fed stores vectorLength == length, so the first push or unshift goes through JSObject::ensureLengthSlow. Fix: keep the slow path in the inline size class but give a run-time length of 0 or 1 the runtime minimum capacity on the inline path as well (one compare and conditional move in emitAllocateButterfly and FTL allocateJSArray), or state in the PR that the growth cost is accepted for Bun workloads.
Why this was flagged
A DFG or FTL compiled function with a rest parameter, new Array(n), Array(n), slice or map whose length is 0 or 1 at run time, followed by push or unshift. On the base branch the inline path in SpeculativeJIT::emitAllocateButterfly (DFGSpeculativeJIT.cpp:14315) finds its size-16 free list empty almost every time, so JSArray::tryCreate at DFGOperations.cpp:2882 served the array with vectorLength 5 (length 0) or 3 (length 1) via Butterfly::optimalContiguousVectorLength. After the change tryCreateArrayInSizeClassOfInlineAllocation (DFGOperations.cpp:2896-2910) uses Butterfly::availableContiguousVectorLength, giving vectorLength 1, and the refilled free list makes the inline path, which stores vectorLength == publicLength (DFGSpeculativeJIT.cpp:14330, FTLLowerDFGToB3.cpp:24295), serve the rest. Every later push or unshift beyond that one slot calls JSObject::ensureLengthSlow (JSObject.cpp:4119) and allocates a second butterfly. No guard limits this: the growth path is unchanged and nothing on the inline path applies BASE_CONTIGUOUS_VECTOR_LEN.
Verification: New helper at DFGOperations.cpp:2902 sizes the butterfly with Butterfly::availableContiguousVectorLength; for length 0 or 1 that is totalSize = 8 (+8) bytes -> MarkedSpace::optimalSizeFor -> 16-byte cell -> vectorLength = (16-8)/8 = 1 (ButterflyInlines.h:46-52). Base JSArray::tryCreate (JSArrayInlines.h:56) used optimalContiguousVectorLength.
There was a problem hiding this comment.
This is the decision that the description leaves to a maintainer (Downsides, and "For a maintainer to decide" in the Notes). The table there has both states of main: the growth cost is what main has today in a program where other code allocates 16-byte auxiliary cells, and it is new only where nothing does.
The minimum capacity on the inline path is option 3 there. It keeps 5 and 3 elements, adds the compare and the conditional move to each run-time-length allocation of each length, and keeps 64 and 48 bytes for an array of length 0 and 1 where this change has 32. I did not build it. I can do that as a follow-up if a maintainer picks it.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
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 adds a dedicated operation for array allocation after inline allocation and routes DFG and FTL slow paths through it. New stress-test and microbenchmark cases cover small arrays and storage distances. ChangesRuntime-length array allocation
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change makes the slow path for small run-time-length arrays refill the free list used by inline allocation. No concrete merge-blocking problem was found. The author documents a trade-off of reduced initial capacity for length 0 and 1 arrays, and maintainers should decide whether to accept it. 🚥 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 |
1 participant
Problem
operationNewArrayWithSizefor each array with a run-time length of 0 or 1: 199,901 calls for 200,000rest(x)arrays.8 + 8 * lengthbytes: size class 16. The slow path (JSArray::tryCreate,dfg/DFGOperations.cpp:2882) takes 48 or 32 bytes. So it never refills the free list that the inline path reads.Fix
operationNewArrayWithSizeAfterInlineAllocationis now the slow path of these inline attempts. It takes the butterfly from the size class of the inline request.JSTests/stress/runtime-length-array-slow-path-refills-inline-allocator.js(fails onmainin 4 of 4 modes). Self-reviewed: 6 concerns raised, 3 addressed.Background
Downsides
pushorunshiftright after creation now grows the array:rest(x)thenpushis 411 instructions, was 237 (Node 370).mainhas that cost already (395) when other code allocates 16-byte auxiliary cells.jsctext: +1,352 bytes.Notes
Where this comes from. No user reported it. It was measured on a socket.io echo during oven-sh/bun#44112: that change removed one
Buffer.from(string)for each websocket frame, and the echo then made 16.0 calls ofoperationNewArrayWithSizefor each message where it made 2.7. Upstream WebKitmainhas the same code in these functions.For a maintainer to decide. The change makes every program behave as
maindoes today when native code allocates a 16-byte auxiliary cell now and then (column 2 of the table below). A program in which nothing did that (column 1) had one C++ call for each short array and got room for 5 or 3 elements with it. Sorest()then 5 pushes goes from 268 to 994 instructions there (Node 346). Three answers are possible:vectorLength == lengthis what the inline path gives every run-time length today, and V8 does the same.JSObject::ensureLengthSlow,runtime/JSObject.cpp:3862). The first push on an empty inline array is a C++ call that only finds the one spare slot of its cell, the second one grows to 3 and the fourth to 7. That is a separate change: it also applies to length 2 and more, which has these costs onmainin both states.The cause.
SpeculativeJIT::emitAllocateButterfly(dfg/DFGSpeculativeJIT.cpp) and FTLallocateJSArray(LValue publicLength, LValue vectorLength, ...)(ftl/FTLLowerDFGToB3.cpp) askvm.auxiliarySpace()forsizeof(IndexingHeader) + 8 * lengthbytes and storevectorLength == publicLength. For a length of 0 or 1 that is the allocator of size class 16.JSArray::tryCreateappliesButterfly::optimalContiguousVectorLength:BASE_CONTIGUOUS_VECTOR_LEN_EMPTY(5 elements, 48 bytes) for length 0 andBASE_CONTIGUOUS_VECTOR_LEN(3 elements, 32 bytes) for length 1.LocalAllocator::allocateSlowCaseis the only code that gives an allocator a free list, and it ran on the allocator of size class 48 or 32. The end of each collection empties every free list (LocalAllocator::prepareForAllocation). If no C++ code had asked for 16 auxiliary bytes at all, the allocator did not exist and the JIT read a null pointer fromCompleteSubspace::m_allocatorForSizeStep. From length 2 on, both paths ask for the same size class. A length that the compiler knows was never affected: DFG turns it intoNewButterflyWithSize, whose slow path takes a byte size, and FTL applies the capacity of the runtime at compile time.Ways to a run-time length of 0 or 1: a rest parameter,
new Array(n),Array(n), a derived class ofArraywith a constructor that callssuper(n),slice,mapand the other builtins that use@newArrayWithSize, a literal with more than one spread. In Bun:EventEmitter.prototype.emit(type, ...args),process.nextTick(fn, ...args).The change.
operationNewArrayWithSizeAfterInlineAllocationhas the signature ofoperationNewArrayWithSize. With a butterfly it makes the cell. With no butterfly and an ArrayStorage structure (a length that is too large for the inline path) it callsJSArray::tryCreateas before. With no butterfly and a contiguous structure it allocatesButterfly::availableContiguousVectorLength(structure, length)elements: the size class of the inline request, with the vector length that fills the cell. For a length of 2 and more that is whatJSArray::tryCreategave.CallArrayAllocatorWithVariableSizeSlowPathGeneratorandCallArrayAllocatorWithVariableStructureVariableSizeSlowPathGeneratorno longer take the operation as an argument. A site that uses one of them cannot pair the inline butterfly with another operation.allocateJSArraychooses the operation in the lazy slow path: the new one when the vector length is the run-time length,operationNewArrayWithSizewhen the vector length is static (it then has the capacity of the runtime already).operationNewArrayWithSize,operationNewArrayWithSizeAndHint,runtime/andheap/are as they were. The two direct calls ofoperationNewArrayWithSize(a realm that is having a bad time) and the two calls with a butterfly (only the cell failed) stay.Method. x86-64 Linux.
jscandbun-profileare release builds (RelWithDebInfo, no LTO) of the base and of this change with one toolchain, unless a table says otherwise.--useConcurrentJIT=false(BUN_JSC_useConcurrentJIT=0). A count of calls is the hit count of a gdb breakpoint with a large ignore count. Instructions are single steps (PTRACE_SINGLESTEP) of the main thread between two native marker calls around N calls of a function that is not inlined, as the difference of two window sizes.perfandvalgrindare not available where this was built.Slow path calls for 200,000 arrays,
jsc(the three array operations together, the same with--useFTLJIT=false)new Array(n), n = 0new Array(n), n = 1new Array(n), n = 2rest()rest(x)rest(x, y)[x].slice()[x, y].slice()[x].map(f)[...one, ...none][x]literal, FTL / DFG[]literal, FTL / DFG398 is one call for each refill of a free list (the butterflies and the cells).
In Bun (
bun-profilewith this WebKit, base and change)new Array(n)1,000,000 times, n = 0 / 1 / 2rest()/rest(x)/rest(x, y), 200,000 times[x].slice()/[x].map(f), 200,000 timesemitter.emit("msg", 1), 200,000 timestsc --noEmiton 25 files ofsrc/js/node(1.5 MB)For the last two rows, the calls of
JSObject::ensureLengthSloware 40 and 40 (socket.io) and 5,820 and 8,292 (tsc), and the calls ofoperationArrayPushare 12 and 12, and 3,055 and 5,500.operationArrayUnshiftwas not counted in these runs. A slow allocation that goes away is about 150 instructions, so the echo gains about 400 instructions for each message, which is about 2% of what one message costs onmain. With oven-sh/bun#44112 the echo has 16 such calls for each message.Instructions for each call, arrays that do not grow (FTL /
--useFTLJIT=false)new Array(n), n = 0new Array(n), n = 1new Array(n), n = 2rest()rest(x)rest(x, y)[x].slice()The inline path is the same. DFG emits the same number of bytes for each node on both builds, before and after "(End Of Main Path)":
NewArrayWithSize289 + 181,CreateRest338 + 77,ArraySlice419 + 160,NewArray228 + 161 ([x]) and 224 + 80 ([]). With the free list of the base filled by a native allocation before each window, the smallest of 16 windows of 100 calls has the same instruction count on both builds for 7 of 7 shapes in DFG. In FTL it has the same count for 5 of 7 shapes in one run. The other two (rest(x, y),[x].slice()) move by 1 or 2 instructions for each call from run to run on each build, with the addresses that the code holds, and have the same set of values on both over 6 runs. A slow call that refills a free list at length 2 takes 1,328 instructions where it took 1,335 (from the entry of the operation to its return).Memory. Heap bytes for each array, cell and butterfly:
rest()64 to 32,rest(x)48 to 32,rest(x, y)48 and 48,new Array(0)64 to 32,new Array(1)48 to 32,new Array(2)48 and 48. Collections for 8,000,000 arrays that die at once:rest()15 to 7,rest(x)11 to 7,rest(x, y)11 and 11.Arrays that grow right after creation. Instructions for each call.
mainis the released build of4fde1587(LTO). Column 2 is that build withnew Uint8Array(buffer)once in 512 calls, which refills the free list of size class 16 (its cost is in the number). Node is 26.3.0 with inlining of the callee off.main, no such allocationmain, one in 512 callsrest()rest(x)rest(x, y)rest()thenpushrest(x)thenpushrest(x, y)thenpushrest()then 5 pushesrest(x)then 5 pushesrest(x, y)then 5 pushesrest()thenunshiftrest(x)thenunshiftrest(x, y)thenunshiftf(ev, ...args)withargs.unshift(ev), 0 / 1 / 2 arguments[x].slice()[x].slice()thenpush[x, y].slice()thenpushThe DFG fast path of
unshiftfor a length of 0 or 1 (compileArrayUnshift,dfg/DFGSpeculativeJIT64.cpp) needs a spare slot. An array from the inline path has none, onmaintoo.Size.
jsctext 43,993,539 to 43,994,891 bytes (+1,352).bun-profiletext +2,412 bytes. The new operation is 1,290 bytes. One more JIT operation.Tests.
JSTests/stress/runtime-length-array-slow-path-refills-inline-allocator.jsreads$vm.deltaBetweenButterfliesof arrays that one function makes one after the other. The most frequent distance is the cell size of the size class. It expects 16 for 11 ways to a run-time length of 0 or 1, 32 for 3 ways to a run-time length of 2, 32 for[x]and 48 for[], for the arrays that DFG code made and for those that FTL code made. Two of its four modes add--forceGCSlowPaths=true, which sends each allocation to the slow path. On the base, 22 of 32 checks fail with FTL and 11 of 16 without (new Array(0) in DFG: expected 16 but got 48). The released build of4fde1587fails the same way. Each mode runs in 16 to 59 ms.JSTests/microbenchmarks/runtime-length-small-array-then-grow.jshas the never-grown, one push, five pushes andunshiftforms at lengths 0, 1 and 2.Test bun-webkit-linux-amd64-ltoandTest bun-webkit-linux-arm64-ltorunrun-javascriptcore-tests(JSTests, LayoutTests/js, the PerformanceTests collections) against thejscof this change, and both pass.JSTests/stresswhose names have to do with arrays (new-array,rest-parameter,create-rest,array-slice,spread,array-species,vector-length,auxiliary-evacuation,having-a-bad-time,array-push,array-unshift) in every mode ofrun-jsc-stress-tests: 2,737 of 2,754 runs pass. The 17 that fail are the modes ofintl-having-a-bad-time.js. The local build has no ICU data, sonew Intl.DateTimeFormat("en")alone fails on it.Self-review. 6 concerns.
mainin the state where nothing else allocates 16-byte auxiliary cells, and two push rows end behind Node. Not addressed in code. It is the first Downsides bullet and the decision above.unshiftform was in no table and not in the microbenchmark. Addressed: both have it.main. Addressed: the table has both.main.operationNewArrayWithSize*operation, paired with the inline path by convention. Not taken. A slow path for the butterfly alone that takes the byte size (whatallocateButterflyhas) makes the pairing structural, and adds a second slow path to each site. The two generator classes and one FTL function now hold the pairing.allocateJSArrayasks to build it fromallocateButterflyandallocateJSArray(IndexingType, ...). Not taken here: it is the same larger change as 5.Not in this change.
x + ""with an empty operand that is known only at run time callsoperationMakeRope2each time: 199,901 calls for 200,000, and 398 with a non-empty operand. The inline path ofMakeRopedoes not take an empty string. No open PR has it.operationNewArrayWithSizeAndHintapplies the capacity of the runtime to its hint. FTL passes it static vector lengths only, which have that capacity already.