perf(runtime): megamorphic read stub cache (pure property read −12%) - #8988
Conversation
Typed-feedback recording is off by default, and guard_observe and record_fallback_call both early-return in that mode — but the property wrappers had already built the whole Observation to hand them, hashing the key and resolving the receiver's shape first. On an isolated property-read loop js_typed_feedback_object_get_field_by_name_f64 was 10% of self time, nearly all of it that dead work. Apply PerryTS#5094's gate, which the array index wrappers already carry and PerryTS#8951 gave the fast store path: when recording is off, take the underlying op directly. Behaviour is unchanged in both modes — with recording off guard_observe returns contract_valid and the fallback recorder is a no-op, so the wrapper already reduced to exactly this call. Also: object_live_slot_count reads live_inline_slot_count through the shape table's record instead of lifting the whole ~48-byte descriptor to discard all but four bytes. That bound is consulted on essentially every property operation, and shape_descriptor_by_id was 10.1% of the same loop. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
…eads The read twin of the dynamic-write stub, 2-way set-associative from the start (PerryTS#8977 measured what direct-mapped costs: a colliding pair evicts each other every rotation, so both miss forever). A hit skips js_object_get_field_by_name's fast-lane guard chain — address class, interned-key flag, arena classification, header type/flags/class, keys-array validation — plus the read-plan probe, whose epoch the collector bumps at loop-poll cadence, so on a steady read loop it is repeatedly cold and falls through to a shape-index hash lookup. Safety mirrors the write stub: entries store CONTENT bits, never an address, so a recycled key address cannot produce a false hit, and keys that do not fit the inline form are not cached. Every hit re-validates heap-object type, not-forwarded, blocking flags, class id, and the receiver's current shape token — which pins the exact key set and order, so a match means the cached slot still names this key. The probe sits after the process.env and Proxy arms, which keep their own semantics, and the stub is only primed from inside the lane, once the receiver is proved ordinary. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
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 (13)
📝 WalkthroughWalkthroughChangesDynamic property read path
Imported private brands
Array-subclass behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to This PR adds a per-thread cache that can speed up ordinary property reads while preserving special-object handling and validating receiver shape and key content. It is mergeable with owner awareness or follow-up for the bounded risk around same-thread reentrancy and explicit class-object eligibility on the optimized path. Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_object_get_field_by_name
participant read_stub
participant read_plan_lookup
Caller->>js_object_get_field_by_name: Read dynamic string property
js_object_get_field_by_name->>read_stub: Probe shape token and key bits
alt Cache hit
read_stub-->>js_object_get_field_by_name: Return cached slot
else Cache miss
js_object_get_field_by_name->>read_plan_lookup: Resolve property slot
read_plan_lookup-->>js_object_get_field_by_name: Return slot
js_object_get_field_by_name->>read_stub: Cache resolved slot
end
js_object_get_field_by_name-->>Caller: Return property value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is detailed and relevant. It explains the implementation, safety invariants, benchmark results, related issues, and validation results. It does not reproduce the template headings or checklist, but it provides the required information in substance. Full details: Docstring CoverageExplanation Docstring coverage is 93.75% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 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 |
* fix(codegen): an imported class no longer installs its private brand twice (PerryTS#8962) `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `<prefix>__<class>_constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY * chore: PR-key the fragment --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
* fix(runtime): inherit Array-subclass fill (PerryTS#8953) * chore: PR-key the fragment; reuse the shared StringHeader payload helper --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
READ_STUB is a new identity-ratcheted thread-local holder; recorded the same not_a_gc_pointer verdict WRITE_STUB carries, since read_stub_key_bits returns short_ascii_sso_bits (content packed inline) and never a heap address.
|
Merged. Going 2-way from the start is right — #8977 already paid for that experiment, and a colliding pair in a direct-mapped table misses forever rather than occasionally. Three fixes pushed, one of which was a real gate failure:
Validation — runtime 2781/0, codegen 1341/0 ( The −12% is not re-measured here. |
2 participants
The read twin of the dynamic-write stub (#8965, made 2-way in #8977). This one is 2-way set-associative from the start — #8977 already measured what direct-mapped costs, and there was no reason to repeat that experiment: a colliding pair evicts each other on every rotation through the key set, so both miss forever.
A hit skips
js_object_get_field_by_name's fast-lane guard chain — address class, interned-key flag, arena classification, header type/flags/class, keys-array validation — and the read-plan probe. That last one matters more than its own profile share suggests: the plan's epoch is bumped by the incremental collector at loop-poll cadence, so on a steady read loop it is repeatedly cold and falls through to a shape-index hash lookup.Measurement
Interleaved A/B, min-of-21, built from the exact parent commit and this commit in one run:
Safety
Entries store content bits, never an address, so a key that dies and has its address recycled cannot produce a false hit; keys that do not fit the inline form are not cached at all. Every hit re-validates heap-object type, not-forwarded, blocking flags, class id, and the receiver's current shape token — which pins the exact key set and order, so a match means the cached slot still names this key. A stale entry misses; it cannot resolve to the wrong property.
Placement carries its own argument: the probe sits after the
process.envand Proxy arms so those keep their semantics, and the stub is only ever primed from inside the fast lane — once the receiver has been proved an ordinary shaped object with a resolvable own slot.Adversarial testing
A wrong-slot read is silent corruption rather than a crash, so this ships with a differential built to attack each invariant directly:
deletethat changes the shape under a cached slot, then re-add;Object.definePropertyinstalling a getter over a cached data slot;Object.freeze;Output is byte-identical to node on all of it. Also:
perry-runtime2779 passed / 0 failed; private-member output identical to base; computed-key differential identical to node.https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
Summary by CodeRabbit
Performance
Bug Fixes
Array.prototype.fillinheritance so it is not incorrectly reported as an own property.fillbehavior.