perf(object): #6812 w16 — per-site builder identity, learned width, and compile-time width hints beat node on overflow-slot writes#6833
Merged
Conversation
added 3 commits
July 25, 2026 13:40
…h in compiled new Empty object literals lower to a unique 0-field anon-shape class per source site (content-addressed on source path + byte offset), giving builder-pattern objects a non-zero class_id: the runtime's learned inline sizing can attribute overflow growth to the site and right-size later allocations, and the static-key write PIC gate admits the objects. js_object_alloc_class_inline_keys (the compiled-new allocation path) now consumes learned_inline_field_count and stores the widened field_count, matching the dynamic-construct path's capacity semantics. Claude-Session: https://claude.ai/code/session_01QJ5mwMDPc63tNLAFPdthAG
…ded dynamic-key builders
A '{} then for (let k = C0; k < C1; k++) o[key] = v' builder has a
statically provable final width even though its keys are computed. Learned
sizing alone right-sizes only instances AFTER the first overflow, leaving
instance #1 permanently under-sized — and as element 0 of the array built
at the site it vetoes the whole-loop clone guard for every hot loop over
that array. The scan (builder_fold::empty_builder_width_hints) records the
proven width per empty-literal site; the per-site anon-shape class carries
it as Class::alloc_width_hint (stable-hashed), and compiled new allocates
max(fields, hint) inline slots. Capacity only — keys/enumeration unchanged.
Claude-Session: https://claude.ai/code/session_01QJ5mwMDPc63tNLAFPdthAG
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEmpty object literals now receive unique source-site shape classes with optional compile-time builder width hints. The hint propagates through HIR and codegen into runtime allocation, which combines it with learned widths. Supporting fixtures, IR regression coverage, documentation, and changelog entries were updated. ChangesBuilder identity and width-aware allocation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleLowering
participant WidthScanner
participant ObjectLowering
participant Codegen
participant RuntimeAllocator
ModuleLowering->>WidthScanner: compute builder width hints
WidthScanner-->>ModuleLowering: return per-source-site widths
ModuleLowering->>ObjectLowering: lower empty object literal
ObjectLowering->>Codegen: emit unique hinted anon-shape class
Codegen->>RuntimeAllocator: allocate inline fields
RuntimeAllocator->>RuntimeAllocator: apply learned inline width
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/perry-hir/src/lower/builder_fold.rs (1)
909-913: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
hint_walk_expr'sCallarm never visits the callee, unlikescan_expr's.An IIFE builder (
(function(){ const o = {}; for(...){} return o; })()) is invisible to this scanner since the calleeExpr::Fn/Expr::Parenis never walked.scan_expr's existingE::Callarm already walks the callee for the same reason (matches!(&c.callee, ast::Callee::Expr(e) if scan_expr(e))).♻️ Proposed fix: walk the callee too
E::Call(c) => { + if let ast::Callee::Expr(callee) = &c.callee { + hint_walk_expr(callee, hints); + } for arg in &c.args { hint_walk_expr(&arg.expr, hints); } }🤖 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` around lines 909 - 913, Update the E::Call arm of hint_walk_expr to also visit the call’s callee expression before walking its arguments, matching scan_expr’s callee traversal; preserve the existing argument traversal and handle expression callees without attempting to walk non-expression callees.crates/perry-hir/src/stable_hash/tests.rs (1)
284-284: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a non-zero width-hint hash regression test.
alloc_width_hint: 0only updates the fixture to compile; it does not verify thatClass::hashincludes the new field. Add a variant with a different non-zero hint and assert that its module hash differs.🤖 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/stable_hash/tests.rs` at line 284, Add a regression-test variant in the existing stable-hash tests using a distinct non-zero alloc_width_hint, then compute its hash through Class::hash and assert it differs from the zero-hint fixture’s module hash. Keep the existing zero-hint case and test structure unchanged.crates/perry-codegen/tests/class_keys_gc_root.rs (1)
156-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this regression in a PR-visible test target.
This new test lives under
crates/perry-codegen/tests, but integration suites there do not run on every PR. Prefer cargo-test-visible unit coverage or wire this target into every PR so the GC-root regression cannot be skipped.
As per coding guidelines, acceptance coverage undercrates/*/tests/*.rsshould preferably be covered by unit tests that run on every PR.🤖 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-codegen/tests/class_keys_gc_root.rs` around lines 156 - 196, Move class_keys_global_is_registered_as_gc_root from the integration-test location into cargo-test-visible unit coverage, or wire its current target into every PR’s test workflow. Preserve the existing assertions verifying the per-class keys global address is passed to js_gc_register_global_root.Source: Coding guidelines
🤖 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 759-774: Update empty_builder_width_hints to handle
ast::ModuleItem::ModuleDecl entries in both top-level scans, including exported
declarations and default exports. Reuse the existing note_hint_pair and
hint_walk_stmt traversal logic so exported function/class bodies and relevant
declaration statements receive the same width hints as non-exported statements.
In `@docs/object-write-matrix.md`:
- Line 52: Reconcile the w16 benchmark figures using one recorded benchmark run:
in docs/object-write-matrix.md:52-52, correct the final ratio or Node
measurement so they agree arithmetically; in
changelog.d/6812-w16-builder-identity.md:1-1, replace the 173× claim with the
matching pre-fix multiplier. Ensure both files report the same baseline and
measurements.
---
Nitpick comments:
In `@crates/perry-codegen/tests/class_keys_gc_root.rs`:
- Around line 156-196: Move class_keys_global_is_registered_as_gc_root from the
integration-test location into cargo-test-visible unit coverage, or wire its
current target into every PR’s test workflow. Preserve the existing assertions
verifying the per-class keys global address is passed to
js_gc_register_global_root.
In `@crates/perry-hir/src/lower/builder_fold.rs`:
- Around line 909-913: Update the E::Call arm of hint_walk_expr to also visit
the call’s callee expression before walking its arguments, matching scan_expr’s
callee traversal; preserve the existing argument traversal and handle expression
callees without attempting to walk non-expression callees.
In `@crates/perry-hir/src/stable_hash/tests.rs`:
- Line 284: Add a regression-test variant in the existing stable-hash tests
using a distinct non-zero alloc_width_hint, then compute its hash through
Class::hash and assert it differs from the zero-hint fixture’s module hash. Keep
the existing zero-hint case and test structure unchanged.
🪄 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: d821ad84-e404-4eb8-ad2b-c7f331ca15f4
📒 Files selected for processing (34)
changelog.d/6812-w16-builder-identity.mdcrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/collectors/class_accessors.rscrates/perry-codegen/src/collectors/scalar_method_dispatch.rscrates/perry-codegen/src/collectors/this_as_value.rscrates/perry-codegen/src/lower_call/buffer_intrinsic.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/type_analysis_tests.rscrates/perry-codegen/tests/class_keys_gc_root.rscrates/perry-codegen/tests/constructor_recursion.rscrates/perry-codegen/tests/native_proof_buffer_views.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/private_guard_declaring_class.rscrates/perry-codegen/tests/static_symbol_hygiene.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-codegen/tests/typed_shape_descriptor.rscrates/perry-hir/src/analysis/value_types_tests.rscrates/perry-hir/src/ir/decl.rscrates/perry-hir/src/lower/builder_fold.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_object.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/module_decl/namespace.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/monomorph/specialize.rscrates/perry-hir/src/stable_hash/decls.rscrates/perry-hir/src/stable_hash/tests.rscrates/perry-runtime/src/object/alloc.rscrates/perry-transform/src/async_to_generator.rscrates/perry-transform/src/deforest/tests.rscrates/perry-transform/src/generator/id_scan.rscrates/perry-transform/src/inline/mod.rsdocs/object-write-matrix.md
Both reproduce at origin/main and at f6fb9a4~1 — broken before the whole #6812 line, surfaced now because this PR touches the test files and e2e-scoped selects suites by changed paths. - native_owned_uint8array_get_fallback: codegen migrated the disposed-view fallback to js_uint8array_index_get_value, which validates against the typed-array kind registry before dereferencing (the memory-safety property the test pins); assert the new helper. - typed_feedback_trace_dump_runs_before_entry_return: the entry epilogue gained a second exit path returning the dynamic js_process_pending_exit_code result, so rfind("ret i32 0") no longer finds the last return. Assert the stronger real invariant: every entry return is immediately preceded by the trace dump. Claude-Session: https://claude.ai/code/session_01QJ5mwMDPc63tNLAFPdthAG
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-codegen/tests/typed_feedback.rs`:
- Around line 221-241: Update the return validation loop in the typed-feedback
test to verify that each `ret i32` is immediately preceded by the trace-dump
call, ignoring only whitespace or empty lines. Remove the fixed 200-byte
`preceding.contains` window so an earlier exit path’s dump cannot satisfy a
later return; preserve the existing requirement that the entry contains at least
one return.
🪄 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: 28d216b0-5ded-47a0-8090-90525dc26316
📒 Files selected for processing (2)
crates/perry-codegen/tests/native_proof_buffer_views.rscrates/perry-codegen/tests/typed_feedback.rs
Comment on lines
+221
to
+241
| // The entry epilogue now has TWO exit paths: the host-return path | ||
| // (`ret i32 0`) and the event-loop exit path, which returns the dynamic | ||
| // `js_process_pending_exit_code` result (`ret i32 %rN`). The old | ||
| // rfind(dump) < rfind("ret i32 0") comparison broke the day the second | ||
| // path appeared — its dump call sits after the literal `ret i32 0`. The | ||
| // real invariant is stronger: EVERY entry return must be immediately | ||
| // preceded by the trace dump, so no exit path can skip the dump. | ||
| let mut ret_count = 0; | ||
| let mut search_from = 0; | ||
| while let Some(rel) = ir[search_from..].find("ret i32") { | ||
| let ret_pos = search_from + rel; | ||
| ret_count += 1; | ||
| let preceding = &ir[ret_pos.saturating_sub(200)..ret_pos]; | ||
| assert!( | ||
| preceding.contains("call void @js_typed_feedback_maybe_dump_trace()"), | ||
| "entry return at byte {ret_pos} is not preceded by the typed-feedback \ | ||
| trace dump:\n...{preceding}" | ||
| ); | ||
| search_from = ret_pos + 1; | ||
| } | ||
| assert!(ret_count >= 1, "entry should return i32"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert adjacency instead of searching a 200-byte window.
preceding.contains(...) can match a dump belonging to an earlier exit path, allowing a later ret i32 without its own dump to pass. It also makes the test depend on unrelated IR size. Check the immediately preceding non-empty instruction, or parse the entry-function control flow, for each return.
Proposed assertion tightening
- let mut search_from = 0;
- while let Some(rel) = ir[search_from..].find("ret i32") {
- let ret_pos = search_from + rel;
+ let mut previous_nonempty = None;
+ for line in ir.lines() {
+ if line.trim_start().starts_with("ret i32") {
ret_count += 1;
- let preceding = &ir[ret_pos.saturating_sub(200)..ret_pos];
- assert!(
- preceding.contains("call void `@js_typed_feedback_maybe_dump_trace`()"),
- "entry return at byte {ret_pos} is not preceded by the typed-feedback \
- trace dump:\n...{preceding}"
- );
- search_from = ret_pos + 1;
+ assert_eq!(
+ previous_nonempty,
+ Some("call void `@js_typed_feedback_maybe_dump_trace`()"),
+ "entry return is not immediately preceded by the typed-feedback trace dump"
+ );
+ }
+ if !line.trim().is_empty() {
+ previous_nonempty = Some(line.trim());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The entry epilogue now has TWO exit paths: the host-return path | |
| // (`ret i32 0`) and the event-loop exit path, which returns the dynamic | |
| // `js_process_pending_exit_code` result (`ret i32 %rN`). The old | |
| // rfind(dump) < rfind("ret i32 0") comparison broke the day the second | |
| // path appeared — its dump call sits after the literal `ret i32 0`. The | |
| // real invariant is stronger: EVERY entry return must be immediately | |
| // preceded by the trace dump, so no exit path can skip the dump. | |
| let mut ret_count = 0; | |
| let mut search_from = 0; | |
| while let Some(rel) = ir[search_from..].find("ret i32") { | |
| let ret_pos = search_from + rel; | |
| ret_count += 1; | |
| let preceding = &ir[ret_pos.saturating_sub(200)..ret_pos]; | |
| assert!( | |
| preceding.contains("call void @js_typed_feedback_maybe_dump_trace()"), | |
| "entry return at byte {ret_pos} is not preceded by the typed-feedback \ | |
| trace dump:\n...{preceding}" | |
| ); | |
| search_from = ret_pos + 1; | |
| } | |
| assert!(ret_count >= 1, "entry should return i32"); | |
| // The entry epilogue now has TWO exit paths: the host-return path | |
| // (`ret i32 0`) and the event-loop exit path, which returns the dynamic | |
| // `js_process_pending_exit_code` result (`ret i32 %rN`). The old | |
| // rfind(dump) < rfind("ret i32 0") comparison broke the day the second | |
| // path appeared — its dump call sits after the literal `ret i32 0`. The | |
| // real invariant is stronger: EVERY entry return must be immediately | |
| // preceded by the trace dump, so no exit path can skip the dump. | |
| let mut ret_count = 0; | |
| let mut previous_nonempty = None; | |
| for line in ir.lines() { | |
| if line.trim_start().starts_with("ret i32") { | |
| ret_count += 1; | |
| assert_eq!( | |
| previous_nonempty, | |
| Some("call void `@js_typed_feedback_maybe_dump_trace`()"), | |
| "entry return is not immediately preceded by the typed-feedback trace dump" | |
| ); | |
| } | |
| if !line.trim().is_empty() { | |
| previous_nonempty = Some(line.trim()); | |
| } | |
| } | |
| assert!(ret_count >= 1, "entry should return i32"); |
🤖 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-codegen/tests/typed_feedback.rs` around lines 221 - 241, Update
the return validation loop in the typed-feedback test to verify that each `ret
i32` is immediately preceded by the trace-dump call, ignoring only whitespace or
empty lines. Remove the fixed 200-byte `preceding.contains` window so an earlier
exit path’s dump cannot satisfy a later return; preserve the existing
requirement that the entry contains at least one return.
…ing Class field in perry test cfg; one-source w16 numbers
- empty_builder_width_hints now descends ModuleDecl items (export fn/class/
const, export default) and pairs 'export const o = {}' with its build
loop — exported builder helpers are the most common real-world shape.
- perry crate test-cfg Class literals gained alloc_width_hint (CI cargo-test
E0063; local checks missed the CLI crate's tests).
- w16 doc row and changelog now cite one measured lineage (pre-fix 4163/24
= 173x, post-fix 3/29 = 0.10, 2026-07-25 sweeps).
Claude-Session: https://claude.ai/code/session_01QJ5mwMDPc63tNLAFPdthAG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the last catastrophic row of the #6812 object-write matrix: w16 overflow-slot writes go from 173× slower than node to 10× faster (3 ms vs 29 ms median; was 4163 ms). Two stacked mechanisms, both broad rather than benchmark-shaped:
1. Builder-pattern objects get a learnable per-site identity (
13053ae4a){}— the seed of every dynamic builder: API responses, parser output, ORM rows) previously lowered to the legacy class-0 empty object. Class 0 is excluded from the static-key write PIC and from the runtime's learned inline sizing, so a{}-built object with more thanINLINE_SLOT_FLOOR(4) fields keeps the rest in the thread-local overflow side-table forever — every write paying TLS + RefCell + PtrHashMap.{}now lowers tonew __AnonShape_<site-hash>(), a unique 0-field shape-only anon class per source site (content-addressed on file + byte offset, same naming scheme the shape-addressed anon classes already use, so all__AnonShape_runtime introspection parity applies).js_object_alloc_class_inline_keys(the compiled-newallocation path) now consumeslearned_inline_field_count, mirroring the dynamic-construct path's capacity semantics: the first instance of a site that overflows teaches the runtime the site's true width; every later instance allocates all fields inline.2. Compile-time width hint for const-bounded dynamic-key builders (
83b1f5c48)const o = {};immediately followed byfor (let k = C0; k < C1; k++) o[key] = v(1–4 writes per iteration, plain=member assigns tooonly) proves the final width statically. The scan (builder_fold::empty_builder_width_hints) records it per site; the site class carries it asClass::alloc_width_hint(included in stable hashing per the build: the object cache doesn't key on codegen env vars — PERRY_WRITE_BARRIERS=0 silently does nothing with a warm cache #6394 rule); compilednewallocatesmax(fields, hint)inline slots. Pure capacity: keys/enumeration are unchanged, over-counting can only waste slots, and the value side of the assignments is deliberately unconstrained (it never affects width).Measured (release, default pipeline, node v26.3.0 arm64, medians of 3 alternated runs, identical checksums)
Full 18-cell matrix: zero regressions; 10 of 18 rows now beat node (w0–w5, w7, w11, w16, w18).
Semantics
empty_site_sanity.ts(14 cases):Object.keys/entries/values/getOwnPropertyNames,constructor/instanceof/prototype, delete + re-add,Object.assign/spread, for-in order,defineProperty, freeze/seal, symbol keys,String(o), JSON round-trip, two same-site instances (pre/post learning) — byte-identical vs node.builder_fold_sanity.ts(11 cases, s3 = pre-existing Dynamic __proto__ assignment creates an own property instead of invoking the prototype setter #6828) andmul_index_sanity.ts— byte-identical, no regressions.Notes
{}is a per-site class registry entry.Refs #6812.
https://claude.ai/code/session_01QJ5mwMDPc63tNLAFPdthAG
Summary by CodeRabbit
{}) by using per-site builder identities and learned sizing to better match expected inline field capacity.w16_overflow_slotbenchmark results and explanations.