Skip to content

fix(gc): refresh obj at the remaining six class-static mirror sites - #7383

Merged
proggeramlug merged 1 commit into
mainfrom
fix/7341-mirror-remaining-sites
Aug 4, 2026
Merged

fix(gc): refresh obj at the remaining six class-static mirror sites#7383
proggeramlug merged 1 commit into
mainfrom
fix/7341-mirror-remaining-sites

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Completes #7381.

Correcting my own scoping call

#7381 fixed two of the eight mirror_class_object_static_write sites, and scoped itself there on the theory that refresh_roots_after_alloc!() — which republishes obj, key, value and interned_key together — could clobber an arm that rebinds value locally.

That theory was wrong. None of the eight arms rebinds any of the four after its handle is taken, so republishing is a no-op except for the relocation it repairs.

What made me narrow it was two gap tests failing alongside the full patch. Those two turned out to fail identically on pristine main — pre-existing, one already in known_failures.json. I attributed them to my change before checking the baseline, and narrowed a correct patch on that basis.

Measured, not inspected

pass fail
pristine main 58 2
all eight sites refreshed 58 2

Byte-identical, same two pre-existing failures.

Result

With all eight refreshed, test_gap_gc_assign_string_source_rooting's fault leaves mirror_class_object_static_write entirely and surfaces the next catch in the chain, at js_jsvalue_equals — still open. This test is a chain rather than a single defect, which is expected for one purpose-built to stress that path.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when updating object fields by name during memory allocation.
    • Ensured object, key, and value references remain valid when static properties are mirrored.
    • Covered all affected field-write paths to prevent issues caused by relocated references.
  • Documentation

    • Added a changelog entry documenting the completed fixes and test coverage.

Completes #7381, which fixed two of eight and scoped itself there on the
theory that refresh_roots_after_alloc!() -- which republishes obj, key,
value and interned_key together -- could clobber an arm that rebinds
value locally.

That theory was wrong. None of the eight arms rebinds any of the four
after its handle is taken, so republishing is a no-op except for the
relocation it repairs. Measured rather than inspected: the full-coverage
build scores 58 pass / 2 fail on the object/assign/class/field/shape gap
set, byte-identical to pristine main, with both failures pre-existing.

With all eight refreshed the fault leaves mirror_class_object_static_write
entirely and surfaces the next catch in the chain at js_jsvalue_equals.
@proggeramlug
proggeramlug merged commit 0f3182d into main Aug 4, 2026
15 of 40 checks passed
@proggeramlug
proggeramlug deleted the fix/7341-mirror-remaining-sites branch August 4, 2026 15:33
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8808cd51-38a2-44fc-9853-d659694b7538

📥 Commits

Reviewing files that changed from the base of the PR and between 37cd06d and 1d5c4c2.

📒 Files selected for processing (2)
  • changelog.d/7382-mirror-remaining-sites.md
  • crates/perry-runtime/src/object/field_set_by_name.rs

📝 Walkthrough

Walkthrough

This PR adds root pointer refreshes before mirror_class_object_static_write calls at six remaining call sites in js_object_set_field_by_name. Each refresh updates object, key, value, and interned-key pointers from their handles after allocator-capable writes. A changelog entry documents the completed fix and a newly exposed fault.

Changes

Root Pointer Refresh for Class Static Mirroring

Layer / File(s) Summary
Root refresh before mirroring
crates/perry-runtime/src/object/field_set_by_name.rs
Adds root refreshes for object, key, value, and interned-key pointers after sidecar, overflow, and inline write paths, before each of the six mirror_class_object_static_write calls.
Changelog entry
changelog.d/7382-mirror-remaining-sites.md
Documents completion of all eight refreshed call sites, validation results, and the next uncovered fault at js_jsvalue_equals.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • PerryTS/perry#6811: Extends the same root-refresh fix pattern applied earlier to write paths in field_set_by_name.rs.
  • PerryTS/perry#6941: Applies the same GC-rooting fix approach to js_object_set_field_by_name for receivers, keys, and values.
  • PerryTS/perry#7381: The originating fix this PR completes by covering the six remaining mirror_class_object_static_write call sites.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/7341-mirror-remaining-sites

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

proggeramlug added a commit that referenced this pull request Aug 4, 2026
I wrote both fragments before opening their PRs and guessed the next
number, so each was off by one: the mirror-remaining-sites work is #7383
(not 7382) and the size-arm rooting is #7385 (not 7384).

7384 is worse than cosmetic -- it is an open PR by another author, so the
fragment claimed a number already in use and would have collided when
that PR adds its own.

Fragment names are PR-keyed precisely so in-flight PRs never collide;
guessing the number defeats that. Open the PR first, then name the
fragment.

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug added a commit that referenced this pull request Aug 4, 2026
…7401)

* fix(lint): bring field_set_by_name.rs back under the 2000-line cap

#7381 and #7383 took the file from 1990 to 2048 lines, so `lint` -- a
REQUIRED check -- has been red on main, and every merge since has
bypassed it.

The overrun is comment volume, not code: eight refresh_roots_after_alloc!
call sites each carried a multi-line rationale block. The rationale now
sits once at the macro that implements it, which is where it belonged,
and the call sites are bare.

Worth recording how I got this wrong first: I checked the gate against
origin/main, saw identical output, and called it pre-existing. That
baseline already contained the two merges that caused it. The right
comparison is a commit from before the change.

* docs: name the fragment for its real PR (#7401)

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant