refactor(codegen): migrate instance_misc1 + logical_collections + map_set onto the Layer 1 rooting API (#7615) - #7627
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR increments the workspace version to ChangesCodegen rooting migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Lowering as Codegen lowering
participant Rooting as rooting API
participant GC as Garbage collector
participant Runtime as Runtime helper
Lowering->>Rooting: Lower and root operands
Rooting->>GC: Keep pointer and boxed slots valid
GC-->>Rooting: Re-read rooted values
Rooting->>Runtime: Emit call with materialized arguments
Runtime-->>Lowering: Return result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ 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 |
…_set onto the Layer 1 rooting API (#7615) (#7627) Slice 2. Adds two combinators with their callers: with_operands_rooted_across_call, for a window whose across step is an EMITTED runtime call and so cannot be derived from an Expr (re.test/re.exec's unconditional js_jsvalue_to_string_coerce), and with_rooted_accumulator, for a GC-managed value the lowering keeps updating while it lowers more user code. RootedSlot now carries its own representation so a boxed slot cannot be read as a raw pointer. Live bugs fixed in-slice: with-set materialising receiver AND interned key above the RHS (#7114 in a second arm), filter/some/every holding the array across the callback's js_closure_new, two unrooted RAW accumulators (Math.min/max variadic and fetch's static headers) that #7280 structurally cannot repair, fetch's three string operands across js_fetch_headers_to_json, and two string-literal reload sites. map_set.rs was already hand-rooted so its IR is byte-identical -- a translation, not a repair, and scoped as such. Deferred with reasons: IndexUpdate's intra-body window, which needs a per-use re-read combinator.
a66cfac to
b99df1e
Compare
…_set onto the Layer 1 rooting API (#7615) (#7627) Slice 2. Adds two combinators with their callers: with_operands_rooted_across_call, for a window whose across step is an EMITTED runtime call and so cannot be derived from an Expr (re.test/re.exec's unconditional js_jsvalue_to_string_coerce), and with_rooted_accumulator, for a GC-managed value the lowering keeps updating while it lowers more user code. RootedSlot now carries its own representation so a boxed slot cannot be read as a raw pointer. Live bugs fixed in-slice: with-set materialising receiver AND interned key above the RHS (#7114 in a second arm), filter/some/every holding the array across the callback's js_closure_new, two unrooted RAW accumulators (Math.min/max variadic and fetch's static headers) that #7280 structurally cannot repair, fetch's three string operands across js_fetch_headers_to_json, and two string-literal reload sites. map_set.rs was already hand-rooted so its IR is byte-identical -- a translation, not a repair, and scoped as such. Deferred with reasons: IndexUpdate's intra-body window, which needs a per-use re-read combinator.
b99df1e to
291248c
Compare
Audit before merge — verified, merged as v0.5.1357Rebased onto #7625 (determinism), which doubles as its confirmation: my Behaviour verified against node 26.5.1 on the migrated surface — Map/Set The Ledger sabotage re-verified on What makes this slice the strongest of the threeTwo raw accumulators that were not rooted at all — And the The honest-scoping standard held again: Two filed rather than smuggled: #7628 ( |
1 participant
Slice 2 of the Layer 1 campaign (#7615):
expr/instance_misc1.rs,expr/logical_collections.rsandlower_call/property_get/map_set.rsaremigrated end to end onto
crate::rootingand listed inMIGRATED_MODULES.Follows the template (#7617) and slices 1a (#7618) / 1b (#7620).
Two combinators, each arriving with its callers
with_operands_rooted_across_call— a window whoseacrossstep is anemitted runtime call rather than a lowered expression.
re.test(s)/re.exec(s)unconditionally emitjs_jsvalue_to_string_coerce, which allocatesand, on an object argument, dispatches a user
toString. There is noExprforany_may_trigger_gcto read, and deriving the window from thestringoperandanswers false for a plain local — dropping the root at exactly the site #7154
faults at. So the window is stated, not derived. The precedent is deliberate:
temp_root::guard_store_operand_acrossalready took aboolfor the samereason, since #7201.
operand_protectionstill decides how each operand isprotected; only the window's extent is stated, and it is stated conservatively.
All three
with_operands_rooted*forms now share one implementation(
with_operands_rooted_window), so the family cannot grow three orderings.with_rooted_accumulator— the operand group's mirror image. An operand islowered once and read once; an accumulator is written, read, rewritten and read
again with arbitrary user code lowered between the writes. It enforces the
invariant a raw handle cannot: the accumulator never exists as a register held
across an emission. Consuming calls re-read it as part of being emitted
(
call/call_void), a helper that returns a fresh address publishes it straightback (
advance), and the single point where a register escapes isfinish,which runs below the last collection point and above the release. Both closures'
?paths release.RootedSlotnow carries its ownRepr(Ptr/Boxed). A temp-root slot isrepresentation-agnostic, so before this the choice between
temp_root_get_i64and
temp_root_get_doublelived at each call site, where a mismatch is a silentmiscompile rather than a type error.
Live bugs found and fixed in-slice
with (o) { x = f() }(Expr::WithSet) materialised the receiver andthe interned property key above the RHS and used them below it. The key is a
load from a
__perry_init_strings_*handle global — a registered root thatevacuation rewrites — so the register named from-space while the global did
not. String literal operand is not GC-rooted across an allocating call in the same expression (stale handle after evacuation) #7114 exactly, in a second arm: the write landed under a garbage key, on
a stale receiver.
arr.filter(cb)/arr.some(cb)/arr.every(cb)held the array in aregister while the callback was lowered, and a callback literal is a
js_closure_new. refactor(codegen): migrate arrays_finds.rs + array_methods.rs onto the Layer 1 rooting API (#7615) #7620'sfind*finding in three more arms that were missedbecause they live in a different file.
Math.min/Math.maxwith three or morearguments thread a raw
ArrayHeader*throughjs_array_push_f64across eachremaining argument's own lowering, so
Math.min(f(), g(), h())pushed into apre-move address.
fetch(u, { headers: { k: f() } })has the same shape withjs_object_alloc. Both are GC: evacuating minor drops an old-to-young field[1] edge, crashing with 'value is not a function' #7154'sObjectSpreadbug, and because what isstale is an
i64derived above the window rather than a NaN-boxed double,GC: #7154's residual is NOT fixed — the loop-polls config is red 0/30, and stock zod alone fails 5/40 #7280's
root_reloadstructurally cannot repair either (slice 1b, finding 2).fetch's three string operands sat in registers across the whole headersconstruction and across
js_fetch_headers_to_json, which enumerates the ownproperties of a program-supplied value and so can re-enter user code through an
accessor — the argument
Expr::ObjectSpreadalready makes forjs_object_copy_own_fields."k" in process.envandJSON.parse(<literal>, <closure>)loaded astring literal's handle above an intervening allocating call and used it below.
Both are repaired by a single re-emitted
load, no runtime call — theReloadhalf ofoperand_protection.delete o[k](both forms),x instanceof <dynamic>,path.join/path.win32.*/path.relative/path.basename(p, ext),arr.includes(v, from),arr.splice(...),Array.from(it, fn),arr.join(sep),arr.slice(s, e),Object.groupBy/Map.groupBy,s.match(re)/s.matchAll(re),JSON.parse(t, reviver),parseInt(s, r),new RegExp(p, f),Array.prototype.<m>.call(like, ...),map.delete(k),a[i]++, andprocess.nextTick(cb, ...args).Scoped honestly
map_set.rswas already hand-rooted end to end (#6970): its migration is atranslation, not a repair, and its emitted IR is byte-identical. What it buys
is that the release stops being a statement a later edit can move into a branch —
the shape #7462 shipped in
URLSearchParams.delete, the direct sibling of thesearms.
ObjectSpreadandObject.assignwere likewise already rooted; thosebecome the accumulator with no behavioural change.
Deliberately not closed:
Expr::IndexUpdate(a[i]++) still holds itsre-read receiver and index across
js_dyn_index_get,js_to_numericandjs_numeric_step— three calls that can each run user code — beforejs_dyn_index_setconsumes them. Closing that needs a per-use re-read insidethe body rather than one group-wide re-read, which is a different combinator.
Per the template's rule it should arrive with the slice that needs it, not ahead
of one. The operand-to-operand half is closed here. Filed separately.
Verification (local; the CI backlog is deep, so this is the evidence)
#7622 control first, as the campaign now requires. Compiling the same source
twice with the same binary: clean on the 4-probe set (172/172 identical), and on
the 149-module corpus it reproduces 5 ordering-only permutations
(
@.str.Nnumbering in__perry_init_strings_*, plusclass_expr_dynamic_parent_ctor::main). Only then was anything attributed.IR identity. 4 purpose-built probes reaching every arm of all three modules:
172 functions, 153 identical, 19 differing —
p1_mapsetbyte-identicalthroughout, and the whole-corpus net delta is
+236 load / +174 store / +164 br / +149 ptrtoint / +82 icmp / +82 inttoptr / +77 bitcast / +71 write-barrier / +12 alloca / +11 invoke, i.e. root plumbing only, nothing deleted, with thesole call-target delta being
js_write_barrier_root_nanbox. Over thegc-root-dominancecorpus (2452 functions, 149 modules): 2436 identical, 16differing — 11 root plumbing, 5 ordering-only of which 4 are in the control set
and the fifth (
class_gap_ref_new_dynamic::main) was pinned by compiling it sixtimes with the baseline binary, differing every time. Net corpus delta is
+20 js_shadow_slot_bind / +20 js_shadow_slot_set / +44 store / +27 bitcast / +15 load / +4 allocaand nothing else.The gate's subject is live: root stores 9826 → 9846 over the identical
129-source corpus, matching the +20 binds exactly.
gc-root-dominancegreen in both gated modes with the allowlist empty:2452 functions / 149 modules / 9846 root stores → 0 violations;
--seeded-violations 40→ 40 planted, 40 caught, 0 missed;--unrooted-allocas --moving-only→ 0 over 7867 gc-capable allocas. All four checker static auditspass (
--self-test,--audit-alloc-re,--audit-poll-capable,--audit-immovable-sources).gc_root_dominance_dep_corpus.sh, 81 zodmodules) green in both gated modes too: 12899 functions / 12908 root stores →
0 violations,
--seeded-violations 40at 40/40,--unrooted-allocas0 over15242 gc-capable allocas. Root stores 12878 → 12908 there as well. This is the
population that matters most for the accumulator change, since zod is exactly
"builds objects field by field out of data".
--stale-registers --moving-onlyratchet is unchanged onboth arms: 23 on the curated corpus, 115 on the dependency corpus.
cargo test -p perry-codegen --lib691 pass;--doc— bothcompile_fail,E0499arms still reject.cargo test -p perry --bins888 pass(
scripts/ci_test_scope.pyputsperryandperry-codegenin this diff'sscope; both were run).
cargo test -p perry-runtime --no-fail-fast1886 pass. One failure on thefirst run,
promise::keyed_table::settling_many_keys_is_not_quadratic, thecargo-test: perry-runtime's suite fails a different number of tests on every run #7365 timing flake; green on rerun (it ran while an IR corpus was compiling).
with the one non-PASS (
test_gap_fetch_request_from_node_incoming_message,SIGABRT) reproduced on a pristine
origin/mainbuild. Filed separately — itis not in
known_failures.json, andparityis tag-gated, so it has beenable to hide.
migrated arm is unchanged again under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1.one run cannot speak for three): a real compiling
temp_root_push_double/temp_root_truncatepair injected into each file turnsmigrated_modules_do_not_reach_past_the_rooting_apired naming both lines —map_set.rs:46/:47,logical_collections.rs:145/:146,instance_misc1.rs:181/:182— then reverted, green again.Unlike slices 1a and 1b, these three named
expr::temp_rootbefore themigration, so the ledger line is load-bearing on the committed source too, not
only under sabotage. Recorded in
MIGRATED_MODULES' doc.lintjob step set enumerated from.github/workflows/test.ymland run:all pass except
benchmarks/ci_public_baseline_check.py, which is red onmainand independent of this diff (it hashesCargo.tomlandbenchmarks/**; this diff touches onlycrates/perry-codegen/src/). refactor(codegen): Layer 1 slice 1 — migrate lower_array_method.rs onto the rooting API (#7615) #7618 andrefactor(codegen): migrate arrays_finds.rs + array_methods.rs onto the Layer 1 rooting API (#7615) #7620 report the same step failing on every
mainrun since 2026-07-29.cargo clippy -p perry-codegenemits no warning in any migrated file.No version bump (maintainer bumps at merge). No measurement runs: this is a
behaviour-preserving refactor, and the benchmark hosts are reserved.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores
0.5.1357.