perf(codegen): inline trusted boxed capture access; reject legacy numerics in strict eval - #8705
Merged
Conversation
added 7 commits
August 24, 2026 06:47
…feedback `typed_feedback_guards_direct_class_method_specialization` asserted `call double @js_class_field_add`, which #8653 deliberately stopped emitting for this shape when it restored the guarded field-init fast path (the #8648 3.11x `shapes` regression). Red on main since then, and invisible to per-PR CI because codegen integration suites only run when the diff names them. Assert the guarded fast path instead, and that the unconditional helper is NOT called.
|
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 (12)
📝 WalkthroughWalkthroughThe PR adds trusted boxed-capture pointer access across closure code generation, adds strict direct-eval validation for legacy numeric literals, updates related changelog entries, and changes a typed-feedback test to expect guarded class-field initialization. ChangesTrusted boxed-capture access
Strict direct-eval numeric validation
Typed-feedback expectation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TrustedClosure as Trusted closure codegen
participant FnCtx
participant CaptureBody as Capture body lowering
TrustedClosure->>TrustedClosure: Load boxed-capture pointers from closure storage
TrustedClosure->>FnCtx: Pass trusted_box_capture_ptrs
CaptureBody->>FnCtx: Read cached capture pointer
CaptureBody->>CaptureBody: Load or store capture cell
sequenceDiagram
participant ConstFoldDirectEval
participant StrictEvalDiagnostic
participant StrictParser
ConstFoldDirectEval->>StrictEvalDiagnostic: Check constant eval source
StrictEvalDiagnostic->>StrictParser: Reparse source with use strict
StrictParser-->>StrictEvalDiagnostic: Return legacy numeric diagnostic
StrictEvalDiagnostic-->>ConstFoldDirectEval: Report diagnostic result
ConstFoldDirectEval->>ConstFoldDirectEval: Synthesize runtime SyntaxError
✨ 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 |
This was referenced Aug 24, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 29, 2026
… entry (−1.8% / −2.8% wolf-ecs) (#9026) * codegen: resolve read-only boxed capture cells once per closure entry 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 #8644/#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 #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 (#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 #9016+#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 * fix: restore #9017's runtime declares clobbered by a cross-branch file checkout Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ * docs: renumber the changeset fragment 0000 -> 9026 The `0000-` placeholder is never a legal fragment number; #9010's gate rejects it outright rather than letting it misattribute the change at release time. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 29, 2026
…ntry A call `f(args)` through a Function-typed `LocalGet` callee reaches the guarded direct-dispatch arm in `lower_call/early_branches.rs`, which consults `resolved_arrow_callback_targets` and falls back to the full dispatcher on a null target — but only METHOD bodies ever populated the map (the PerryTS#8642/PerryTS#8705 callback-parameter resolution). A captured arrow, a module-global arrow, or a plain function's callback parameter paid `js_closure_callN` — two runtime boundaries plus strategy dispatch — on every call of every loop iteration. `collect_loop_called_callee_bindings` admits `(binding, arity)` pairs whose callee is a parameter, captured binding, or module global; never assigned in the body NOR anywhere in the module (a capture or global can be written by other bodies, and immutability is the entry-resolved identity argument); with at least one call site inside a loop to amortize the entry resolver call. `emit_callee_binding_resolutions` then reads each binding RAW at entry — slot load, capture-slot load (plus an untrusted `js_box_get_bits` cell read for a boxed capture, which returns the TDZ sentinel rather than throwing), or global load — and feeds `js_closure_resolve_arrow_direct_call`. A sentinel or non-closure resolves to null and every call keeps its fallback, so a body that runs before a captured binding initializes behaves exactly as before. The `Function` type-hint check mirrors the consuming arm's own predicate, so resolution and consumption cannot disagree. Wired for plain function bodies (parameters only — no module-wide reassignment oracle in that path) and closure bodies (all three sources); async, generator-wrapper and CPS-step bodies excluded as everywhere else. Isolated (Linux, same build, kill-switch A/B): captured arrow 8.0 -> 4.4 ns/op, capturing arrow 7.0 -> 2.5, module-global and parameter shapes equally; every other op in the 12-op sweep within +-0.1 ns. wolf-ecs (mini, 11 pairs): add_remove -0.52%, entity_cycle -0.36%, 11/11 in the 2 s window — the benchmark drivers call through captured bindings. Differential vs node identical: reassigned-global callees observe the new value (excluded from resolution), ordinary functions keep receiverless `this === undefined`, bound functions and rest/arity-mismatch shapes keep the dispatcher, throwing callees, captures mutated between calls, async arrows. `PERRY_CALLEE_BINDING_RESOLUTION=0` restores per-call dispatch, output-identical. Remaining in this family (profiled, not taken here): `js_typed_feedback_closure_direct_call_guard` is 24% of the isolated sweep — the statically-known-callee path re-guards every call; and the resolver returns the public trampoline rather than a clone matched to the site. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
This was referenced Aug 29, 2026
release blocker: source-graph IR assertions reject intentional trusted-box and pshape lowering
#9164
Closed
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.
Lands #8702 and #8698, plus a stale-test fix.
#8702 — inline trusted boxed capture access
Loads compiler-validated raw box capture pointers once at entry to private exact-arrow clones and reads/writes the box cell directly, keeping public and indirect closure bodies on the checked runtime accessors.
I verified the safety argument rather than taking it on trust, since caching a raw heap pointer across a collection is precisely how this codebase has been bitten before. The claim holds: box cells are malloc-side, not GC-heap —
closure/box_captures.rsopens with "Lifetime bridge between GC closures and malloc-side async box cells" — so they genuinely do not move under evacuation. The crate already carriesclosure_box_captures_owner_movedto rekey when the closure relocates, which matches the PR's "retains each non-moving box cell for the invocation even if the closure relocates". TDZ keeps itsTAG_TDZtest plus the trusted getter on the cold path, and direct stores keep the child-shading barrier.#8698 — reject legacy numerics in strict eval
Re-lexes constant direct-eval bodies under inherited strict mode and surfaces the deferred legacy decimal/octal diagnostics as
SyntaxError, leaving modern octal literals and numeric-looking text in comments/strings valid. The PR carried no changelog fragment and noskip-changeloglabel, so I added one rather than block on it.Stale test fixed — red on
mainsince #8653typed_feedback_guards_direct_class_method_specializationassertedcall double @js_class_field_add. #8653 deliberately stopped emitting that for this shape when it restored the guarded field-init fast path and reverted the #8648 3.11×shapesregression — so the test has been red since, and invisible to per-PR CI because codegen integration suites only run when the diff names them. #8702's author flagged it as pre-existing; I confirmed that independently on cleanmainbefore touching it.The assertion now pins the actual emitted shape rather than the old helper:
js_class_field_set_fallbackon the slow arm is what preserves DefineField semantics (an inherited setter must not run), so the negative assertion is safe.Validation
lint-job checkers passperry-runtime --lib(RUST_TEST_THREADS=1): 2655 passed, 0 failedperry-codegen --lib: 1214 passed, 0 failedperry-codegen --tests(all 28 integration suites): 0 failuresperry-hir --lib: 331 passed, 0 failedAn intermediate run showed 2 GC failures in
gc::tests::handle_bound_method_name::*. They are not from this stack: I reproduced them on cleanmainand isolated the cause to forcingCARGO_INCREMENTAL=0onto theperry-devprofile, which declaresincremental = true— same commit gives 2655/0 with incremental on and 2 failures with it off. CI's actual invocation (CARGO_INCREMENTAL=0, default test profile) gives 2655/0, so CI is unaffected. Worth knowing those two tests compare literal addresses and are therefore build-config sensitive.No version bump.
Summary by CodeRabbit
Bug Fixes
evalnow correctly raisesSyntaxErrorfor legacy decimal and octal numeric literals.Performance