perf(runtime): gate property-tail registry probes by receiver header - #7897
Conversation
📝 WalkthroughWalkthroughThe property-get miss path now gates Symbol and Set registry probes. Cold dispatch helpers preserve Symbol and Set property behavior. Tests cover probe suppression, positive dispatch, headerless Symbols, and GC-allocated Symbols. ChangesProperty miss dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs (1)
18-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical pointer NaN-boxing helper.
Line 19 duplicates the pointer tag as
0x7FFD. Usecrate::value::js_nanbox_pointer(obj as i64)instead. This keeps the Symbol receiver aligned with the canonicalPOINTER_TAGencoding.Proposed fix
- let sym_f64 = - f64::from_bits(0x7FFD_0000_0000_0000u64 | (obj as u64 & 0x0000_FFFF_FFFF_FFFF)); + let sym_f64 = crate::value::js_nanbox_pointer(obj as i64);As per coding guidelines, “objects/pointers use
POINTER_TAG; preserve raw bits withf64::from_bits, not numericas f64conversion.”🤖 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/field_get_set/probe_dispatch.rs` around lines 18 - 23, Update the Symbol receiver setup in the field probe dispatch to use crate::value::js_nanbox_pointer(obj as i64) instead of manually constructing the 0x7FFD NaN-boxed bits. Preserve the resulting raw-bit handling when passing the value to js_symbol_description.Source: Coding guidelines
🤖 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/field_get_set/get_field_by_name_tail.rs`:
- Around line 454-464: Update the symbol check around may_be_symbol_header to
first validate obj with is_plausible_heap_addr, then perform the SYMBOL_MAGIC
read and registry lookup. Preserve support for both GC-backed and Box-leaked
Symbols by using the plausibility predicate rather than requiring a GC header.
In `@crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs`:
- Around line 42-79: Update the Set property probe around the registered-key
handling so unknown Set keys return None rather than Some(undefined), allowing
map_set_instance_property to continue prototype and instance-property lookup.
Preserve the existing size, known-method, and exotic-expando behavior while
changing only the final fallback for unrecognized keys.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs`:
- Around line 18-23: Update the Symbol receiver setup in the field probe
dispatch to use crate::value::js_nanbox_pointer(obj as i64) instead of manually
constructing the 0x7FFD NaN-boxed bits. Preserve the resulting raw-bit handling
when passing the value to js_symbol_description.
🪄 Autofix
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: 33c5bc46-8992-4b55-af79-aba7c5c036ce
📒 Files selected for processing (6)
changelog.d/7897-header-gated-property-tail.mdcrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_probe_tests.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/field_get_set/probe_dispatch.rsscripts/addr_class_ratchet_baseline.txt
|
Also applied the review nit: Symbol receiver construction now uses the canonical |
|
Because #7897 merged while these review fixes were being rebuilt and measured, the fixes could not enter the merged commit. They are now isolated in follow-up #7898 with the failing-before prototype regression, implausible-address guard coverage, full runtime validation, and the final quiet-mini A/B. |
Summary
SYMBOL_MAGICbefore consulting the process-global registryGcHeaderonce and consultSET_REGISTRYonly forGC_TYPE_SETReproduction
The issue's old
pipeline_bigattribution is stale on currentmain: a fresh debug-symbol sample contains zerois_registered_symbol_slowsamples in either arm. The structural bug is still reproducible without timing noise, though. With both registry latches armed, the new plain-object miss test fails before this change becauseSET_REGISTRYmoves from 801 to 803; after the change neither Set nor Symbol probe counters move. The companion test sabotages the Symbol magic screen after dispatch and verifies that Set plus both Symbol storage classes still use their authoritative registries.Performance
An initial inline version grew
get_field_by_name_object_tailby 40 bytes and regressedpipeline_bigby +4.636% paired geomean (30 alternating quiet-M1-mini pairs, 95% CI +4.589% to +4.681%). That version was rejected.The final reviewed version moves the cold registered-receiver bodies out of line. Against the same base compiler/runtime and correctness oracle, 30 alternating pairs on the locked quiet M1 mini measured:
556260000 3 3Validation
RUST_TEST_THREADS=1 cargo test -p perry-runtime get_field_by_name_probe_tests(2 passed)RUST_TEST_THREADS=1 cargo test -p perry-runtime(2,154 passed, 4 ignored; doc tests 6 ignored)python3 scripts/check_test_registration.pypython3 scripts/check_thread_locals.pypython3 scripts/addr_class_inventory.pycargo fmt --checkgit diff --checkbash scripts/check_file_size.shReview coverage also rejects an implausible upper-bound receiver before reading
SYMBOL_MAGICand proves an unknown Set key falls through to a data property installed onSet.prototypeinstead of being swallowed asundefined.The address-class audit passes and also reports the pre-existing stale
dyn_index.rsratchet entry (baseline 2, found 1); this PR does not broaden into that adjacent cleanup.Closes #7867