perf(runtime): share the store-plan cache with receiver-based [[Set]]#6541
Merged
proggeramlug merged 3 commits intoJul 17, 2026
Merged
Conversation
added 3 commits
July 17, 2026 19:57
Round-2 follow-up to the store-plan cache: even with the interception vet memoized, every dynamic field access still paid the exotic-check gauntlet (buffer/typed-array/exotic registry probes), a RuntimeHandleScope with rooted handles, per-read FNV key hashing, and — decisively — the shape-transition cache only stores append EDGES, so an overwrite of an EXISTING own field always fell to the O(keys) linear scan with per-store descriptor probes. Add a fast lane at the top of both js_object_get_field_by_name and js_object_set_field_by_name. The gate proves, with cheap checks only, everything the skipped slow path would have tested: - tag/band checks: not a proxy/handle/stream encoding; - classify_heap_generation != Unknown: the address is in a registered arena page, so its GcHeader is real — and no malloc-backed exotic (BufferHeader/TypedArrayHeader/DateCell/RegExpHeader/Temporal cell) can classify as arena; - GC_TYPE_OBJECT: not a closure/array/error/Map/Set; - class_id not 0 and not the native-module id: excludes arguments objects (allocated with class 0), URL-shape objects, builtin prototype hosts, plain literals; - flag gate: HAS_DESCRIPTORS (own accessors must dispatch) and TYPED_ARRAY_PROTO (reflection-accessor host) for reads; plus the frozen family, PROTO_OVERRIDE and NULL_PROTO for writes; - writes additionally require a store-plan hit (no vtable setter / prototype interceptor for this class+key). Own-field resolution comes from a new read-plan cache in prop_plan.rs: (keys_array, interned key) -> field index, direct-mapped 8192 entries, valid for one PROP_PLAN_EPOCH window. The epoch already flushes on GC, descriptor/prototype/vtable mutations; js_object_delete_field now bumps it too (a delete rewrites key->slot mappings in place; every delete variant funnels there). A lane miss does one bounded (<=4096) validated scan to populate; an absent own key falls through so prototype and getter resolution are untouched. Writes degrade the object layout to full-visit (mark_object_dynamic_shape_unknown) before the barriered slot store, and nothing on the lane allocates from the arena, so raw pointers need no rooting handles. Fiber micro-benchmark (200k 23-field ctor objects, hot loops; node ~30ms per loop): overwrite-heavy access loop 26.3s -> 10.3-17.2s (load-dependent, -35..-61%), alternate-link loop 2.8s -> 0.8-0.9s (-67%), inline-field loop 11.7s -> 6.2-6.4s (-47%). Roughly 3x on fiber-shaped access vs the pre-round-1 baseline. Differential tests byte-identical to node: existing test_gap_prop_plan_cache_invalidation.ts plus new test_gap_field_lane_semantics.ts (delete under a warm plan, re-add, inherited class getter after warm own-reads, hot getter/setter loop). perry-runtime suite 1328/0 single-threaded. addr-class + GC store-site audits green (canonical is_above_handle_band predicates).
Codegen emits js_put_value_set for a large class of `obj.f = v` stores (receiver-based OrdinarySet), which runs ordinary_set_with_receiver. That function has its own PerryTS#5054 fast path, but for class instances it re-ran the full O(prototype-chain) interception walk (class_instance_set_may_intercept) on EVERY call — the store-plan cache added in PerryTS#6532 was only consulted from js_object_set_field_by_name, so this dominant path never benefited. Profiling a fiber-churn workload showed class_instance_set_may_intercept under js_put_value_set as a top frame, and the compiled TUI's per-keystroke store loops go through this path (which is why PerryTS#6532/PerryTS#6539 left TUI keystroke latency unmoved). Consult the shared store-plan cache in the class-instance branch before the interception walk. The verdict is the identical `!class_instance_set_may_intercept(class_id, key)` that js_object_set_field_by_name already memoizes, so a hit recorded on either path serves the other. Eligibility matches the store lane: the existing SLOW_FLAGS gate already excludes frozen/sealed/non-extensible and own-descriptor objects; add the per-instance chain-divergence flags (OBJ_FLAG_PROTO_OVERRIDE / OBJ_FLAG_NULL_PROTO) and the native-module class id, since a cached class-chain verdict is only valid for instances whose chain matches the class chain. The key is interned through the new shared object::interned_key_ptr helper so both paths key on the same canonical pointer. Fiber micro-benchmark (200k 23-field instances, store-heavy loops kept live so GC is not a factor): overflow-field store loop 10085 -> 5881 ms (-42%), alternate-link loop 816 -> 500 ms (-39%), inline-field store loop 5674 -> 3222 ms (-43%), all beyond PerryTS#6539. Differential test test_gap_put_value_plan_cache.ts is byte-identical to node: inherited setter fires on receiver-set after the plan warms, a plain data field stays a plain store, a per-instance setPrototypeOf override bypasses the class plan, and a prototype accessor installed after warm-up intercepts fresh instances. perry-runtime suite green single-threaded; addr-class + file-size audits pass. Note: the value returned by the fast path is unchanged; only the duplicate vet is skipped — the actual store still flows through target_set -> js_object_set_field_by_name.
📝 WalkthroughWalkthroughProperty access gains guarded read and write fast lanes for plain arena objects, backed by epoch-invalidated property-plan caches. Deletes invalidate read plans, proxy writes cache interception decisions, and new tests cover deletion, accessors, prototype changes, and warmed assignments. ChangesProperty fast lanes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_object_get_field_by_name
participant prop_plan
participant ObjectStorage
Caller->>js_object_get_field_by_name: read property
js_object_get_field_by_name->>prop_plan: lookup keys-array and interned-key plan
prop_plan-->>js_object_get_field_by_name: cached index or miss
js_object_get_field_by_name->>ObjectStorage: read inline or overflow field
ObjectStorage-->>Caller: property value
sequenceDiagram
participant Caller
participant ordinary_set_with_receiver
participant prop_plan
participant js_object_set_field_by_name
Caller->>ordinary_set_with_receiver: write property
ordinary_set_with_receiver->>prop_plan: check cached interception decision
prop_plan-->>ordinary_set_with_receiver: cached or computed verdict
ordinary_set_with_receiver->>js_object_set_field_by_name: perform eligible store
js_object_set_field_by_name-->>Caller: completed write
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/delete_rest.rs (1)
37-40: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftInvalidate plans only after a successful semantic mutation.
This unconditional bump also flushes all plans for missing keys, failed non-configurable deletes, and exotic/handle dispatches. Move it into successful mutation branches; ordinary key compaction should bump when the keys mapping is replaced.
🤖 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/delete_rest.rs` around lines 37 - 40, Move the prop_plan::prop_plan_epoch_bump call out of the unconditional delete path and into only those branches that successfully perform a semantic mutation; do not invalidate plans for missing keys, failed non-configurable deletes, or exotic/handle dispatches. Ensure ordinary key compaction bumps the epoch when the keys mapping is replaced.
🤖 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-runtime/src/object/mod.rs`:
- Around line 774-783: Update interned_key_ptr so the result of js_string_intern
is validated before returning it: inspect the returned pointer’s GcHeader and
return its address only when GC_FLAG_INTERNED is set; otherwise return 0.
Preserve the existing fast path for keys already marked interned and the current
null/address-band checks.
In `@crates/perry-runtime/src/object/prop_plan.rs`:
- Around line 275-284: Add a test-only shared Mutex in prop_plan.rs and acquire
its guard at the start of read_plan_roundtrip_and_epoch_flush and every other
test that calls prop_plan_epoch_bump or modifies vtable-generation state. Reuse
the same guard across all plan-state tests so they cannot run concurrently,
while leaving unrelated tests unguarded.
In `@test-files/test_gap_put_value_plan_cache.ts`:
- Around line 15-21: Update the test setup around class C and c2 so c2 does not
retain its constructor-created own k property before assigning through the
modified prototype. Remove or delete c2.k while preserving c1.k and the
inherited setter on c2, then keep the assignment and log assertions focused on
confirming the setter is invoked.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/delete_rest.rs`:
- Around line 37-40: Move the prop_plan::prop_plan_epoch_bump call out of the
unconditional delete path and into only those branches that successfully perform
a semantic mutation; do not invalidate plans for missing keys, failed
non-configurable deletes, or exotic/handle dispatches. Ensure ordinary key
compaction bumps the epoch when the keys mapping is replaced.
🪄 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: f80ad796-c657-47ec-82af-ced17d7d49ef
📒 Files selected for processing (8)
crates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/prop_plan.rscrates/perry-runtime/src/proxy.rstest-files/test_gap_field_lane_semantics.tstest-files/test_gap_put_value_plan_cache.ts
Comment on lines
+774
to
+783
| pub(crate) unsafe fn interned_key_ptr(key: *const crate::StringHeader) -> usize { | ||
| if key.is_null() || !crate::value::addr_class::is_above_handle_band(key as usize) { | ||
| return 0; | ||
| } | ||
| let gc_hdr = (key as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; | ||
| if (*gc_hdr).gc_flags & crate::gc::GC_FLAG_INTERNED != 0 { | ||
| key as usize | ||
| } else { | ||
| crate::string::js_string_intern(key, key_content_hash(key)) as usize | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not cache keys the interner declines to intern.
js_string_intern returns the original, unmarked pointer for over-limit strings. Treating that pointer as canonical lets a store-plan verdict follow mutable non-interned contents and potentially skip a setter or non-writable-property check. Return 0 unless the returned pointer actually has GC_FLAG_INTERNED.
Proposed fix
- crate::string::js_string_intern(key, key_content_hash(key)) as usize
+ let interned = crate::string::js_string_intern(key, key_content_hash(key));
+ if interned.is_null() {
+ return 0;
+ }
+ let gc_hdr = (interned as *const u8).sub(crate::gc::GC_HEADER_SIZE)
+ as *const crate::gc::GcHeader;
+ if (*gc_hdr).gc_flags & crate::gc::GC_FLAG_INTERNED != 0 {
+ interned as usize
+ } else {
+ 0
+ }📝 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
| pub(crate) unsafe fn interned_key_ptr(key: *const crate::StringHeader) -> usize { | |
| if key.is_null() || !crate::value::addr_class::is_above_handle_band(key as usize) { | |
| return 0; | |
| } | |
| let gc_hdr = (key as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; | |
| if (*gc_hdr).gc_flags & crate::gc::GC_FLAG_INTERNED != 0 { | |
| key as usize | |
| } else { | |
| crate::string::js_string_intern(key, key_content_hash(key)) as usize | |
| } | |
| let interned = crate::string::js_string_intern(key, key_content_hash(key)); | |
| if interned.is_null() { | |
| return 0; | |
| } | |
| let gc_hdr = (interned as *const u8).sub(crate::gc::GC_HEADER_SIZE) | |
| as *const crate::gc::GcHeader; | |
| if (*gc_hdr).gc_flags & crate::gc::GC_FLAG_INTERNED != 0 { | |
| interned as usize | |
| } else { | |
| 0 | |
| } |
🤖 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/mod.rs` around lines 774 - 783, Update
interned_key_ptr so the result of js_string_intern is validated before returning
it: inspect the returned pointer’s GcHeader and return its address only when
GC_FLAG_INTERNED is set; otherwise return 0. Preserve the existing fast path for
keys already marked interned and the current null/address-band checks.
Comment on lines
+275
to
+284
| #[test] | ||
| fn read_plan_roundtrip_and_epoch_flush() { | ||
| let keys = 0xAAAA_0040usize; | ||
| let key = 0xBBBB_0080usize; | ||
| read_plan_record(keys, key, 21); | ||
| assert_eq!(read_plan_lookup(keys, key), Some(21)); | ||
| assert_eq!(read_plan_lookup(keys, key + 8), None); | ||
| prop_plan_epoch_bump(); | ||
| assert_eq!(read_plan_lookup(keys, key), None); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby tests.
ast-grep outline crates/perry-runtime/src/object/prop_plan.rs --view expanded || true
echo
echo "=== Relevant test lines ==="
sed -n '1,360p' crates/perry-runtime/src/object/prop_plan.rs | cat -n
echo
echo "=== Search for plan epoch/vtable test locks or related globals ==="
rg -n "PLAN_TEST_LOCK|prop_plan_epoch_bump|vtable|epoch|Mutex" crates/perry-runtime/src/object/prop_plan.rs crates/perry-runtime/src -g '!target' || trueRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== file size ==="
wc -l crates/perry-runtime/src/object/prop_plan.rs
echo
echo "=== test module and global plan state ==="
sed -n '1,340p' crates/perry-runtime/src/object/prop_plan.rs | nl -ba | sed -n '1,340p'
echo
echo "=== concurrent-test / serial markers in this file ==="
rg -n "serial|Mutex|OnceLock|LazyLock|static .*Lock|test_threads|PLAN_TEST_LOCK|prop_plan_epoch_bump|vtable" crates/perry-runtime/src/object/prop_plan.rs || trueRepository: PerryTS/perry
Length of output: 301
Serialize the plan-state tests in crates/perry-runtime/src/object/prop_plan.rs. prop_plan_epoch_bump() and the vtable-generation bump touch shared global state, so these tests can make each other flaky when run in parallel. Add a test-only Mutex guard around the plan-state tests.
🤖 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/prop_plan.rs` around lines 275 - 284, Add a
test-only shared Mutex in prop_plan.rs and acquire its guard at the start of
read_plan_roundtrip_and_epoch_flush and every other test that calls
prop_plan_epoch_bump or modifies vtable-generation state. Reuse the same guard
across all plan-state tests so they cannot run concurrently, while leaving
unrelated tests unguarded.
Comment on lines
+15
to
+21
| class C { v = 1; k = 0; } | ||
| const c1 = new C(), c2 = new C(); | ||
| for (let i = 0; i < 50000; i++) { c1.k = i; } | ||
| const log: string[] = []; | ||
| Object.setPrototypeOf(c2, { set k(v: number){ log.push("ov:" + v); } }); | ||
| (c2 as any).k = 7; | ||
| out.push("s3=" + log.join(",") + "|" + c1.k); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Leave c2 without an own k property.
Because the constructor creates c2.k, JavaScript must update that own data property instead of invoking the inherited setter. The test therefore passes even if the divergence guard is broken.
Proposed fix
-class C { v = 1; k = 0; }
+class C { v = 1; }
const c1 = new C(), c2 = new C();
-for (let i = 0; i < 50000; i++) { c1.k = i; }
+for (let i = 0; i < 50000; i++) { (c1 as any).k = i; }
const log: string[] = [];
Object.setPrototypeOf(c2, { set k(v: number){ log.push("ov:" + v); } });
(c2 as any).k = 7;
-out.push("s3=" + log.join(",") + "|" + c1.k);
+out.push("s3=" + log.join(",") + "|" + (c1 as any).k);📝 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
| class C { v = 1; k = 0; } | |
| const c1 = new C(), c2 = new C(); | |
| for (let i = 0; i < 50000; i++) { c1.k = i; } | |
| const log: string[] = []; | |
| Object.setPrototypeOf(c2, { set k(v: number){ log.push("ov:" + v); } }); | |
| (c2 as any).k = 7; | |
| out.push("s3=" + log.join(",") + "|" + c1.k); | |
| class C { v = 1; } | |
| const c1 = new C(), c2 = new C(); | |
| for (let i = 0; i < 50000; i++) { (c1 as any).k = i; } | |
| const log: string[] = []; | |
| Object.setPrototypeOf(c2, { set k(v: number){ log.push("ov:" + v); } }); | |
| (c2 as any).k = 7; | |
| out.push("s3=" + log.join(",") + "|" + (c1 as any).k); |
🤖 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 `@test-files/test_gap_put_value_plan_cache.ts` around lines 15 - 21, Update the
test setup around class C and c2 so c2 does not retain its constructor-created
own k property before assigning through the modified prototype. Remove or delete
c2.k while preserving c1.k and the inherited setter on c2, then keep the
assignment and log assertions focused on confirming the setter is invoked.
proggeramlug
added a commit
that referenced
this pull request
Jul 18, 2026
…#6596) * test: gap test for #6595 repeated capture-class statics * fix(runtime): exclude class objects from the store-plan cache (#6595) The #6541 receiver-based [[Set]] records a store-plan verdict BEFORE the store flows through target_set -> js_object_set_field_by_name, so the very same write hits the mirror-free top fast lane there. For a per-evaluation CLASS OBJECT (what a capture-carrying class materializes as — bundled zod's entire ZodType family) that lane skips the #6530 mirror_class_object_static_write hook: from the second same-shaped class on, the shape-transition cache also hits, the post-class static (ZodX.create = ...) lands only on the class object's own fields, and ClassRef-world static dispatch misses — returning the receiver, so .optional() yielded the class and every ._def read blew up (errorMap/_getType/description). Class objects are now ineligible for the store-plan cache at all four gates (ordinary_set fast path, top fast lane, in-body check + record): their writes always take the mirroring completions, and their template cid — shared with their instances — can no longer conflate two different prototype chains in one cache line. Statics writes are cold; instances (OBJECT_TYPE_REGULAR) keep the fast lanes unchanged. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Problem
Codegen emits
js_put_value_setfor a large class ofobj.f = vstores (receiver-basedOrdinarySet), which runsordinary_set_with_receiver. That function has its own #5054 fast path, but for class instances it re-ran the full O(prototype-chain) interception walk (class_instance_set_may_intercept) on every call — the store-plan cache added in #6532 was only consulted fromjs_object_set_field_by_name, so this path never benefited.Empirically this is the store path that matters: profiling a fiber-churn workload shows
class_instance_set_may_interceptunderjs_put_value_setas a top frame, and the fiber micro-bench's overwrite loops go through it (which is why #6532/#6539's caching left those loops — and a compiled TUI's per-keystroke stores — only partly improved).Fix
Consult the shared store-plan cache in the class-instance branch before the interception walk. The verdict is the identical
!class_instance_set_may_intercept(class_id, key)thatjs_object_set_field_by_namealready memoizes, so a hit recorded on either path serves the other. Eligibility matches the store lane: the existingSLOW_FLAGSgate already excludes frozen/sealed/non-extensible and own-descriptor objects; this adds the per-instance chain-divergence flags (OBJ_FLAG_PROTO_OVERRIDE/OBJ_FLAG_NULL_PROTO) and the native-module class id, since a cached class-chain verdict is only valid for instances whose chain matches the class chain. The key is interned through a new sharedobject::interned_key_ptrhelper so both paths key on the same canonical pointer. The value returned is unchanged — only the duplicate vet is skipped; the actual store still flows throughtarget_set→js_object_set_field_by_name.Numbers
Fiber micro-benchmark (200k 23-field instances, store-heavy loops with all objects kept live so GC is not a factor), beyond #6539:
Scope note (honest): this helps store-bound workloads. A GC-heavy churn workload (rolling-window fiber allocation) is dominated by collector work, not the store vet, so this change alone does not move it materially — reclamation under churn is a separate, larger effort.
Verification
test-files/test_gap_put_value_plan_cache.tsis byte-identical to node: inherited setter fires on receiver-set after the plan warms, a plain data field stays a plain store, a per-instancesetPrototypeOfoverride bypasses the class plan, and a prototype accessor installed after warm-up intercepts fresh instances. perry-runtime suite 1328/0 single-threaded; addr-class + file-size audits pass; gap shards running.Summary by CodeRabbit
Performance
Bug Fixes
Tests