perf(runtime): #6759 Phase C2 — per-key descriptor summaries in the ObjectMeta record#6800
Conversation
…bjectMeta record (WIP)
📝 WalkthroughWalkthroughPhase C2 stores per-key descriptor summaries in ChangesDescriptor summary gating
GC suppression scope
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DescriptorInstaller
participant ObjectMeta
participant DescriptorLookup
participant DescriptorTable
DescriptorInstaller->>ObjectMeta: record descriptor key bits
DescriptorLookup->>ObjectMeta: check owner and key summary
ObjectMeta-->>DescriptorLookup: possible entry or authoritative miss
DescriptorLookup->>DescriptorTable: probe only when possible
DescriptorTable-->>DescriptorLookup: descriptor attributes or accessor
🚥 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.
🧹 Nitpick comments (4)
crates/perry-runtime/src/gc/policy.rs (2)
646-684: 🩺 Stability & Availability | 🔵 TrivialRebuild the linked runtime archives during validation.
This runtime change requires rebuilding
perry-runtime-staticandperry-stdlib-static, along with the relevant runtime crates, so tests do not run against stale archives.🤖 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-runtime/src/gc/policy.rs` around lines 646 - 684, Update the validation/build workflow for the GcSuppressScope change to rebuild perry-runtime-static and perry-stdlib-static, including the relevant runtime crates, before running tests. Ensure validation uses these freshly generated archives rather than stale linked artifacts.Source: Coding guidelines
663-668: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMark the RAII guard as must-use.
Calling
GcSuppressScope::new()without retaining the result immediately drops it, so suppression lasts for no time. Add#[must_use]to prevent accidental misuse.🤖 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-runtime/src/gc/policy.rs` around lines 663 - 668, Add a #[must_use] attribute to the GcSuppressScope RAII guard, targeting the GcSuppressScope type or its new constructor, so callers are warned when the returned guard is immediately discarded. Preserve the existing suppression behavior and constructor logic.crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs (1)
667-746: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
if x_bit != 0guard is always true.
descriptor_key_bit_bytesreturns1u64 << (hash & 63), which can never be0, so this condition never gates anything (likely copy/paste from thex_bit != other_bitcheck above). Harmless — the assertion still runs — but the guard is dead weight.🧹 Optional cleanup
- // The attr install must not set the ACCESSOR word. - if x_bit != 0 { - assert!( - !crate::object::descriptor_state::may_have_descriptor_entry(addr, "x", true), - "attr install must not claim a possible accessor entry" - ); - } + // The attr install must not set the ACCESSOR word. + assert!( + !crate::object::descriptor_state::may_have_descriptor_entry(addr, "x", true), + "attr install must not claim a possible accessor entry" + );🤖 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-runtime/src/gc/tests/dead_owner_side_tables.rs` around lines 667 - 746, Remove the redundant if x_bit != 0 guard around the accessor-word assertion in test_descriptor_meta_summary_gates_probes, since test_descriptor_key_bit("x") always returns a nonzero bit. Keep the assertion itself and its existing message unchanged.crates/perry-runtime/src/object/descriptor_state.rs (1)
542-550: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding direct test coverage for the refined write-intercept gate.
The per-key refinement to
plain_data_write_may_interceptsits on the dynamic-write hot path (webpack'sdefineProperty(exports, "__esModule", …)case called out in the comment) but isn't directly exercised by the new tests indead_owner_side_tables.rs(those coverget_property_attrs/get_accessor_descriptor, not this function). A small test installing one descriptor key and asserting the write-intercept verdict for both that key and an unrelated key would lock in this behavior.🤖 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-runtime/src/object/descriptor_state.rs` around lines 542 - 550, Add focused test coverage for the refined write-intercept gate used by plain_data_write_may_intercept: install an own descriptor for one key, then assert the gate reports interception for that key but not for an unrelated string key. Place the test with the existing descriptor-side-table tests and preserve coverage of the symbol-key/prototype behavior already handled by the implementation.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 646-684: Update the validation/build workflow for the
GcSuppressScope change to rebuild perry-runtime-static and perry-stdlib-static,
including the relevant runtime crates, before running tests. Ensure validation
uses these freshly generated archives rather than stale linked artifacts.
- Around line 663-668: Add a #[must_use] attribute to the GcSuppressScope RAII
guard, targeting the GcSuppressScope type or its new constructor, so callers are
warned when the returned guard is immediately discarded. Preserve the existing
suppression behavior and constructor logic.
In `@crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs`:
- Around line 667-746: Remove the redundant if x_bit != 0 guard around the
accessor-word assertion in test_descriptor_meta_summary_gates_probes, since
test_descriptor_key_bit("x") always returns a nonzero bit. Keep the assertion
itself and its existing message unchanged.
In `@crates/perry-runtime/src/object/descriptor_state.rs`:
- Around line 542-550: Add focused test coverage for the refined write-intercept
gate used by plain_data_write_may_intercept: install an own descriptor for one
key, then assert the gate reports interception for that key but not for an
unrelated string key. Place the test with the existing descriptor-side-table
tests and preserve coverage of the symbol-key/prototype behavior already handled
by the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c1cdf9b-4a66-46a2-adee-e3c186ee6dfd
📒 Files selected for processing (8)
changelog.d/6800-object-meta-descriptor-summaries-phase-c2.mdcrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/dead_owner_side_tables.rscrates/perry-runtime/src/object/descriptor_state.rscrates/perry-runtime/src/object/field_get_set/enumeration.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/prototype_chain.rsdocs/shape-tree-plan.md
|
Gap suite vs |
1 participant
Phase C2 of #6759 / #6798: per-key descriptor-presence facts move into the per-object
ObjectMetarecord, so the hot read/write/reflection paths can prove "no descriptor can cover this key" in two loads instead of a per-callStringbuild + side-table probe.What changed
ObjectMetagrows two POD Bloom words —attr_key_bits/accessor_key_bits, bitfnv(key) & 63per installed key, monotonic per object. The GC trace arm is untouched (still visits onlyprototype); the record size flows fromsize_ofat the one alloc site.set_property_attrs,set_accessor_descriptor, and both gate-neutralset_builtin_*variants — the only four insert sites, verified by audit; everything else is removes/scans). For meta-capable owners this maintains the invariant the probes rely on: a clear bit — or a still-null meta — proves the tables hold no entry for that key.get_property_attrs/get_accessor_descriptorgate on the summary: authoritative misses returnNonewith no allocation. All ~30 call sites (read/write accessor gates, prototype-chain walks,for-in/Object.keysenumerability filtering, JSON getter checks, Reflect) get the per-key precision automatically.Object.keysfast path replaces its per-call O(table-size) owner scan with the owner-level summary check.plain_data_write_may_interceptrefinesOBJ_FLAG_HAS_DESCRIPTORSper-key: an object with a descriptor on one key (webpack'sdefineProperty(exports, "__esModule", …)) keeps the dynamic-write fast path for its other keys.GcSuppressScope(flag-only RAII no-move window):object_meta_ensureallocates, and a triggered collection could move the owner while installers (freeze/seal loops, defineProperty) hold raw pointers across repeated installs. All GC triggers checkGC_FLAG_SUPPRESSED(gc_check_trigger, budgeted stepper, safepoint moving minor), so the tiny window is genuinely move-free. Nesting-safe (restores prior state).Why not the design doc's per-shape attrs sidecar
Shape records are keyed on the keys_array address with validation-on-hit — a trust model that only works for positive facts. "No descriptors on this shape" is a negative fact with nothing to validate against, and keys identity churns on every grow-realloc, so a shape-resident claim goes stale undetectably. The
metarecord is the sound carrier: it travels with the object (GC-traced, moved + rewritten with its owner) and is null on every fresh allocation — which also means no shape-split is needed (no O(N) keys clone on freeze). True shape-resident attributes return in C3 when shape identity becomes header-resident and descriptor install becomes an explicit transition.docs/shape-tree-plan.mdupdated accordingly.Bonus correctness: a fresh object at a recycled address can no longer be misread as owning a dead tenant's not-yet-pruned descriptor entries (null meta ⇒ authoritative miss).
Validation
cargo test -p perry-runtime --lib -- --test-threads=1green, including two new tests: summary gating semantics (fresh-owner miss, installed-key hit, non-colliding miss, attr/accessor word separation, handle-band conservative round-trip) and a copied-minor move test (bits travel with the moved meta record while the table entry rekeys — gated getter resolves at the new address).node --experimental-strip-types— results in PR comment.Per the #6759 method: lands independently behind green suites; runtime-only (no codegen change, no header change).
Summary by CodeRabbit