perf(codegen): number-by-construction locals step inline in ++/-- (−2.1% / −1.1% wolf-ecs) - #9018
Conversation
The Update lowering keeps `js_to_numeric` + `js_numeric_step` for any counter outside `integer_locals`/`unsigned_i32_locals`, to preserve BigInt stepping and ToNumeric coercion. That gate predates PerryTS#8105: a local admitted by `collect_number_by_construction_locals` can never hold a BigInt (or anything else non-Number) — the collector admits a local only when its initialiser and every later write is an expression the spec guarantees evaluates to a Number, and the fact is already trusted for the strictly harder claim of licensing a bare `load double` with no value check. For such a value both calls are the identity the inline arm computes: `js_to_numeric` routes a non-BigInt through `js_number_coerce` (identity on a Number), and `js_numeric_step`'s non-BigInt arm is exactly `numeric ± 1.0`. So the gate now accepts the fact and emits the plain `fadd`/`fsub`. The shape this retires is a loop counter no integer fact can admit: `for (let j = a.length - 1; j >= 0; j--)` — the init is not an Integer literal, so `j--` paid two runtime calls per iteration. wolf-ecs's benchmark drivers run exactly that loop per operation. Boxed and captured locals are never in the set, so the capture arms keep their calls. Differential check vs node (fractional counters, postfix/prefix returns, NaN, -0, BigInt locals, string counters, 2^53-boundary doubles): identical output. Mac mini, 11 pairs vs the PerryTS#9016 build, both windows: add_remove −2.12%/−2.14%, entity_cycle −1.08%/−1.12% (11/11 except one 10/11). 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 update lowering now uses inline numeric stepping for locals proven to contain Numbers. The changelog documents this optimization. ChangesNumeric update optimization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change replaces redundant numeric coercion and stepping calls with an equivalent inline update only for locals proven to contain Numbers, while preserving existing behavior for other values. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the change, motivation, semantics, benchmark results, related issue reference, and testing. It does not use the template headings or include the checklist, but the required technical information is mostly present. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 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, and PerryTS#9010's gate now rejects it outright rather than letting it reach `main` and misattribute the change at release time.
|
Merged. The one-clause change is sound because the fact it leans on is already stronger than what it needs: Given that, both retired calls really are the identity: Fixed on the branch: the fragment was Incidentally this was the gate's first live catch. Running it against this PR before the fix: Validation with |
… 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>
1 participant
What
i++/i--on a local carrying the #8105 number-by-construction fact nowsteps inline (
fadd/fsub ±1.0) instead of callingjs_to_numeric+js_numeric_stepper update. One added clause in the Update lowering's gate;boxed and captured locals are never in the fact, so the capture arms keep their
calls.
Why
The two calls exist for BigInt stepping and ToNumeric coercion, and the gate
admitting the inline arm predates #8105: only
integer_locals/unsigned_i32_localsqualified. A counter likecan never be admitted by the integer facts — its init is not an Integer
literal — so every
j--paid two runtime calls per loop iteration. That isthe standard "iterate an untyped array backwards" shape; wolf-ecs's benchmark
drivers run it once per entity per operation (
js_numeric_step1.15% +js_to_numeric/js_number_coerce1.26% of add_remove).The number-by-construction collector admits a local only when its initialiser
and every later write is spec-guaranteed to evaluate to a Number, and the fact
already licenses a harder optimization (a bare
load doublewith no valuecheck, #8105). For a value that IS a Number, both retired calls are the
identity the inline arm computes:
js_to_numericroutes a non-BigInt throughjs_number_coerce(identity on a Number), andjs_numeric_step's non-BigIntarm is literally
numeric ± 1.0(value/dynamic_arith.rs).Semantics
Differential check against node, exact-output identical: fractional counters
reassigned in loops,
a.length - 1inits, postfix returning the old value,prefix returning the new, NaN propagation, the
(-1)++ → +0(not-0) edge,BigInt locals (still step as BigInt — not in the fact), string counters (still
coerce — not in the fact), and 2^53-boundary doubles.
Numbers
Mac mini, 11 alternating pairs, measured on top of the #9016 build (the change
is independent of #9016 — different file, different mechanism):
IR confirmation: the two benchmark driver closures drop from
step=2 to_numeric=2calls to zero; the remaining callers are field updates(
this.entID++), a different lowering.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.Summary by CodeRabbit