fix(gc): a relocated non-object owner must keep its explicit prototype (#10493) - #10552
proggeramlug wants to merge 2 commits into
Conversation
PerryTS#10362 follow-up) Base: e6dcb62 (main, v0.5.1587). `Object.setPrototypeOf` stores a shaped object's prototype in that object's meta record and everything else — every receiver `meta_capable_object` turns away — in the residual address-keyed registry (`object::prototype_chain`). That entry owes the collector two things: a rekey when the owner's address changes, and its value treated as a child edge so the prototype is retained and rewritten. Both were wired to two kinds by hand: the rekey to ordinary objects (the `ObjectOverflowFields` move hook) and to arrays (below the layout-kind return in the relocation funnel), the value visit to the Array and Object arms of the rewrite descriptor. The registry's population is not those two kinds. A lazy JSON array, Map, Set, Error, Promise, Date, RegExp or Temporal cell reaches the recorder through `Object.setPrototypeOf`, and a closure through `dyn_eval`. Every one of those is movable, none was rekeyed, and none had its prototype value traced. So the entry stayed under the address the owner had just left, the dead-owner prune dropped it on the next collection, and the prototype was gone: var a = JSON.parse(text); // >= 1 KB top-level array: lazy Object.setPrototypeOf(a, proto); Object.getPrototypeOf(a) === proto // true, then false after one minor Reported by CodeRabbit against PerryTS#10381 for `GC_TYPE_LAZY_ARRAY`. It is older than PerryTS#10381 — the pre-PerryTS#10381 funnel returns on the same layout-kind check — and it is not confined to lazy arrays: a runtime survey of every movable kind found Map, Set, Error, Promise, Date and RegExp losing the entry the same way, with arrays and ordinary objects as the controls that kept theirs. Both obligations now follow the registry's population, which `prototype_chain::residual_prototype_owner_type` states once: every kind except the four that can never be a receiver (strings and bigints are primitives, meta records and compiled regex programs are internal). * `gc/layout/transfer.rs` rekeys before the layout-kind return, for every owner kind, behind the registry's own latch. The array-arm and move-hook copies are deleted, so there is one home instead of two partial ones. * `gc/layout_slot_visit.rs` emits the recorded value as a child edge ahead of the kind arms — no arm's early return can skip it — for the same population. * Rekeying alone would have been worse than the bug: the entry would follow the owner while still naming the prototype's pre-collection address. The survey measured exactly that between the two halves. `gc/tests/residual_prototype_relocation.rs` is the witness. One test drives a nursery lazy array through the real `Object.setPrototypeOf` and a real copying minor that provably moves both it and its prototype; the other runs every movable owner kind with the prototype held by nothing but the registry entry, so it also pins retention. Both fail on the parent commit, and each half of the fix has its own sabotage: removing the rekey fails them at "the registry entry did not follow its owner", restricting the value visit to arrays and objects fails them at "the recorded prototype still names its pre-collection address". instructions:u, min of 5, base vs this: gc3 11,755,950,680 -> 11,770,591,001 +0.12% w1000 1,045,688,901 -> 1,046,210,601 +0.05% w5000 1,886,469,457 -> 1,888,582,673 +0.11% w20000 4,727,860,476 -> 4,735,365,693 +0.16% oldyoung 1,454,672,934 -> 1,455,225,497 +0.04% alloc-only 320,198,430 -> 320,203,034 +0.00% protoreloc 1,519,990,848 -> 1,521,552,668 +0.10% (and correct only here) latched 1,913,984,987 -> 1,927,027,915 +0.68% `latched` is the priced case: a program that has re-prototyped a non-object at all, churning Errors, Maps and Dates. Every traced cell of an owner-capable kind then takes the registry's global mutex and a SipHash probe, which is what arrays and ordinary objects have always paid. Removing that price means giving the exotic cells their prototypes back in their own meta records (they all have one since PerryTS#8891) instead of in an address-keyed table — a storage change worth its own design pass, not this fix.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe GC now relocates residual explicit-prototype registry entries for all eligible movable owner types. Slot rewriting also retains and updates recorded prototype values. New copying-minor tests cover lazy arrays and other movable receiver kinds. ChangesResidual prototype relocation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CopyingMinor
participant layout_transfer
participant PrototypeRegistry
participant SlotVisitor
CopyingMinor->>layout_transfer: Move owner to new address
layout_transfer->>PrototypeRegistry: Rekey residual prototype entry
CopyingMinor->>SlotVisitor: Visit moved object slots
SlotVisitor->>PrototypeRegistry: Rewrite recorded prototype address
Merge Risk: ⚪ Minimal · up to The change preserves explicit prototypes across copying-minor collection for affected receiver types, with focused coverage for rekeying and prototype retention. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 7 files. (1 skipped: 1 unsupported.)
✨ 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 |
Independent re-verification at a newer merge-base (
|
| fixture | base | fix |
|---|---|---|
protoreloc (the #10493 repro), no-auto and auto |
MISMATCH — after lazy=false error=false promise=false date=false regexp=false |
IDENTICAL |
latched (no-auto and auto) |
IDENTICAL | IDENTICAL |
gc3, w1000, w5000, w20000, oldyoung, alloc |
IDENTICAL | IDENTICAL |
The bug reproduces under the default auto-optimize path as well as PERRY_NO_AUTO_OPTIMIZE=1, and the fix closes it in both.
Tests. cargo test --release -p perry-runtime -- --test-threads=1 on the fix: 3969 passed, 1 failed — gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds, which I then ran on the base arm at this same merge-base: it fails there too (heap_generation.rs:285), so it is pre-existing and not owned by this PR.
The witness can fail. Both new tests, dropped unchanged into the base tree, fail there:
test_lazy_array_explicit_prototype_survives_a_copying_minor ... FAILED
assertion `left == right` failed: the registry entry must follow the lazy header
to its new address and name the prototype at ITS current address
left: None right: Some(9222534149130289248)
test_residual_prototype_owners_of_every_movable_kind_survive_a_copying_minor ... FAILED
lazy array: the registry entry did not follow its owner to the new address
instructions:u, 5 alternating runs, min, no-auto binaries:
| fixture | base | fix | delta |
|---|---|---|---|
| gc3 | 12,036,890,607 | 12,019,794,616 | -0.142% |
| w1000 | 1,060,140,917 | 1,059,651,231 | -0.046% |
| w20000 | 4,836,398,853 | 4,825,853,798 | -0.218% |
| oldyoung | 1,537,332,238 | 1,535,984,247 | -0.088% |
| alloc-only | 322,948,098 | 322,953,518 | +0.002% |
| protoreloc | 1,588,144,841 | 1,590,462,125 | +0.146% |
| latched | 2,363,769,576 | 2,379,380,673 | +0.660% |
latched reproduces this PR's priced case to within 0.02pp of its reported +0.68%. The unlatched fixtures came out slightly negative here rather than the +0.04…+0.16% reported at the older base — same direction of effect as the inline(always) annotations, and in any case not a regression on the axis that could have been one.
Review notes, having read the change rather than only run it — three things I checked because they are what the deleted code used to guarantee:
- Deleting the rekey from the
ObjectOverflowFieldsmove hook is safe only if the funnel runs on every path that reaches that hook. It does: all threegc_type_after_payload_movecall sites (copying.rs:608,oldgen.rs:1506,oldgen.rs:1635) are preceded bylayout_transferon the same pair, andarray/push_pop.rs:216covers growth. - Reading
obj_typefrom the source header insidelayout_transferis sound at the two old-gen sites that callset_forwarding_addressbefore it — forwarding overwrites the first user word, not the header'sobj_type, which is exactly what the adjacentgc_type_after_payload_move((*header).obj_type, …)already relies on. - The hoisted value visit reaches marking, not only the rewrite pass:
trace.rs:1752'strace_heap_rewrite_slotsgoes throughvisit_gc_rewrite_slot_descriptors, so retention holds for every owner kind — which is what the second test pins by holding the prototype through nothing but the registry entry. Pointer-free kinds (Temporal) are not skipped on this path;gc_type_is_pointer_freegates only black-birth seeding and a barrier fast path.
Nothing to change from my side.
|
Landed via merge train #10578 (v0.5.1593). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Fixes #10493. Based on main
e6dcb6274.The bug
An explicit
[[Prototype]]set on a non-meta-capable receiver is silently lost when the collector moves that receiver.Object.getPrototypeOfthen answers the wrong value and inherited methods disappear, with exit code 0 and no warning. Seven kinds are affected — lazy JSON array, Map, Set, Error, Promise, Date, RegExp — with arrays and ordinary objects as controls that keep theirs. Temporal cells anddyn_evalclosures are in the same population by construction.Correct before a collection, wrong after, with nothing in the program touching the objects in between. Repro and the full seven-kind table are in #10493.
Not a regression: the pre-#10381 funnel returns on the same check. Credit to CodeRabbit, which flagged the lazy-array half on #10381 after it merged; the population turned out to be wider.
Two independent holes, both fixed here
layout_transferreachedobject_static_prototype_owner_movedonly below its layout-kind early return, which aGcLayoutSlotKind::Nonecell never passes. The entry kept naming the pre-move address.Fixing only the first is worse than fixing neither — it converts "prototype silently lost" into "entry names a stale address". That intermediate state was measured, which is why both halves are in one commit.
The shape of the fix
Both obligations now follow the registry's population, stated once in
prototype_chain::residual_prototype_owner_type— everything except strings, bigints, meta records and compiled regex programs — instead of being wired to two kinds by hand:#[cold]call. The array-arm and move-hook copies are deleted, so there is one home instead of two partial ones;CodeRabbit's suggestion (migrate from the
LazyArrayTapemove hook) fixes one kind of seven and does not touch the value half, so this takes the early return itself as the defect.Verification
JS repro: main prints
after lazy=false error=false promise=false date=false regexp=false; with this PR it matches node exactly.Sabotage, both reverted: removing the rekey fails both tests at "the registry entry did not follow its owner"; restricting the value visit back to arrays and objects fails them at "the recorded prototype still names its pre-collection address". Each half has its own witness.
The unit test asserts every premise before the assertion that matters — the array is nursery-born and movable, it is not meta-capable, the entry really landed in the residual registry, and
getPrototypeOfresolved it before the collection — then asserts the address actually changed. Otherwise it would pass without testing anything.Gates (fix vs base, both at
e6dcb6274): outputs node-identical on all eight fixtures (protorelocmismatches on main — that is the bug) ·PERRY_GC_FROMSPACE_SCAN_ABORT=1clean everywhere with thefcd108bfbpositive control still aborting · seeded stress 20 seeds × 4 fixtures,seed_stress_failures=0· runtime suites base 3966/1 and fix 3968/1 with the same pre-existing failure · 14 gc integration targets 13-ok/1-failed on both arms (pre-existing) · fmt · clippy 881 both, identical sets · file-size, root-holders (unchanged — the fix caches nothing), rekeyed-key-tables, store-site, addr-class and thread-local audits all match base.Cost
+0.04 % to +0.16 % instructions on fixtures that never arm the registry; +0.68 % on a fixture that arms it and churns Errors/Maps/Dates — the registry's global mutex plus a SipHash probe per traced owner-capable cell, which is what arrays and ordinary objects have always paid.
A first draft cost +1.4 % on gc3: an inner generic function created
&mut Fshims in the copying scan and the funnel stopped inlining. The committed shape avoids that.Follow-ups, deliberately not here (both in #10493)
getPrototypeOfon a registered Map or Set answers the builtin prototype before consulting the registry, so a custom prototype there is invisible to reflection even with no collection involved. Pre-existing and untouched.Summary by CodeRabbit
Bug Fixes
Tests