perf(hir): #6812 — fold straight-line builder sequences into object literals#6829
Conversation
📝 WalkthroughWalkthroughThe PR adds HIR preprocessing that folds eligible object-builder assignments into literals, and expands object-array write fast paths to support multiplication and inner bounds derived from array length. Runtime guard handling and performance documentation are also updated. ChangesBuilder folding
Object-array write fast path
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant LoopMatcher
participant Codegen
participant RuntimeGuard
participant Array
LoopMatcher->>Codegen: match multiplication and dynamic-length loop
Codegen->>RuntimeGuard: pass dynamic-bound sentinel
RuntimeGuard->>Array: validate array and load length
Array-->>Codegen: return runtime inner bound
Codegen->>Codegen: execute fast loop and finalize counters
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Evidence per the #6812 protocol (release build of this branch, default pipeline, node v26.3.0, macOS arm64): 15-run alternated medians, builder cell (
Full 18-cell matrix regression sweep on this branch: no regressions — every previously-winning cell still beats node (three-writes 0.70×, one-write 0.71×, stable-dynkey 0.75×, class-inst 0.83×, canonical 0.86×); all checksums identical. Gap suite: 98.2% (387 pass), 0 compile failures, 0 crashes, 0 skipped, only the 7 known-triaged mismatches — gate OK ( Suites: perry-hir 210 ✓, perry-codegen ✓, perry-runtime 1462 ✓ (single-threaded). Semantic sanity (byte-diff vs node): identical on all fold-relevant cases — side-effect ordering, self-reference blocking, accessor-literal protection, mid-build exception, duplicate-key overwrite, folds inside arrows/class methods/loops, string-literal vs dynamic computed keys, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-hir/src/lower/builder_fold.rs`:
- Around line 45-53: Replace the hard-coded `module_contains_function_expr`
result with a lightweight recursive walk that returns true when any
`Expr::Arrow` or `Expr::Fn` appears anywhere in the module. Keep
`module_has_candidate` combining this check with the existing `Scan` result, so
modules are cloned and processed only when a direct candidate or nested function
expression is present.
- Around line 493-502: Extend walk_decl to recurse through ast::Decl::Var
initializers, including closures nested in variable declarators, alongside the
existing function and class handling. Update walk_class to process
ClassMember::ClassProp and PrivateProp initializer expressions instead of
skipping them, so closures such as arrow or function expressions in fields are
traversed by fold_stmts. Preserve the existing behavior for declarations and
class members without initializers.
- Around line 14-23: Update expr_references_ident and the Call/New/OptChain-call
handling in the folding logic to detect identifiers captured by callable
expressions, including functions referenced through assignment values such as
o.self = get(). Prevent folding when evaluating the value could invoke code
before the object binding is initialized, or otherwise ensure the transformed
expression remains TDZ-safe; revise the nearby soundness argument to reflect
this constraint.
In `@docs/object-write-matrix.md`:
- Around line 4-5: Update the benchmark evidence in object-write-matrix.md to
use Node v26.5.0, re-run the object-write matrix under the required
release/default pipeline, and replace the commit reference, medians, ratios, and
raw-sample reference with results from that run.
- Line 51: Update the w15_append_build row in the object-write performance
matrix with the Node 26.5.0 rerun results, reflecting the optimized fresh-object
builder performance of approximately 196 ms and 25× gap; alternatively label the
existing 1489 ms and 186× figures explicitly as a pre-optimization baseline, and
remove the stale “TOP GAP” assessment from the current matrix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77463468-f70c-47bd-9ac6-3c079c889e14
📒 Files selected for processing (5)
changelog.d/6812-builder-fold.mdcrates/perry-hir/src/lower/builder_fold.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/mod.rsdocs/object-write-matrix.md
| `main` @ `8bda0351e`; numbers are medians of 3 alternated runs, node v26.3.0, | ||
| macOS arm64, release + default pipeline, checksums identical on every cell |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)(Cargo\.toml|CLAUDE\.md|\.node-version|docs/object-write-matrix\.md|changelog\.d/.*\.md)$' || true
echo
echo "== docs/object-write-matrix.md lines 1-80 =="
if [ -f docs/object-write-matrix.md ]; then
sed -n '1,80p' docs/object-write-matrix.md | nl -ba
fi
echo
echo "== node version files =="
for f in .node-version .nvmrc .tools-versions; do
if [ -f "$f" ]; then
echo "-- $f --"
cat -n "$f"
fi
done
echo
echo "== CLAUDE version lines =="
if [ -f CLAUDE.md ]; then
rg -n -C 2 '(^|[\s])Current Version|^\d+\.\d+\.\d+' CLAUDE.md || true
fi
echo
echo "== Cargo workspace version =="
if [ -f Cargo.toml ]; then
rg -n -C 2 '^\[workspace\.package\]|\bversion\s*=' Cargo.toml || true
fiRepository: PerryTS/perry
Length of output: 4714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/object-write-matrix.md lines 1-90 =="
if [ -f docs/object-write-matrix.md ]; then
sed -n '1,90p' docs/object-write-matrix.md
fi
echo
echo "== node-version and .tool files =="
for f in .node-version .nvmrc .tools-versions; do
if [ -f "$f" ]; then
echo "-- $f --"
cat "$f"
fi
done
echo
echo "== CLAUDE version lines =="
if [ -f CLAUDE.md ]; then
sed -n '/Current Version/,$p' CLAUDE.md | sed -n '1,20p'
fi
echo
echo "== Cargo workspace version =="
if [ -f Cargo.toml ]; then
rg -n -C 2 '^\[workspace\.package\]|\bversion\s*=' Cargo.toml | sed -n '1,20p'
fi
echo
echo "== relevant benchmark/runtime references =="
rg -n "Node|node v|26\.5\.0|26\.3\.0|parity|gap|bench|write-matrix|object-write|w15_append_build|builder.fold" docs README.md CLAUDE.md .github 2>/dev/null | sed -n '1,120p' || trueRepository: PerryTS/perry
Length of output: 28147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check git command availability and repo ref =="
command -v git || true
git rev-parse --short HEAD 2>/dev/null || true
git rev-parse HEAD 2>/dev/null || true
echo
echo "== current docs/object-write-matrix.md commit/ref line exact =="
head -5 docs/object-write-matrix.mdRepository: PerryTS/perry
Length of output: 595
Update the object-write matrix to the pinned Node runtime.
The matrix cites Node v26.3.0, but this repo requires Node 26.5.0 for parity/gap testing. Re-run the benchmark and update the commit ref, medians, ratios, and raw-sample reference so the evidence matches CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/object-write-matrix.md` around lines 4 - 5, Update the benchmark
evidence in object-write-matrix.md to use Node v26.5.0, re-run the object-write
matrix under the required release/default pipeline, and replace the commit
reference, medians, ratios, and raw-sample reference with results from that run.
Source: Coding guidelines
…RHS, let alias, while form, dynamic bounds) (#6830) * perf(codegen): #6812 — widen whole-loop clone eligibility: Mul RHS, let-alias, inner while form * fix(codegen): #6812 — Mul widening landed on the wrong matcher; index-offset matcher must stay Add|Sub The blind text replace widened packed_f64_range_loop_index_offset's guard instead of match_object_array_write_number's — which would have treated an `i * c` index as offset `c` (a wrong-index miscompile) in the packed range-loop family. Reverted there; Mul now admitted where intended, with the endpoint-product range analysis and fmul emission it was built for. * docs(changelog): changeset for clone-eligibility widening * perf(codegen,runtime): #6812 — dynamic array-length inner bounds for the versioned write clone `for (let i = 0; i < arr.length; i++)` over the same array the loop writes qualifies: the guard receives a u32::MAX sentinel and resolves the scan length from the validated array's header (rejecting >16M), and the emitter loads the length register in a new guard-ok fast.entry block — the fast nest can never outrun the proven prefix. Length is loop-invariant by construction (the matched body admits only element-field stores). The 16M cap serves as the finite-range ceiling. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
All five findings addressed on the branch (commit
Full battery (11-case sanity byte-diff, 18-cell matrix sweep, hir/codegen/runtime suites, gap gate) re-run on the fixed tip; results in the follow-up comment. |
…bit critical) A call/new/member-read/spread/in/instanceof in a folded value can reach the builder binding through a closure or trap WITHOUT naming it (hoisted fn, getter), turning the original's post-init evaluation into a TDZ ReferenceError. The textual reference check is replaced by a strict whitelist of expressions that cannot execute user code. Also: the candidate pre-scan is now a real recursive scan (the always-true kludge gated nothing), and the mutating walk reaches var-initializer function expressions and class field initializers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-hir/src/lower/builder_fold.rs`:
- Line 289: Update value_is_fold_safe, used by the fold_key_ok guard, to reject
unary -, +, and ~, string concatenation/equality or inequality, short-circuit
logical operators, binary **, and template interpolations that may invoke user
coercion before builder initialization. For E::Tpl, permit only trap-free
interpolations and do not recursively approve expressions via
value_is_fold_safe; preserve safe operator cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ad4f0be-2c07-415a-aab1-9d53a7d9a2fd
📒 Files selected for processing (2)
crates/perry-hir/src/lower/builder_fold.rsdocs/object-write-matrix.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/object-write-matrix.md
| let Some((key, value)) = assign_to_name_key(fs, &name_start) else { | ||
| break; | ||
| }; | ||
| if !fold_key_ok(&key, &keys) || !value_is_fold_safe(value, &name_start) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files around target:"
git ls-files | rg 'crates/perry-hir/src/lower/builder_fold.rs|Cargo.toml|CLAUDE.md|changelog.d' || true
echo
echo "Target file outline:"
ast-grep outline crates/perry-hir/src/lower/builder_fold.rs --view compact 2>/dev/null | head -200 || true
echo
echo "Relevant source lines:"
sed -n '250,345p;445,545p' crates/perry-hir/src/lower/builder_fold.rs | cat -n
echo
echo "Search for ast expression variants:"
rg -n "enum .*Expr|Expr::Bin|UnaryOp|BinaryOp|Tpl|Unary|Bin|Tpl" crates/perry-hir/src -g '*.rs' | head -200Repository: PerryTS/perry
Length of output: 33474
🌐 Web query:
ECMAScript specification + operator ToPrimitive Temporal Numeric ToNumeric Unary - + ~ template literal ToString ToPrimitive OrdinaryToPrimitive
💡 Result:
In the ECMAScript specification (ECMA-262), these terms refer to core abstract operations and language semantics used to handle type conversion and operator evaluation [1][2]. Abstract Operations ToPrimitive(input [, preferredType]): Converts an object to a primitive value [1][3]. If input is an object, it checks for a @@toPrimitive (Symbol.toPrimitive) method [3][4]. If missing, it falls back to OrdinaryToPrimitive, which typically calls toString or valueOf depending on the preferredType hint [1][3]. ToNumeric(value): Used to convert a value to either a Number or a BigInt [1][3]. It performs ToPrimitive(value, number), then if the result is a BigInt, returns it; otherwise, it calls ToNumber [1][5]. ToString(argument): Converts an argument to a String value [4]. OrdinaryToPrimitive(O, hint): A standard method for converting an object to a primitive when no Symbol.toPrimitive method is defined [1][3]. It invokes methods on the object (typically valueOf or toString) based on the hint [1]. Unary Operators Unary +: The unary plus operator evaluates its operand and returns the result of ToNumber(value) [2]. Unary -: The unary minus operator converts its operand to a numeric value (via ToNumeric, though specific handling depends on the type) and then negates the result [2][4]. Bitwise NOT (~): Operates on the result of ToNumeric or ToInt32 conversions (depending on context), performing bitwise inversion. Template Literals: Template literals involve evaluation that includes converting embedded expressions to strings, often invoking ToString [6]. These mechanisms ensure that ECMAScript operators behave predictably by coercing diverse input types into the specific primitives (Number, BigInt, or String) required for their respective operations [1][2].
Citations:
- 1: https://tc39.es/ecma262/multipage/abstract-operations.html
- 2: https://tc39.es/ecma262/2025/multipage/ecmascript-language-expressions.html
- 3: https://tc39.es/ecma262/2026/multipage/abstract-operations.html
- 4: https://tc39.es/ecma262/2026/multipage/ecmascript-data-types-and-values.html
- 5: https://tc39.es/ecma262/2025/multipage/abstract-operations.html
- 6: https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html
🌐 Web query:
ECMAScript 2026 binary Addition operator evaluation valueOf toString Symbol.toPrimitive spec 13.13.7
💡 Result:
In the ECMAScript 2026 specification, the addition operator (+) is defined under the Additive Operators section (Section 13.8.1) [1][2]. The runtime evaluation of the binary addition operator relies on the abstract operation EvaluateStringOrNumericBinaryExpression, which determines whether to perform string concatenation or numeric addition [3][4][2]. When the addition operator is applied to objects, they must be converted to primitives [5]. The conversion process follows the ToPrimitive abstract operation (Section 7.1.1) [6][7]. The order and priority of primitive conversion methods are as follows: 1. Symbol.toPrimitive: If an object has a [Symbol.toPrimitive] method, it is called first with a hint ("number", "string", or "default") [6][8][9]. 2. valueOf and toString: If [Symbol.toPrimitive] is not present, the engine falls back to calling valueOf and toString [8][9]. The order depends on the preferred hint [9]: - For a "string" hint: toString is called first, followed by valueOf if necessary [9]. - For "number" or "default" hints: valueOf is called first, followed by toString if necessary [9]. For the binary addition operator (+), the preferred type hint passed to ToPrimitive is "default" [6][10]. While most objects follow the "default" behavior of calling valueOf first, specific built-in objects like Date override this; for Date objects, the "default" hint is treated as "string", prioritizing toString [6][8][7]. If the conversion result is not a primitive value, a TypeError is thrown [6][10].
Citations:
- 1: https://262.ecma-international.org/17.0/
- 2: https://tc39.es/ecma262/2026/multipage/ecmascript-language-expressions.html
- 3: https://tc39.es/ecma262/2022/multipage/ecmascript-language-expressions.html
- 4: https://read262.jedfox.com/ecmascript-language-expressions/additive-operators/
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Addition
- 6: https://tc39.es/ecma262/2026/multipage/abstract-operations.html
- 7: https://tc39.es/ecma262/2025/multipage/abstract-operations.html
- 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toPrimitive
- 9: https://javascript.info/object-toprimitive
- 10: https://tc39.es/ecma262/multipage/abstract-operations.html
Restrict unsafe coercions in value_is_fold_safe.
The current whitelist still allows unsafe evaluation to move inside object initializers: unary -, +, ~, string +/equality, inequality, short-circuit logical operators, binary **, and template interpolation all force primitive conversion or ToString, which can invoke a captured valueOf/toString/Symbol.toPrimitive before the builder binding is initialized. Exclude those operators; for E::Tpl, only allow interpolations that cannot reach a user trap, not recursive value_is_fold_safe expressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-hir/src/lower/builder_fold.rs` at line 289, Update
value_is_fold_safe, used by the fold_key_ok guard, to reject unary -, +, and ~,
string concatenation/equality or inequality, short-circuit logical operators,
binary **, and template interpolations that may invoke user coercion before
builder initialization. For E::Tpl, permit only trap-free interpolations and do
not recursively approve expressions via value_is_fold_safe; preserve safe
operator cases.
1 participant
First gap-closure slice from the #6812 coverage matrix (matrix + triage measurements:
docs/object-write-matrix.md, posted on the issue). Targets the matrix's worst row — the builder pattern (const o: any = {}; o.a = …; o.b = …;), measured at 186× node onmain: every real-world API response, parser, and ORM row builds objects this way, and the shape is doubly excluded from the fast paths (class_id == 0receivers fail the write-PIC gate, and each assign is a full dynamic transition write at ~517 ns).What this does
A pre-lowering AST pass (
perry-hir/src/lower/builder_fold.rs) folds straight-line builder sequences into the object literal they spell out, so they lower through the existing anon-shape literal machinery — shape-cached keys array, typed slots, direct stores; the code path that already beats node. Modules with no candidates pay one linear read-only scan and no clone.Soundness (full argument in the module docs)
The rewrite is unobservable because: value expressions run in the same order at the same sequence points (only the allocation moves after them, and a bare allocation has no user-visible effects); values may not reference the bound name (checked conservatively by symbol anywhere in the value, shadowing ignored — false positives only block the fold); a throwing value leaves a partially-built object no live code can reach. The fold refuses:
__proto__keys (setter semantics — pre-existing dynamic-assignment gap filed separately as #6828), duplicate keys, literals containing accessors/spread/computed/method props (an appended key could turn a setter invocation into a redefinition), non-UTF8 string keys, exported declarations, and anything past 64 props. A miss is only a missed optimization.Evidence
letrebinding): byte-identical with node on every fold-relevant case.w15_append_build: 1489 ms → 196 ms (7.6×) — 186× → 25× vs node, checksum identical. 15-run idle-protocol table in the PR comment.What remains for this row (next slices, profiled)
The residual 25× is: per-slot GC layout tracking (#5094), the anon-shape constructor's stores using the class-field guard call instead of the inline path, the TLS floor (#6809), and GC/barrier throughput under allocation pressure — each its own reviewable slice.
Summary by CodeRabbit