chore(codegen,runtime): repsel Phase 4b cleanup — delete the dead PERRY_UNBOXED_OBJECT_FIELDS prototype - #7485
Merged
Conversation
added 3 commits
August 6, 2026 07:25
…rototype (repsel Phase 4b cleanup)
The prototype's write path was bit-identical to the default typed-shape
path and its read side was never implemented. Deletes the env flag, the
hard-coded {x,y} object-literal matcher, js_gc_init_unboxed_object_layout,
js_object_{get,set}_unboxed_f64_field, and the flag's cache-key entries.
The js_typed_feedback_object_set_unboxed_f64_field sentinel symbol stays
(check_runtime_symbols.sh + keepalive anchor surface, #854 foundation);
its guarded fast path now uses the plain indexed setter, which routes
through the identical runtime_store_jsvalue_slot.
GC layout tests that used the prototype installer as a convenience move
onto the load-bearing js_gc_init_typed_shape_layout; prototype-only twins
of already-covered typed-shape tests are dropped.
📝 WalkthroughWalkthroughThe PR removes the Phase 4b unboxed object-field prototype, including compiler lowering, runtime APIs, GC layout initialization, and cache wiring. Typed feedback now uses boxed numeric values with indexed stores. GC tests use typed-shape layout initialization. ChangesUnboxed object field removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/gc/tests/layout_trace/object_layout_invalidation.rs`:
- Around line 227-239: Update the affected shape-invalidation tests, including
test_typed_object_dynamic_added_property_falls_back and the related
delete/define-property cases, to root obj, keys, and descriptor objects in
RuntimeHandleScope before allocator calls, then reload their raw pointers before
object operations. Add forced moving-GC coverage for each allocation-sensitive
path and preserve the existing invalidation assertions.
🪄 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: 75dd1862-0d49-4dbf-9bdc-f96af3dd7b61
📒 Files selected for processing (19)
changelog.d/7485-repsel-4b-field-store-elision.mdcrates/perry-codegen/src/expr/object_literal.rscrates/perry-codegen/src/gc_call_effects.rscrates/perry-codegen/src/runtime_decls/arrays.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/tests/typed_shape_descriptors.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/tests/layout_trace.rscrates/perry-runtime/src/gc/tests/layout_trace/object_layout_invalidation.rscrates/perry-runtime/src/gc/tests/layout_trace/typed_shape.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_get_set/field_ops.rscrates/perry-runtime/src/typed_feedback.rscrates/perry-runtime/src/typed_feedback/tests.rscrates/perry/src/commands/compile/build_cache.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/object_cache/object_cache_tests.rsscripts/addr_class_ratchet_baseline.txt
💤 Files with no reviewable changes (7)
- crates/perry-codegen/src/gc_call_effects.rs
- crates/perry/src/commands/compile/build_cache.rs
- crates/perry-runtime/src/gc/layout.rs
- crates/perry-codegen/src/runtime_decls/objects.rs
- crates/perry/src/commands/compile/object_cache/object_cache_tests.rs
- crates/perry-codegen/src/runtime_decls/arrays.rs
- crates/perry-runtime/src/object/field_get_set/field_ops.rs
Comment on lines
+227
to
239
| fn test_typed_object_dynamic_added_property_falls_back() { | ||
| clear_marks(); | ||
| clear_mark_seeds(); | ||
|
|
||
| let obj = unboxed_point_for_shape_change_test(86_101); | ||
| let obj = typed_point_for_shape_change_test(86_101); | ||
| let z_key = crate::string::js_string_from_bytes(b"z".as_ptr(), 1); | ||
| crate::object::js_object_set_field_by_name(obj, z_key, 3.0); | ||
|
|
||
| assert_eq!( | ||
| test_layout_pointer_slot_count(obj as usize, 3), | ||
| None, | ||
| "adding a dynamic property must invalidate the exact unboxed shape" | ||
| "adding a dynamic property must invalidate the exact typed shape" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline / size =="
wc -l crates/perry-runtime/src/gc/tests/layout_trace/object_layout_invalidation.rs
sed -n '1,340p' crates/perry-runtime/src/gc/tests/layout_trace/object_layout_invalidation.rs
echo "== search handle scope and gc register APIs =="
rg -n "RuntimeHandleScope|gc_register_mutable_root_scanner|mark_seeds|clear_mark_seeds|test_layout_pointer_slot_count|js_object_set_field_by_name|js_string_from_bytes|js_closure" crates/perry-runtime/src/gc crates/perry-runtime -g '*.rs' | head -n 250Repository: PerryTS/perry
Length of output: 40227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gc module files and relevant api definitions =="
for f in $(git ls-files 'crates/perry-runtime/src/gc/*.rs' 'crates/perry-runtime/src/gc/**/*.rs' | sort); do
echo "--- $f"
rg -n "struct RuntimeHandleScope|impl RuntimeHandleScope|fn new\\(|gc_register_mutable_root_scanner|js_string_from_bytes|js_object_set_field_by_name|js_object_delete_field|js_object_define_property|root_nanbox_f64|force_schedule" "$f" || true
done
echo "== inspect object/string API headers =="
sed -n '1,230p' crates/perry-runtime/src/object/mod.rs
echo "== inspect string allocation header =="
rg -n "pub fn js_string_from_bytes|fn js_string_from_bytes|StringHeader|gc_malloc|mark_seed|RootScope|RuntimeHandleScope" crates/perry-runtime/src/string -g '*.rs' | head -n 200
sed -n '1,180p' crates/perry-runtime/src/string/mod.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gc module files and relevant api definitions =="
for f in $(git ls-files 'crates/perry-runtime/src/gc/*.rs' 'crates/perry-runtime/src/gc/**/*.rs' | sort); do
echo "--- $f"
rg -n "struct RuntimeHandleScope|impl RuntimeHandleScope|fn new\\(|gc_register_mutable_root_scanner|js_string_from_bytes|js_object_set_field_by_name|js_object_delete_field|js_object_define_property|root_nanbox_f64|force_schedule" "$f" || true
done
echo "== inspect object/string API headers =="
sed -n '1,260p' crates/perry-runtime/src/object/mod.rs
echo "== inspect string allocation header =="
rg -n "pub fn js_string_from_bytes|fn js_string_from_bytes|StringHeader|gc_malloc|mark_seed|RootScope|RuntimeHandleScope" crates/perry-runtime/src/string -g '*.rs' | head -n 200
sed -n '1,180p' crates/perry-runtime/src/string/mod.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime_handles.rs =="
cat -n crates/perry-runtime/src/gc/roots/runtime_handles.rs
echo "== string_from_bytes implementations =="
sed -n '453,625p' crates/perry-runtime/src/string/mod.rs
sed -n '420,460p' crates/perry-runtime/src/string/base64_codec.rs
echo "== object field_by_name/delete/define implementations references =="
rg -n "pub.*fn js_object_set_field_by_name|fn js_object_set_field_by_name|pub.*fn js_object_delete_field|fn js_object_delete_field|pub.*fn js_object_define_property|fn js_object_define_property|gc_force_schedule|GcTriggerKind|GcTriggerSnapshot|TestFfiMutableRootSlots|GIL_SCHEME|GC_INCR_ENABLED" crates/perry-runtime/src/object -g '*.rs' | head -n 200
echo "== force schedule candidates in tests =="
rg -n "GcTriggerKind|force_schedule|RuntimeHandleScope|root_nanbox_f64|root_string_ptr|root" crates/perry-runtime/src/gc/tests/runtime_roots.rs crates/perry-runtime/src/gc/tests/layout_trace/object_layout_invalidation.rs -g '*.rs'
echo "== reading focused object ops around set/delete/define public functions =="
fd 'field_set_by_name\.rs|delete_rest\.rs|descriptors\.rs|object_ops\.rs' crates/perry-runtime/src/object -x sh -c 'echo "--- $1"; rg -n "js_object_set_field_by_name|js_object_delete_field|js_object_define_property|GC_TRIGGERS|gc_force_schedule" "$1"' sh {}Repository: PerryTS/perry
Length of output: 29564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== field_set_by_name.rs relevant source =="
sed -n '1,140p' crates/perry-runtime/src/object/field_set_by_name.rs
rg -n "arena_alloc_gc|gc_force_schedule|GcTrigger|GcTriggerKind|gc_try_force|maybe" crates/perry-runtime/src/object/field_set_by_name.rs crates/perry-runtime/src/gc -g '*.rs' | head -n 200
echo "== delete_rest.rs relevant source =="
sed -n '1,110p' crates/perry-runtime/src/object/delete_rest.rs
sed -n '1,80p' crates/perry-runtime/src/object/descriptors.rs
rg -n "arena_alloc_gc|gc_force_schedule|GcTriggerKind|maybe_trigger|GcTrigger|alloc" crates/perry-runtime/src/object/delete_rest.rs crates/perry-runtime/src/object/descriptors.rs | head -n 200
echo "== define_property.rs relevant source =="
sed -n '260,330p' crates/perry-runtime/src/object/object_ops/define_property.rs
rg -n "arena_alloc_gc|gc_force_schedule|GcTriggerKind|maybe_trigger|GcTrigger|alloc" crates/perry-runtime/src/object/object_ops/define_property.rs | head -n 200
echo "== focused gc triggering/source =="
rg -n "fn arena_alloc_gc|pub fn arena_alloc_gc|arena_alloc_gc_longlived|fn gc_force_schedule|pub fn gc_force_schedule|GcTriggerKind::Direct|GcTriggerSnapshot|force_schedule" crates/perry-runtime/src -g '*.rs' | head -n 240
sed -n '1,120p' crates/perry-runtime/src/arena.rs
sed -n '440,520p' crates/perry-runtime/src/gc/roots.rs
sed -n '430,455p' crates/perry-runtime/src/gc/tests/runtime_roots.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== object set_by_name tail rooting source =="
sed -n '140p' crates/perry-runtime/src/object/field_set_by_name.rs
fd 'tail\.rs' crates/perry-runtime/src/object/field_set_by_name -x sh -c 'echo "--- $1"; sed -n "1,260p" "$1"' sh {}
echo "== object delete allocation/source =="
sed -n '290,350p' crates/perry-runtime/src/object/delete_rest.rs
echo "== object define property allocation/source around tail =="
sed -n '330,530p' crates/perry-runtime/src/object/object_ops/define_property.rs
echo "== find runtime_handle_scope_new definitions at crate::gc == "
rg -n "impl RuntimeHandleScope|pub fn new\\(\\)" crates/perry-runtime/src -g '*.rs'
echo "== exact gc force schedule candidate source snippets =="
rg -n "maybe_trigger|maybe_schedule|direct_gc_force|gc_collect_minor|gc_collect_full|arena_alloc_gc" crates/perry-runtime/src/gc -g '*.rs' | head -n 220Repository: PerryTS/perry
Length of output: 50369
Root these objects across allocator calls.
Line 231 stores obj only as a raw pointer before js_string_from_bytes allocates z_key. Lines 250-252 do the same before allocate-shifting delete. Lines 269-271 and 295-307 do the same before js_object_define_property allocates descriptor/key objects. A moving arena allocation can relocate these values after the raw pointer is captured.
Keep obj, key, and descriptor objects in RuntimeHandleScope before each allocation, and reload the raw pointers before the object operations. Add a forced moving-GC check for these shape-invalidation paths.
Also applies to: 245-262, 264-287, 290-316.
🤖 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/layout_trace/object_layout_invalidation.rs`
around lines 227 - 239, Update the affected shape-invalidation tests, including
test_typed_object_dynamic_added_property_falls_back and the related
delete/define-property cases, to root obj, keys, and descriptor objects in
RuntimeHandleScope before allocator calls, then reload their raw pointers before
object operations. Add forced moving-GC coverage for each allocation-sensitive
path and preserve the existing invalidation assertions.
Sources: Coding guidelines, Learnings
Contributor
Author
|
Reviewed and merged. The central finding — that both brief items already landed as #6919 nine days ago — is verified and important: the prompt file and today's engine-plan both said otherwise, and I'll correct the plan (the "in flight" line was mine). Declining to re-implement and shipping only the sanctioned cleanup was exactly right. Verified on a rebase over current tip: runtime 1715/0, codegen 641/0, the retained sentinel's guard test passes unmodified, fmt clean, addr-class pass with the ratchet tightened, and the two residual mentions of the deleted flag are historical doc comments. The kill-policy rationale holds — a default-off prototype with an unimplemented read side is exactly "a decision that hasn't been made", and the sentinel/keepalive split keeps what the release gate needs. Also credit for the three main-tip findings in your report (debt ceilings red from the #7467 batch, |
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.
Phase 4b (narrow) of
docs/representation-selection-rfc.md§5.7 — the remaining scoped item.Context: where Phase 4b stands
The two mandatory Phase 4b items are already on
mainvia #6919 (with the #6921 ctor-exit follow-up closed by #6930):Ptr<Shape>-proven receiver elidejs_gc_note_slot_layoutwhen the value is non-pointer-by-construction andjs_string_addref_if_heap_stringwhen the value provably cannot be a heap string (expr/property_set.rs,expr/write_barrier.rs,expr/helpers.rs). The generational write barrier is untouched.runtime_store_jsvalue_slotcanonicalizes INT32-boxed numeric stores into raw-f64-masked slots (gc/barrier.rs::canonicalize_typed_slot_store_bits), so one FFI integer no longer evicts an object's typed descriptor permanently.Per the Phase 4b recon, full field-unboxing was assessed and REJECTED (recorded in RFC §5.7):
numberfields are already bit-unboxed — NaN-boxing reserves only0x7FF9..=0x7FFF, so a number slot holds raw IEEE bits andraw_f64_maskis a proof bit, not a storage change, with Phase 3b having removed the read-side guard; raw string handles at rest would break SSO and buy nothing; and rawi1/i32slots would need a third GC mask plus a layout probe at ~25 hot direct slot-read sites (JSON.stringify,util.inspect, v8 IPC serde, descriptor reads).What this PR ships: the sanctioned Phase 4b cleanup
Deletes the dead
PERRY_UNBOXED_OBJECT_FIELDSprototype — its write path was bit-identical to the default typed-shape path and its read side was never implemented:{x,y}object-literal matcher + emission block (expr/object_literal.rs)js_gc_init_unboxed_object_layout(gc/layout.rs),js_object_{get,set}_unboxed_f64_field(object/field_get_set/field_ops.rs), their codegen declarations andgc_call_effectsentrypoint_literal_uses_typed_shape_pathKept deliberately:
js_typed_feedback_object_set_unboxed_f64_fieldstays — it is acheck_runtime_symbols.shsentinel, a keepalive anchor, and part of the chore: triage dead-code warnings (fields, variants, constants, dead assignments) #854 typed-feedback foundation. Its guarded fast path now uses the plain indexed setter (js_object_set_field), which routes through the identicalruntime_store_jsvalue_slot; observable behavior is unchanged and its guard test passes unmodified.js_gc_init_typed_shape_layoutuntouched — default-on and load-bearing.layout_trace/unboxed_object.rs→layout_trace/object_layout_invalidation.rs); the shape-change invalidation coverage (dynamic add / delete /defineProperty/ accessor) is preserved on the default path. Prototype-only twins of teststyped_shape.rs/object_closure_slots.rsalready carry are dropped.addr_classratchet baseline tightened for the deleted setter's bare-address site.No behavior change: the flag was default-off, never set in CI, and its write path emitted bit-identical stores.
Validation
Structural proof (branch compiler,
--trace llvm, probe: class withboolean/Foo|null/number[]/string/numberfields written in a loop on a proven receiver): the hot function emits 0js_gc_note_slot_layoutand 0js_string_addref_if_heap_string; the guarded ctor path keeps 4/4 — exactly the merged 4b.1 shape. The only IR difference vs a pristine-main build of the same probe is the three deleteddeclarelines; the emitted code is byte-identical.Full gap suite A/B (macOS, node v26.5.1 = the
.node-versionpin), self-measured pristineorigin/mainbaseline vs this branch, same session, same method (run_parity_tests.sh --filter test_gap_, prebuilt-archive mode) — run twice, once at base 2105558 and again after rebasing onto f06270d: zero changed outcomes across all 486 tests in both sweeps (each arm: 410 parity_pass / 10 parity_fail / 65 compile_fail / 1 crash, identical name sets; the parity_fails are the snapshot's standingbug-openentries, and the compile_fails are a local-environment artifact of the prebuilt ext-featured stdlib + missingnode_modules, byte-identical across arms).The merged 4b gap test
test_gap_repsel_p4b_field_store_elision.tsis byte-exact vs node 26.5.1 on this branch, default and underPERRY_GC_FORCE_EVACUATE=1.cargo test -p perry-codegen --lib641 passed;cargo test -p perry-runtime --lib1715 passed (includes the converted layout-invalidation tests and the typed-feedback wrapper test, unmodified);perrybin object-cache/build-cache tests green;cargo fmt --all --checkclean;check_runtime_symbols.sh101/101 sentinels on the built archive;raw_handle_debt.pyandaddr_class_inventory.pyoutputs byte-identical to pristine main (addr_classbaseline tightened by the deleted site).Perf (interleaved min-of-9, same host, 1-min load ~8.4-8.8 — the quiet-machine gate was not met, so treat as a no-regression check, not a perf claim): branch 1216 ms vs main 1229 ms on a 50M-iteration class-field store loop; branch ≤ base in all 9 rounds. A dead default-off flag deletion is expected to be perf-neutral.
Pre-existing on
mainat f06270d, unrelated to this PR (identical on a pristine-main checkout):typed_shape_descriptors::integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrierfails ("IR should contain block marker apush.numeric_fast.");check_file_size.shfails oncrates/perry-runtime/src/array/indexing.rs(2005 lines);raw_handle_debt.pyper-module ceilings fail onarray/indexing.rs(7>6),object/descriptors.rs(17>12), andobject/field_get_set/enumeration.rs(5 bare reads, no ceiling).Summary by CodeRabbit
Bug Fixes
Refactor
Tests