Repository navigation
AOT: objects and arrays in loops, vectors, subclasses, patterns in place, code from strings - #1040
Jarred-Sumner wants to merge 4 commits into
Conversation
…ace, code from strings On 7101dff. 830 files, +73,852 -6,221: the rest of aot/ 608 files, +62,980 -5,686; aot/tests 177 files, +7,694 -238; outside aot/ 45 files, +3,178 -297. Objects and classes - An object or array that each iteration of a loop allocates and that doesn't escape is carried through the loop as its fields, private fields included. int32 fields take four bytes, and short arrays, empty literals and some records take smaller cells. - A class that a function creates has code and a structure for each call in module code. Subclasses of Array, typed arrays, Map and Set are allocated from templates. Static fields are read as variables; accessors from Object.defineProperty(), defaults on prototypes and assigned prototypes keep their slots. - A Proxy trap written next to the Proxy is called directly, and Reflect.set() runs in a stub. Loops and numbers - Loops over typed arrays and arrays of numbers use 128-bit vectors. An element stored by one iteration reaches the next in a register. Arrays that only some iterations read are checked before the loop too. - push(), pop(), shift() and splice() of arrays in fields, in variables and behind methods are inlined. Module variables that hold typed arrays cache the address and length. - Remainders, products modulo n, shifts and carries stay integers. Strings and regular expressions - A string that a loop consumes is carried as a position in its base string. A loop over split() or matchAll() takes one piece at a time. A sticky pattern, or one held by a constant, a field or a private field, is matched in place. Calls - Callbacks of array methods run in one loop also where the method is in a function that the loop calls. A chain of then() calls keeps one object. Generators are stepped inside more consumers. Recursive functions check arrays and numbers once. Code from strings - The interpreter of strings that a program evaluates at run time (eval(), new Function(), node:vm) caches properties, globals and the names of a context, and calls getters and common built-ins directly. Builds that stop on main - A loop over a typed array under y - (y & ~7) with y of 2 ** 32 or more. arm64: more than 128 MB of compiled regular expressions. With `--aot-harden=proof-checks`: a built-in constructor passed as a callback; `a[0] - 1 + a[2] * x` in both arms of a branch, then `a[2] = 7`; a loop whose two arms store one parameter into an element. The remainder of a product of BigInts in two arms of a loop. Plain Bun too - concat() keeps the elements of a species constructor's result where the argument has holes. `Wire::schemaHash` changes, so this engine and Bun's side go together.
…engths in loops On 69e40c6. 31 files, +654 -88: the rest of aot/ 26 files, +396 -71; outside aot/ 4 files, +257 -16; aot/tests 1 file, +1 -1. - AOTObjectKeys.cpp includes the header that defines propertyNameEnumerator(). It compiled only next to a file that includes it. - A read of a name that no object of the loop has is answered inline after the first miss. A subclass that counts in a static field of its base class keeps the fast version of the loop. - Where shared memory may grow, the length of a typed array in a field is read before the loop. - `if (a % b)` compares the remainder without allocating the number. A counter whose value may exceed int32 steps as an integer. - A generator that iterates over another generator steps it on the main path. A generator's step that finishes resumes through one shared stub. - A regular expression created at run time is attempted only where the character before it and its leading lookaheads let a match begin. - Nested loops that add elements of any type are visited as often as before this pull request. - With `--aot-harden=proof-checks`, an element read under a string that spells an integer does not stop the build.
|
Preview build of 69e40c6: |
… many edges On 5a7ad89. 374 files, +5,889 -5,856: aot/tests 110 files, +3,548 -3,533; the rest of aot/ 240 files, +2,102 -2,085; outside aot/ 24 files, +239 -238. - Names and comments that this pull request adds are put into plain English, and its tests of the intermediate representation get shorter file names. 319 files, 2,331 lines; nothing else in them changes. - LLIntSlowPaths.cpp, AOTReactionChain.cpp and AOTPropertyCachesLeafOperations.cpp read a structure through WTF::opaque(), as the places beside them do. - Six files include what they use: they compiled only next to files that include it. - The link counts the names that a loop copies into a record once, after they are read, and the walk collects the edges of a body once for all of its loops. A loop with many `continue` took time quadratic in their number. - `if (!map.has(key)) map.set(key, value)` followed by `map.get(key)` looks an existing key up once, as on main. - A value that comes around a loop through a catch block keeps its type. - Tests: expectations follow the code this pull request emits. On Linux arm64, where blocks are 64 KB, the test of a string that grows in place makes enough additions.
![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 splice fast path's removed-elements array (tryCreateArrayWithStorage with WrittenByCaller, Source/JavaScriptCore/aot/runtime/AOTArrayOperations.cpp:744-766) can be observed by a concurrent marker with uninitialised slots — the cell is published before the caller fills it, but nothing in the heap references it yet and there is no safepoint between allocation and the copy, matching the contract of JSC's tryCreateUninitializedRestricted, so it is not affected. The other candidate concerns (Array.from detach handling, flatMap callable check, shift start-move preconditions, sort key-array reuse, Buffer.byteLength divergence) were examined and ruled out as well. This is a very large change (834 files, ~74k lines) touching a new AOT compiler, runtime, and machine encoders, so a human review of the overall design is still worthwhile.
Extended reasoning...
The diff adds or rewrites 834 files (about 74k insertions) across the JavaScriptCore AOT compiler: builtin emitters, IR schema, machine encoders for ARM64 and x86_64, object model, and runtime operations including array, RegExp, and property-cache paths. It touches no auth or injection surface, but it is memory-safety-sensitive engine code (butterfly manipulation, GC fences, hand-written allocation). Three confirmed findings (a null dereference on the in-line lastIndex store, a quadratic shift fallback, and a signed-shift overflow in the RegExp bit-parallel emitter) are posted inline; the size and the nature of the code rule out approval.
Findings marked 🟡 are optional suggestions and need no follow-up push.
On f21acba. 9 files, +71 -4: aot/tests 5 files, +60 -0; the rest of aot/ 4 files, +11 -4. - A store of `lastIndex` asks for an object before it reads the receiver's structure. - shift() of a long array that has named properties moves its start, as on main. A queue took time quadratic in its length. - AOTRegExpBitParallel.cpp shifts an unsigned one. - `this.size` of a subclass of Map, read in a loop that sets and deletes, is checked by structure. - Tests: four sources name 128-bit vectors in their switches.
| if (bits.isOfPropertyMap) { | ||
| JSObject* map = constructEmptyObject(globalObject); | ||
| map->putDirect(globalObject->vm(), asString(JSValue::decode(key))->toIdentifier(globalObject), literal); |
There was a problem hiding this comment.
🔴 Programs that call Object.defineProperties with a symbol key on an object the fast path refuses (a Proxy or host object) now get a wrong property or a crash instead of the symbol property. At AOTDefinitionOperations.cpp:500 the new isOfPropertyMap fallback does asString(JSValue::decode(key))->toIdentifier(...) on the raw key, but the key can be a Symbol (tryRepeatDefinition at :460 handles that case). Fix: when the key is not a string, keep the old ObjectDefineProperty call (or convert the key with toPropertyKey and putDirect under that Identifier) so every symbol and non-atom key defines the right property.
Why this was flagged
The walk sets PropertyDescriptorFlags::IsOfPropertyMap at AOTWalkWrittenDescriptors.cpp:750-751 for the per-key definition inside Object.defineProperties, whose keys come from ownKeys and may be symbols. At run time operationAOTDefineOwnProperty (AOTDefinitionOperations.cpp:474) reaches line 498 when tryDefineOwnPropertyFromBits returns false, for example because the target is a Proxy or another object for which definesWithoutOtherCode (:410-427) is false. Line 500 then calls asString() on a Symbol cell and toIdentifier on it: in release builds this reads the Symbol's PrivateName as a JSString fiber, so the map gets a string key made from the symbol description (or faults), and callInEngine ObjectDefineProperties defines that string property instead of the symbol property. The base branch passed the original key to Builtins::ObjectDefineProperty at the same site, which handled symbols correctly.
Verification: Triggered when a DefineOwnProperty carrying the IsOfPropertyMap flag has a Symbol key and the target refuses the fast path (a Proxy or host object). Source/JavaScriptCore/aot/runtime/AOTDefinitionOperations.cpp:500 applies asString to the raw key with no isString() test; line 460 accepts Symbol keys. In debug builds asString asserts; in release the Symbol cell is type-confused as a JSString.
| CellPointer viewAskedAnew = access->isMirroredView() ? viewOnSlowPath(access, view) : CellPointer(m_out.opaque(view.value())); | ||
| branchUnless(hasNoBits(loadByte(viewAskedAnew, m_heaps.JSArrayBufferView_mode), resizabilityAndAutoLengthMask), otherwise); | ||
| m_out.jump(outside); |
There was a problem hiding this comment.
🔴 Programs that keep a typed array over 2 GiB in a module variable read undefined and lose writes past the 2 GiB mark after merging; the base reads and writes the real element. The view mirror caps its length at (1 << 31) >> logElementSize (AOTStaticsEmitter.cpp:96-97, AOTInstance.cpp:1576), but askViewWherePositionIsNotBelowNamedLength at AOTElementAccessEmitterTypedArrays.cpp:117-118 treats "index not below the named length" on a fixed-length view as out of bounds, which is only true when the named length is the real one. Fix: for an isMirroredView() access whose index is not below the mirrored length, go to otherwise (the slow path on the real view) instead of outside/done.
Why this was flagged
A module-level variable holds a fixed-length typed array whose byte length exceeds 2^31, for example const big = new Float64Array(2**28 + 16); a function reads or writes big[i] with i >= 2^28, an int32 index. The mirror length is capped: AOTStaticsEmitter.cpp:96-97 stores min(length, (1 << 31) >> logElementSize) and AOTInstance.cpp:1576 does the same in refresh(). In generateGetTypedArrayElement the branch at AOTElementAccessEmitterTypedArrays.cpp:453 finds the index not below named.length and goes to isNotBelowNamedLength; askViewWherePositionIsNotBelowNamedLength (:116-118) only checks that the real view is fixed-length and then jumps to outside, which joins absentElement (undefined) at :464; generatePutTypedArrayElement does the same at :546-550 and jumps to done, dropping the store. On the base branch the named length came from GetTypedArrayLength (the real 64-bit m_length), so index < real length took the inline load. The proof checks in typedArrayElementsNamedBy (:104-109) do not catch this: belowOrEqual(named.length, lengthOf(asked)) holds for a capped length.
Verification: The mirror caps the length at AOTStaticsEmitter.cpp:96-97. askViewWherePositionIsNotBelowNamedLength (lines 113-119) never consults the view's real length; a fixed-length view lands in outside, undefined for reads (line 464) and done for stores (line 550). On the base, the named length came from GetTypedArrayLength (the real length). The checksBounds() assertions at lines 104-109 do not catch this.
The JavaScriptCore side of a round of work on Bun's ahead-of-time compiler (
bun build --compile --aotandbun test --aot, both experimental). Bun's side is a pull request in oven-sh/bun that pins this one's preview. This follows #1036.Merge order
This merges first; then Bun's pull request pins the merged commit and runs CI once more.
What's in it
839 files, +74,597 −6,300 against main.
aot/walk: the main passaot/facts: the linkaot/codegen,stubs,machine: code generationaot/runtime,builtins,objectaot/testsaot/aot/Outside
aot/Only compiled programs take these paths; plain Bun runs a few checks more and gives the same answers. The row marked Plain Bun too changes an answer.
llint/LowLevelInterpreterAOT.asm,llint/LLIntSlowPaths.cppand 9 moreinterpreter/Interpreter.cpp,interpreter/Interpreter.hand 1 moreruntime/ArrayPrototype.cppconcat()into the result of a species constructor. Plain Bun tooruntime/JSArray.cppruntime/RegExpCachedResult.cpp,runtime/RegExpCachedResult.hand 3 moreruntime/StringPrototype.cpp,runtime/StringSplitCache.hand 1 moreruntime/ObjectConstructor.cppruntime/JSArrayBufferView.cpp,runtime/JSArrayBufferView.hand 1 moreruntime/JSMicrotask.cpp,runtime/JSPromise.cppand 3 morethen()callsruntime/JSONObject.cppruntime/DatePrototype.cpp,runtime/DatePrototype.hand 1 moreyarr/YarrFlatProgram.h,yarr/YarrFlatProgramBuilder.cppand 5 moreassembler/MacroAssemblerARM64.h,assembler/X86Assembler.hruntime/RegExp.cppIf you don't use
--aotconcat()keeps the elements of a species constructor's result where the argument has holes.Changes
Builds that stop on main
y - (y & ~7), whereyis 2 ** 32 or morelist.map(Date))--aot-harden=proof-checksa[0] - 1 + a[2] * xin both arms of a branch, thena[2] = 7if (a[i] < 0) a[i] = x; else a[i] = xin a loop that then adds the next element and multiplies the stored one(a * b) % c, in two arms of a loopObjects and classes
Class.create(), also in TypeScript) has code and a structure for each call in module code.Object.defineProperty(), defaults on prototypes and assigned prototypes keep their slots.Reflect.set()runs in a stub.Loops and numbers
push(),pop(),shift()andsplice()of arrays in fields, in variables and behind methods are inlined. An array filled by a loop reserves its capacity.n, shifts and carries stay integers.Strings and regular expressions
split()ormatchAll()takes one piece at a time, without an array or result objects.Calls
sort()computes keys once where the comparator only reads them.then()calls keeps one object until something needs a promise of it.Code from strings
eval(),new Function(),node:vm) are interpreted in an executable. The interpreter now caches properties, globals and the names of a context, and calls getters and common built-ins directly.Build and checks
Known issues
Not on main:
--aot-harden=proof-checkstext["3"]) can stop the build.What else differs from
bun runandbun testis listed in Bun's pull request.Tests
IR tests and native tests are in
aot/tests; stored programs are in Bun's pull request. CI's run of this pull request is the first of this tree.