perf(codegen): resolve read-only boxed capture cells once per closure entry (−1.8% / −2.8% wolf-ecs) - #9026
Conversation
Every read of a boxed capture in an ordinary closure body paid `js_box_get_bits`: an `is_registered_box_ptr` probe (thread-local cache + registry, 1.45% of the wolf-ecs entity cycle by itself) followed by one load. The PerryTS#8644/PerryTS#8705 trusted-clone machinery already retires this inside its private clones — validated at dispatch, cell pointers cached at entry, cells loaded per use — but only method callback parameters resolve those clones; a hoisted function declaration called directly (`function add(lB){...}`, capturing the ECS and its queries) runs its public body and pays the probe on every read of every iteration. This is the public body's variant of the same cache. Entry resolves each admitted capture slot through the new `js_box_capture_cell_ptr`: a registered pointer answers its own cell — boxes never move (the collector rewrites the value inside the cell) and cell memory is never returned to the allocator while a capturing closure is live (the PerryTS#8208 argument the update lowering already relies on) — and an unregistered pointer answers a shared immutable `undefined` cell, so per-read behaviour is exactly `js_box_get_bits`'s (PerryTS#4926: invalid box reads as `undefined`) in both cases. The cached pointers feed the existing `trusted_box_capture_ptrs` read arm: per-use cell load with the inline TDZ check, so writes through sibling closures stay visible. Admission is narrow by construction: only bindings the body never writes (the trusted `LocalSet`/`Update` arms store straight through the cached pointer, which must never reach the fallback cell), read at least twice or inside a loop (a cold-branch single read must not become an unconditional entry call), and never in async, generator-wrapper, or CPS async-step bodies (the repsel context gate's own exclusions). `PERRY_BOX_CAPTURE_ENTRY_CELLS=0` restores the per-read calls. Differential vs node (sibling-closure mutation visibility, hoisted function-decl consts, pre-initialization reads, shared written bindings): identical output, and the kill switch produces byte-identical results. Mac mini, 11 alternating pairs on the PerryTS#9016+PerryTS#9018 stack, both windows: add_remove −1.81%/−1.82%, entity_cycle −2.75%/−2.73% (11/11 except one 10/11) — slightly better than the hand-hoisted source ceiling (−1.65%/−2.72%) this was sized against before building. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
…nch file checkout Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
|
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 (1)
📝 WalkthroughWalkthroughThe change profiles boxed-capture usage, gates entry-cell caching with an environment variable, adds runtime pointer resolution, and caches eligible capture-cell pointers in supported closures. ChangesBox Capture Entry-Cell Caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The optimization changes generated closure behavior and adds a runtime ABI helper; native integrations should confirm that the new symbol is only reachable through authorized generated callers. No concrete user-facing failure is identified, so the PR is mergeable with explicit owner awareness of this bounded integration risk. Sequence Diagram(s)sequenceDiagram
participant ClosureCodegen
participant js_box_capture_cell_ptr
participant BoxCell
ClosureCodegen->>js_box_capture_cell_ptr: Resolve eligible capture slot at closure entry
js_box_capture_cell_ptr->>BoxCell: Resolve registered pointer or shared undefined cell
BoxCell-->>ClosureCodegen: Return stable cell address
ClosureCodegen->>BoxCell: Load capture value on repeated reads
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly explains the optimization, scope, semantics, benchmark results, and test coverage. It does not use the template headings or provide a related issue and checklist, but the required technical information is mostly present. Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 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 |
The `0000-` placeholder is never a legal fragment number; PerryTS#9010's gate rejects it outright rather than letting it misattribute the change at release time.
|
Merged. Caching a raw cell pointer across a body is the shape that has bitten this repo repeatedly (a runtime-side cache of a heap pointer is a GC root the static checker cannot see), so I went at the two things that would make it unsound rather than the measurement. The admission collector is the right shape. CLAUDE.md's own pitfall list says a capture collector's catch-all silently skips refs — that is exactly how a "never written" verdict goes wrong, and here a wrong verdict is not a missed optimization but a store through a cached pointer. Checked all three walkers:
Skipping nested The fallback cell deserves its narrow admission. Probe against node 26.5.1, 8 cases: read-only capture in a loop with a sibling writer between calls, a nested closure writing the cell mid-loop, read-before-initialization, two closures sharing a binding across interleaved writes, the hoisted-function-declaration shape the PR is motivated by, and a capture reassigned to a different object between calls. Byte-identical to node — and identical again with Renumbered the fragment from Validation: |
…is red) (#9044) * fix(build-cache): register the two unkeyed codegen env vars CI's codegen_env_vars_are_build_cache_inputs gate fails on current main: PERRY_BOX_CAPTURE_ENTRY_CELLS (#9026's once-per-closure-entry cell resolution gate) and PERRY_GUARDED_PREINLINE_MAX_IR_BYTES (the guarded-preinline size ceiling) both change emitted code but key neither the cache nor an exclusion — every open PR is red on it. Register both as cache inputs, same rationale as the RS4GC and TRE budgets beside them. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * docs: add the changeset fragment for #9044 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
What
An ordinary closure body that repeatedly reads a boxed capture it never
assigns now resolves the box's cell address once at entry (new runtime
helper
js_box_capture_cell_ptr) and loads the cell directly per read,through the existing
trusted_box_capture_ptrsread arm (per-use load +inline TDZ check). Only the pointer is cached — never the value — so a write
through any sibling closure sharing the binding stays visible.
Why
Every read of a boxed capture in a public closure body pays
js_box_get_bits:an
is_registered_box_ptrregistry probe plus one load. In wolf-ecs the probealone is 1.45% of the entity cycle, because the benchmark's drivers are
hoisted function declarations (
function add(lB)capturing the ECS and itsqueries) — the #8644/#8705 trusted-clone machinery never fires for them (it
resolves method callback parameters), so
ecs/qAwere re-probed and re-readon every loop iteration.
Caching the pointer is sound where caching the value is not:
inside the cell — and cell memory is never returned to the allocator while
a capturing closure is live (the fix(async): release and reuse a completed plain-async activation's box cells (#7933 follow-up) #8208 argument the update lowering already
relies on);
undefinedcell,reproducing
js_box_get_bits's per-read answer (codegen: js_box_set/js_box_get emitted with undef box-pointer operand (mutable-capture box alloc elided on taken path) — source bug behind #4898 #4926) exactly.Admission is narrow: never-written bindings only (the trusted write arms store
straight through the cached pointer, which must never reach the fallback
cell); read ≥2 times or inside a loop (a cold-branch single read must not
become an unconditional entry call); not in async / generator-wrapper /
CPS-async-step bodies (the repsel context gate's own exclusions).
PERRY_BOX_CAPTURE_ENTRY_CELLS=0restores the per-read calls.Numbers
Sized against a hand-hoisted source ceiling before building (−1.65%/−2.72%);
the compiler version slightly beats it. Mac mini, 11 alternating pairs on the
#9016+#9018 stack:
IR: the two hot driver closures drop from 6
js_box_get_bitscalls each to 0,plus 2 entry resolves.
Semantics
Differential vs node, identical output: sibling-closure mutation visibility
interleaved with reads, hoisted function declarations over
constbindings,pre-initialization reads, a written shared binding read in a loop, nested-loop
reads with calls in between. The
PERRY_BOX_CAPTURE_ENTRY_CELLS=0build isoutput-identical to the enabled build on the same corpus. ECS differential
probes vs node unchanged.
Testing
RUSTFLAGS=-D warnings cargo check --workspace --all-targets(host excludes) — clean.perry-codegenandperry-runtime --libsuites.issue_8655_array_subclass_indexing,issue_8690_loop_versioned_arraylike,issue_8897_field_push_writeback,issue_8773_closure_capture_packed_loops.Summary by CodeRabbit
Performance
Configuration
Documentation