codegen: emit the receiver-unknown numeric index tiers once per dynamic site - #8940
codegen: emit the receiver-unknown numeric index tiers once per dynamic site#8940proggeramlug wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe compiler removes the duplicate receiver-unknown numeric dispatch for dynamic keys on erased arrays. Non-SSO keys now use ChangesClaimed array key dispatch
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized codegen change removes duplicate numeric-tier emission while preserving the complete fallback behavior; no actionable merge-blocking risk remains at the current head beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description clearly explains the change, motivation, semantic impact, generated-IR reduction, affected code paths, and verification status. It does not use the template headings or include the checklist and related-issue section, but it is sufficiently complete and directly relevant. Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 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 |
|
Local gates on #8931 has merged since this was opened — rebasing onto main next so only this PR's commit remains; Mac-mini paired screens and the before/after traced clone sizes to follow. |
|
Traced lowered-IR sizes (
21 other traced clones unchanged (no dynamic-key site). Machine-code effect and the Mac-mini screens follow once the build here finishes. |
…ic site `lower_claimable_array_string_key_get` — the canonical-i32 split's runtime-key arm for erased-Array receivers — still carried its own copy of the inline typed-array + dense-subclass `arrlike.ic` + dispatcher lattice from the v83 dynamic-key work. Since the brand arm (`aidx.claimed.other`) every integral key below 2^31, canonical or INT32-boxed, is served by the canonical arm's single copy, so the runtime copy only ever ran for integral keys in [2^31, 2^32) — at ~11 KB of IR per site (wolf-ecs `SparseSet.has`'s proven-this u31 clone was 46.7 KB for one statement, 22 KB of it these two lattices, against a 16 KiB pre-statepoint inline budget). Its other caller passes static string/symbol keys, where the numeric path is dead. Those keys now take the complete `js_array_get_index_or_string` route, which handles every index. The v83 test asserts exactly one inline typed-array tier and one dense-subclass tier per site, and no `aidxkey.int` block. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
4ae1c46 to
9e49235
Compare
|
Rebased onto main ( |
|
Mac-mini paired screens (11 alternating pairs,
So: no time win — add/remove is noise-flat and the entity cycle is consistently ~0.4% slower (deltas 0.14…0.71 with one 1.53), most plausibly code layout after the hot |
|
Follow-up experiment (v99: a 40 KiB pre-statepoint inline budget for one-statement bodies, which this PR's smaller Closing this PR: the change is correct and smaller, but it does not pay by the campaign's retain rule, and I'd rather not land layout-neutral code churn while three ECS PRs are in flight. The branch stays ( |
Rebased onto main after #8931 merged; one commit.
What
arr[key]on an erased-Array receiver with a dynamic key (packed[sparse[x]]in wolf-ecsSparseSet.has,a[b[i]]in general) lowers through the canonical-i32 split. Since the brand arm (#8890's v90), every integral key below 2³¹ — canonical double or INT32-boxed — is served in the canonical arm'saidx.claimed.otherbranch, which emits the receiver-unknown numeric tiers (inline typed-array readtav.*, dense Array-subclassarrlike.ic.*, complete dispatcher) once. The runtime-key arm (lower_claimable_array_string_key_get) still carried its own copy of that lattice from the v83 dynamic-key work (aidxkey.int→aidxkey.int.exact), which after v90 only ever ran for integral keys in[2³¹, 2³²). Its other caller passes static string/symbol keys, where the numeric path is dead.This PR drops that copy: those keys take the complete
js_array_get_index_or_stringroute (which handles every index). Same semantics; ~11 KB of IR less per dynamic site.Why it matters
Diagnosing why
SparseSet.has$pshapeis still ablinsideaddComponent$pshape(campaign item #4): an env-gated trace of the pre-statepoint inline admission showed the one-statement clonehas$pshape$idx_u31at 45,936 bytes of lowered IR against the 16 KiB budget (GUARDED_SPECIALIZATION_PREINLINE_MAX_IR_BYTES) — which is why the earlier 32 KiB experiment (v82b) could never fire. Byte split:arrlike.ic13.2 KB,tav9.4 KB, guardedarr6.7 KB,aidx5.7 KB,plen5.1 KB — i.e. the dynamic-key site alone was ~30 KB because the two arms each emitted the same lattice. This is the first of the size reductions; the clone is still above budget afterwards (the.lengthIC lattice and the two guardedthis.sparse[x]tiers are next), so no inlining change is claimed here — only smaller code at every such site.Verification (local, per the campaign rule)
any_typed_dynamic_key_takes_the_numeric_tiers_when_it_is_an_array_index) now asserts exactly onetav.get.brandand onearrlike.ic.family_tokenblock definition per site, noaidxkey.intblock, and the SSO-string and complete-route calls still present.index_get_claim_tests: 12/12.perry-codegen,perry-transform,perry-hir) and the lint gates (-D warnings, file size, GC store inventory, raw-handle debt, shape census, local-binding audit, addr-class audit) are running now; I'll confirm in a comment together with the Mac-mini paired screens (2 s + 50 ms windows, 11 pairs, both wolf-ecs benchmarks) and the traced clone sizes before/after.https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
Summary by CodeRabbit
Performance
Bug Fixes
Tests