perf(runtime): typed-feedback store forwards when recording is off; one-entry cache for the external-slot remembered set (ECS round 4) - #8951
Merged
proggeramlug merged 2 commits intoAug 28, 2026
Conversation
added 2 commits
August 28, 2026 11:35
…ecording is off; a one-entry cache in front of the external-slot remembered set js_typed_feedback_array_set_index_or_string converted the index and made two out-of-line calls whose first statement is "recording is off, return" — 1.5% of an ECS frame on `column[index] = record`. One flag test, then the strict store. A Map's entries buffer is an external slot span, so every map.set(k, v) on an old map reached mark_dirty_external_slot_page: a thread-local table probe plus a linear scan of the page's header list, which grows with every map whose buffer shares the page. The (page, header) pair is now cached inline in HotTls — recorded only after the table holds it, dropped wherever the table drops a pair — so a repeated store into the same map answers from two loads, exactly as the inline-slot dirty-page cache does. Claude-Session: https://claude.ai/code/session_01FUvFrRNZyc5qknBiJbYbby
|
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 (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe runtime adds a direct strict-store path when typed feedback is disabled. It also adds a one-entry hot-TLS cache for repeated external dirty-slot pairs and invalidates the cache when entries are cleared. ChangesRuntime optimizations
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes localized runtime performance changes while preserving array-store behavior and remembered-set cache invalidation. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description explains the two changes and reports test coverage, but it does not use the required section headings or provide the required related-issue, test-plan checklist, and contributor checklist information. Resolution Reformat the description using the repository template. Add Summary, Changes, Related issue with an issue reference or "n/a", Test plan with verification commands and completed checkboxes, Screenshots / output if applicable, and the required Checklist entries.
✨ 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 |
Contributor
Author
|
Gate on the isolated perrymaster clone for |
Contributor
Author
|
Paired measurement (idle Mac mini, 9 alternating pairs, |
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 28, 2026
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
proggeramlug
added a commit
that referenced
this pull request
Aug 28, 2026
…s (computed-key read now at node parity) (#8983) * perf(runtime): skip dead feedback observation on the property wrappers 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 #5094's gate, which the array index wrappers already carry and #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 * chore(changelog): PR-key the fragment (8983-feedback-gate-and-shape-field.md) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 28, 2026
…8988) * perf(runtime): skip dead feedback observation on the property wrappers 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 #5094's gate, which the array index wrappers already carry and #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 * perf(runtime): megamorphic read stub cache for dynamic string-keyed reads The read twin of the dynamic-write stub, 2-way set-associative from the start (#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 * fix(codegen): initialize imported private brands once (#8986) * fix(codegen): an imported class no longer installs its private brand twice (#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 (#8987) * fix(runtime): inherit Array-subclass fill (#8953) * chore: PR-key the fragment; reuse the shared StringHeader payload helper --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * chore: PR-key the fragment, drop a duplicate, classify READ_STUB 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. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> Co-authored-by: x <x@x>
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.
Two runtime mechanisms from the ECS round-4 chain, cut from current main. Suites on the isolated perrymaster gate: runtime (2752, incl. the new external-cache test). Paired measurement on the
codehz/ecs"5k entities: 3 commands each + sync" row (idle Mac mini, alternating pairs) follows in a comment.js_typed_feedback_array_set_index_or_stringconverted the index and made two out-of-line calls (observe_array,record_guard_pass) whose first statement is "recording is off, return" — 1.5% of the merged-main ECS frame oncolumn[index] = record. One flag test (typed_feedback_enabled, the default is off), then the strict store; the recording path is unchanged when the env enables it.Map's entries buffer is an external slot span, so everymap.set(k, v)on an old map reachedmark_dirty_external_slot_page: a thread-local table probe plus a linear scan of that page's header list (which grows with every map whose buffer shares the page) — the two hottest offsets ofjs_map_seton merged main. The(page, header)pair is cached inline inHotTls, recorded only after the table holds it and dropped wherever the table drops a pair (clear_one_external_dirty_slot_header, which every clear path goes through), so a repeated store into the same map answers from two loads — the external-slot twin of the inline-slot dirty-page cache (perf: ECS round 4 — barrier early exits, inline captureless some loop, lean Map/Set lanes, empty-pop fast path, codegen-time const fold, inline hot-TLS values (−16.7%) #8916).external_dirty_slot_pair_cache_mirrors_the_tablepins hit / miss / re-record-after-clear.https://claude.ai/code/session_01FUvFrRNZyc5qknBiJbYbby
Summary by CodeRabbit
Performance
Reliability
Tests
Documentation