perf(codegen): refresh rooted arrays, gate super-scope, and count store safepoints - #8680
Conversation
…re safepoints Lands four reviewed PRs as one squash. - #8670: refresh rooted arrays during iteration. - #8673: fix a `--report-size` false positive from std-internal crate names. - #8668: specialize dense Array-subclass indexing. - #8678 (#8583): count property/index STORES as GC safepoint sites in the spill estimate. `PropertySet`/`PropertyUpdate`/`IndexSet` lower to collecting runtime calls that rewrite-statepoints-for-gc gives a statepoint, but none were counted, so a closed-shape object literal's constructor -- one long run of `this.field = v` -- estimated ~0, was never spilled to the shadow frame, and RS4GC grew one `__AnonShape_*_constructor` from 34,009 to 2,280,128 instructions, overrunning the #8586 per-function budget and refusing the whole module. Reads are deliberately not counted: they frequently inline to a shape-cached load with no call, so counting them would over-spill read-heavy hot loops. Version bump stripped per maintainer policy; the Cargo.lock diff was verified version-only before stripping.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe PR adds guarded dense indexing for Array subclasses, refreshes rooted array iteration after relocation, counts collecting store expressions as safepoints, and separates Rust standard-library internal backtrace crates in size reports. It adds runtime, compiler, integration, unit, and changelog coverage. ChangesArray-subclass indexing
Rooted array iteration
Safepoint counting
Size-report filtering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedGetter
participant ArraySubclassCache
participant PackedArraylikeDispatcher
participant GenericPropertyLookup
GeneratedGetter->>ArraySubclassCache: validate class, shape, length, and dense slot
ArraySubclassCache-->>GeneratedGetter: return inline or spill value
GeneratedGetter->>PackedArraylikeDispatcher: dispatch cache miss
PackedArraylikeDispatcher->>GenericPropertyLookup: resolve holes, accessors, proxies, or rejected shapes
GenericPropertyLookup-->>PackedArraylikeDispatcher: return generic property result
✨ 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 |
Lands #8685. main was red on `lint` and `cargo-test` independently of any in-flight PR: three checkers went stale after correct code landed. - `shape_descriptor_census.py` still asserted the pre-#8665 fail-closed shape in the generic property-read PIC, and its callsite baseline was missing two legitimate `object_header_size_bytes(...)` sites that #8680 added in `inline_dyn_typed_array.rs`. - `addr_class_inventory.py`'s `lone-valid-obj-ptr` rule reimplemented a 1-line lookahead instead of reusing the comment/blank-aware `band_predicate_near` helper the `handle-floor` rule already uses. That flagged two correctly-paired `is_valid_obj_ptr` + `try_read_gc_header` guards in `array/subclass.rs` as false positives. - `codegen_env_vars_are_build_cache_inputs` failed on two unclassified env vars: `PERRY_CONST_ARRAY_DESCRIPTOR` (changes emitted IR, so a build-cache input) and `PERRY_DIALECT_DUMP` (read only on an already fatal dialect-construction failure, so excluded). The addr-class ratchet baseline was regenerated from the merged tree rather than hand-merged through its conflict. Every change is in the strict direction: four entries removed outright and `class_registry/construct.rs` 5 -> 3, nothing loosened. The rule still detects real violations (`--self-test` passes). Version bump stripped per maintainer policy; the changelog fragment was renamed to its own PR number. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Lands four reviewed PRs as one squash: #8670, #8673, #8668, #8678.
#8678 (#8583) is the notable one — it was the last blocker to a working
ccbinary.PropertySet/PropertyUpdate/IndexSetlower to collecting runtime calls thatrewrite-statepoints-for-gcgives a statepoint, butcount_safepoint_sitescounted none of them. A closed-shape object literal compiles to a constructor that is one long run ofthis.field = vstores, so its estimate was ~0, it was never spilled to the shadow frame, and RS4GC grew one__AnonShape_*_constructorfrom 34,009 to 2,280,128 instructions — overrunning the #8586 per-function budget and refusing the whole module.Reads (
PropertyGet/IndexGet) are deliberately excluded: they frequently inline to a shape-cached load with no call, so counting them would over-spill read-heavy hot loops. That asymmetry is documented at the match arm.Validation (on the merged 4-PR result, not per-branch)
cargo fmt --all --check: pass (one fmt fix applied to perf(codegen): count property/index stores as GC safepoints in the spill estimate (#8583) #8678's own file)perry-codegen --lib: 1194 passed, 0 failed (+1 = perf(codegen): count property/index stores as GC safepoints in the spill estimate (#8583) #8678'sproperty_and_index_stores_are_safepoints)perry-runtime --lib(RUST_TEST_THREADS=1): 2651 passed, 0 failedVersion bump stripped per maintainer policy; the
Cargo.lockdiff was confirmed version-only first. Fourchangelog.d/fragments carried through.Follow-up (not a blocker)
#8678 improves estimate accuracy; fully spilling that constructor also needs the spill threshold reconciled with the #8586 budget — #8623's 32M sits above the 1.57M budget. Tracked separately.
Summary by CodeRabbit
Performance
Arraysubclasses is now significantly faster while preserving correct behavior for holes, accessors, proxies, and prototype changes.Bug Fixes
Improvements