fix(codegen,runtime): closure-literal singletons broke function identity — pi boots - #9128
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughClosure singleton allocation is restricted to compiler-generated async step closures. Native namespace reads now include dynamic fields. Regression tests cover closure identity and patched builtin namespace members. ChangesFunction identity and namespace member updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR restores fresh function identity and makes native-module overrides visible through shared reads, but named built-in imports may observe those overrides before the expected synchronization point. The change is mergeable with explicit owner awareness and follow-up to confirm the intended snapshot behavior. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary of both fixes, their motivation, affected behavior, and validation results. It does not use the template headings and omits an explicit related issue, command-based test plan, and checklist confirmation, but the core information is present. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 too large.)
✨ 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 |
added 3 commits
August 30, 2026 06:57
…n identity — pi boot threw Cyclic __proto__
perry-compiled pi (13MB esbuild bundle) died at startup with
`TypeError: Cyclic __proto__ value` out of `js_object_set_prototype_of`,
with obj_bits == proto_bits exactly — the two ARGUMENTS were the same
pointer — while the chain behind the proto was healthy. None of the
bundle's 17 textual `Object.setPrototypeOf(` sites fired a JS logging
shim, because the self-set was manufactured upstream of the call: the
closure-literal singleton caches handed back ONE ClosureHeader for two
evaluations of the same function literal, so `setPrototypeOf(wrapped,
original)` (a graceful-fs-style wrap pattern) received one object twice
and correctly refused the "cycle".
Mechanism: `expr/closure.rs` routed closure literals through
`js_closure_alloc_singleton` (captureless arrows) and
`js_closure_alloc_with_captures_singleton` (arrows with captures, and
non-arrow literals whose captures are all boxes) keyed by
(func_ptr, capture bits). Two evaluations of the same literal with
bit-identical captures — e.g. an arrow capturing the same constant, or
any captureless arrow — came back `===`-equal. ECMA-262
OrdinaryFunctionCreate requires a fresh object per evaluation, and the
distinction is observable through `===`, expando properties, WeakMap
keys, addEventListener de-duplication, and `Object.setPrototypeOf`.
Minimal repros (byte-compared against node before/after):
function mk() { return () => K; } // captured arrow
const a = mk(), b = mk(); // perry: a === b (node: false)
Object.setPrototypeOf(a, b); // perry threw Cyclic __proto__
and the same with `() => 1` (captureless). Both now match node.
Fix: gate every closure.rs literal singleton path on
`is_plain_async_step_body` — the file's existing detector for the
compiler-synthesized plain-async step closures (their terminal
`Stmt::ReleaseBoxes` arms cannot appear in user code). Those are the
closures the caches were built for (PerryTS#8269's parallel async-await
pattern re-creates them per resume with the same per-activation box
captures, and their identity never escapes the promise machinery), and
they keep the fast path. Every user-authored arrow and function
expression now mints a fresh closure. Runtime-internal singleton users
(function-declaration references, property_get/i18n/arrays wrapper
thunks) are separate paths and unchanged. A genuine
`setPrototypeOf(x, x)` still throws — the cycle check is untouched.
Perf note: this deliberately gives back the user-arrow closure reuse
from the PerryTS#8269/PerryTS#8291 captured-singleton extension (e.g. ECS
`World.executeEntityCommands`' per-call inner arrow) and the captureless
user-arrow singleton at literal sites; a sound replacement needs
escape-aware caching rather than identity-violating sharing.
Validation: repros above and test-files/
test_gap_9090_closure_literal_identity.ts byte-identical to node;
`cargo test -p perry-runtime --lib -- --test-threads=1` green — 2813
passed, 0 failed with `--skip reserved_floor` (that module's at-scale
tests SIGABRT on this pre-PerryTS#9110 base; known PerryTS#9108/PerryTS#9110, unrelated);
`cargo test -p perry-codegen`: 283+75 passed after updating the four
native_proof_regressions pins from `js_closure_alloc_singleton` to
`js_closure_alloc` (their real subject — the alloc storing the public
wrapper pointer — is preserved); one pre-existing env-leak flake
(`packed_f64_loop_unary_math_store_versions_with_side_exit`) passes in
isolation.
Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
… — pi boot threw Cyclic __proto__ (part 2)
With the closure-literal identity fix in place, pi still died at startup
with `TypeError: Cyclic __proto__ value`, obj_bits == proto_bits exactly.
The instrumented throw site showed both arguments were ONE closure with
`func_ptr = 0xBADD_DEAD` (BOUND_METHOD_FUNC_PTR, capture_count 3) and a
healthy 3-link chain behind it — the canonical bound-native callable that
`bound_native_callable_export_value` mints once per (module, member).
The failing code is graceful-fs's module init, bundled into pi
(pi-bundle.mjs:6621/6686/6705):
var chdir = process.chdir;
process.chdir = function (d) { ... };
if (Object.setPrototypeOf) Object.setPrototypeOf(process.chdir, chdir);
and the same wrap for fs.rename / fs.read. Under perry the patch write
did not round-trip on the re-read, so setPrototypeOf received the SAME
canonical closure for both arguments — a self-set — and the cycle check
correctly refused it. The earlier probe of this exact shape passed
because it patched a PLAIN object, where writes round-trip; the failure
needs a builtin namespace receiver. The JS shim over
`Object.setPrototypeOf(` never fired because the conflation happens in
the native member-READ, upstream of the call.
Root cause: user writes to builtin namespace members are stored in two
different places depending on the lowering — computed stores
(`process[k] = fn`) go through `nm_field_set_override` into
`NATIVE_NAMESPACE_PROP_OVERRIDES`, while static stores
(`process.chdir = fn`) reach the generic store path and land as an own
dynamic field on the canonical namespace object. The NAME-KEYED read
entries carry no object pointer and consulted only the override table:
* `js_native_module_property_by_name` (codegen static reads of
process.* members) missed own-field stores, so the graceful-fs
static patch was invisible to the static re-read;
* `js_native_module_esm_export_value` (codegen property reads off a
builtin DEFAULT import — `import fs from "node:fs"; fs.rename`)
consulted NOTHING (consult_overrides=false plus its own snapshot
cache), so no fs patch was ever visible. In Node the default import
of a core module is the live mutable CJS exports object, so the
patched value must win; the tls DEFAULT_* cache-coherence hack was
the ad-hoc version of this for three keys.
Fix: `native_namespace_user_value(module, prop)` consults the override
table and then the canonical namespace object's own field (never
creating a namespace — if none exists, no user store can have landed on
one). Both name-keyed read entries call it before any built-in
resolution or snapshot cache. Named ESM import bindings of core modules
snapshot at module init before user patches run, so their intended
snapshot semantics are unaffected in the eager case.
Validation: r11-r16 probe matrix (process/fs, static/computed reads and
writes) and test-files/test_gap_9091_native_member_patch_roundtrip.ts
byte-identical to node; a genuine `setPrototypeOf(x, x)` still throws.
Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
proggeramlug
force-pushed
the
fix-pi-closure-identity
branch
from
August 30, 2026 05:18
eb8e06f to
649784f
Compare
Contributor
Author
|
Merged, plus a rustfmt commit. pi boots natively — worth stating on its own. The singleton keying was a genuine spec violation, not just a nuisance: OrdinaryFunctionCreate makes a fresh function object per evaluation, so Verified against node v26.5.1, 15 identity shapes, byte-identical:
Cases 8 and 9 were the ones I most wanted green: making every literal fresh must not make an existing reference compare unequal to itself. One thing to flag for whoever measures next: this trades an allocation elision for spec correctness, so closure-heavy hot paths may show a regression. That's the right trade and I'm not asking for it back — but if a benchmark moves, this is the likely cause rather than a mystery. Validation: codegen 1349 passed, runtime 2822 passed (exit 0), perry --bins 1066 passed, fmt clean, (One runtime run in the middle showed a single unrelated failure that did not reproduce on re-run — same order/state flake I've hit twice today, not this PR.) |
This was referenced Aug 30, 2026
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 30, 2026
A fresh (identity-carrying) capturing closure was born as js_closure_alloc plus one js_closure_set_capture_bits runtime call per capture, and each setter re-resolved the GC header, re-checked forwarding, re-dispatched on the object kind for layout_note_slot and paid the write barrier's page-table classification again. After PerryTS#9128 made every user closure literal fresh, that per-capture chain was ~24% of a capturing-closure birth and js_closure_alloc itself ~34% (sample, main@PerryTS#9128). New runtime entry js_closure_alloc_init(func_ptr, capture_count, captures_ptr): no-collect-first nursery allocation (its Some contract keeps the raw capture bits valid; no trigger check), header + bulk slot copy, ONE newborn layout classification (layout_init_from_slots: forget-once, then pointer-free / unknown / side-mask — no per-slot notes, no interleaved table removes), and a barrier pass that classifies the parent once for all slots (runtime_write_barrier_newborn_slots; with barriers off it is the incremental-mark shade check per value). The block-boundary fallback takes the original alloc + per-slot setter path. Codegen emits it for fresh closures whose captures are all plain bits (bulk_fresh_init); box-cell captures keep the per-slot setter path (their set_closure_box_capture bookkeeping has no bulk twin); the reserved this / new.target slots are pre-filled with the pointer-free sentinel and patched post-create exactly as before. Singleton (compiler-synthesized async-step) closures are untouched. Closure-birth differential vs node (plain and boxed captures, this-arrows, new.target, async, identity, arrays of closures, nested and 10-capture closures): byte-identical. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
proggeramlug
added a commit
that referenced
this pull request
Aug 30, 2026
…15%) (#9136) * perf(runtime,codegen): one-call birth for fresh capturing closures A fresh (identity-carrying) capturing closure was born as js_closure_alloc plus one js_closure_set_capture_bits runtime call per capture, and each setter re-resolved the GC header, re-checked forwarding, re-dispatched on the object kind for layout_note_slot and paid the write barrier's page-table classification again. After #9128 made every user closure literal fresh, that per-capture chain was ~24% of a capturing-closure birth and js_closure_alloc itself ~34% (sample, main@#9128). New runtime entry js_closure_alloc_init(func_ptr, capture_count, captures_ptr): no-collect-first nursery allocation (its Some contract keeps the raw capture bits valid; no trigger check), header + bulk slot copy, ONE newborn layout classification (layout_init_from_slots: forget-once, then pointer-free / unknown / side-mask — no per-slot notes, no interleaved table removes), and a barrier pass that classifies the parent once for all slots (runtime_write_barrier_newborn_slots; with barriers off it is the incremental-mark shade check per value). The block-boundary fallback takes the original alloc + per-slot setter path. Codegen emits it for fresh closures whose captures are all plain bits (bulk_fresh_init); box-cell captures keep the per-slot setter path (their set_closure_box_capture bookkeeping has no bulk twin); the reserved this / new.target slots are pre-filled with the pointer-free sentinel and patched post-create exactly as before. Singleton (compiler-synthesized async-step) closures are untouched. Closure-birth differential vs node (plain and boxed captures, this-arrows, new.target, async, identity, arrays of closures, nested and 10-capture closures): byte-identical. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p * perf(runtime): skip the barrier and the memcpy call on pointer-free closure births Follow-up cuts on the same entry, from a Linux perf annotate of the birth loop (18.5 ns/op, 478 instr/iter, IPC 4.97 — throughput-bound, so instruction count is the cost): - layout_init_from_slots now RETURNS whether any slot is pointer-bearing, and the birth skips runtime_write_barrier_newborn_slots entirely when nothing is. A closure capturing only numbers/booleans/SSO strings paid a call plus a page-table classification per slot for a barrier whose own child check would reject every one of them (write_barrier_slot_decoded was 9.3% of the loop on a NUMBER capture). - The ≤64-slot case classifies into a register-resident u64 instead of a LayoutSlotMask, and reads the mask-min-slots threshold once instead of through a per-birth OnceLock call. - layout_forget_object is called only when the per-object layout tables can actually hold an entry (per_object_layouts_maybe_nonempty), matching what the tables' own accessors check anyway (4.5% of the loop). - Slot counts ≤8 copy through a counted store loop; the runtime-length copy_nonoverlapping compiled to a memcpy PLT call (2.6% for ONE slot). Mini, medians: bare capturing closure 24.3 -> 21.0 ns (-13.6%), captured-arrow-field literal 27.8 -> 22.2 (-20.1%); captureless and plain literals unchanged. Cumulative against main: 28.5 -> 21.0 and 31.4 -> 22.2. Closure-birth differential vs node unchanged (byte-identical). Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p * chore(gc): audit the counted capture-store loop and the forEach identity stacks gc_store_site_inventory flagged #9136's counted store loop; classified BARRIERED to match the copy_nonoverlapping arm beside it, which is followed by the same closure layout/barrier rebuild. gc_runtime_root_holders flagged #9095's SET_FOREACH_STACK / MAP_FOREACH_STACK; classified not_a_gc_pointer — the entries are header addresses used only for identity comparison, never dereferenced, and set_header_moved_for_gc / map_header_moved_for_gc rewrite them when a header moves. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
The last two of pi's four startup walls, and with them pi boots natively for the first time (
--versionprints0.0.0, node-identical; all four earlier walls were #9069/#9073/#9085 + these).Commit 1 — closure-literal singleton identity.
expr/closure.rsrouted closure literals throughjs_closure_alloc_singleton/js_closure_alloc_with_captures_singletonkeyed on (func_ptr, capture bits): two evaluations of the same literal with bit-identical captures — any captureless arrow, or an arrow capturing the same constant — came back===-equal. ECMA-262 OrdinaryFunctionCreate requires a fresh object per evaluation, observable via===, expandos, and WeakMap keys. pi hit it through graceful-fs's wrap pattern:setPrototypeOf(wrapped, original)received ONE object twice and the runtime correctly threwCyclic __proto__ value— with obj_bits == proto_bits exactly, and none of the bundle's 17 textual setPrototypeOf sites firing a JS logging shim, which is what pointed below the JS boundary. Gap fixturetest_gap_9090_closure_literal_identity.ts.Commit 2 — name-keyed builtin-member reads must see user overrides (part 2 of the same boot sequence,
native_module.rs). Gap fixturetest_gap_9091_native_member_patch_roundtrip.ts.First pi measurements (loaded dev box, so wall time is an upper bound):
--versionboots clean;[gc-time]share_permille=102 — GC ≈ 10% of pi startup, with the vacuity gate satisfied (copying minors ran, 124k-object nursery census) — first real data point for the concurrent-GC decision gate (threshold 15%).Diagnosed and implemented by a subagent (runtime-side throw instrumentation → bundle-side call-site shims → identity-conflation hypothesis → smallest repro); validated by the coordinating session via the full pi boot.
Summary by CodeRabbit
Bug Fixes
Tests