fix(runtime): seeded static literal shapes hold key atoms, so megamorphic reads confirm again (#11653 regression) - #11673
Conversation
…he megamorphic confirm answers it The megamorphic read confirms its slot guess by one pointer compare of the key listed at the guessed position against the site's key: the pool's atom for that text (#11633). Since link-time ShapeIds (#11653) every literal with a static id gets its record and keys list from the seed unit, which runs in each agent right after js_gc_init, before any module's string pool. The list's canonical copy stores the atom of each key only when one exists, and none did yet, so a seeded list held its own strings: no read site ever passes those, every confirm against a seeded literal missed, and each megamorphic read of one fell to ic_slow_body and the by-name walk (lead_mega1 225.8 -> 435.8 instructions per read). canonical_keys_for_names now mints the atom of each name first, exactly as the pool mints it (non-empty UTF-8 literals of at most INTERN_MAX_BYTE_LEN bytes); the pool finds that atom later, and the seeded list holds the same strings as a list written after the pools ran. Tests: a seeded literal answers the megamorphic confirm like a shape minted after the pools ran (sabotage: without the atom mint the seeded record's confirm fails); lead_mega1 as an e2e instruction bound (<= 260 per read; 182.9 fixed, 392.9 without the fix).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughStatic shape seeding now mints eligible key names as string-pool atoms before building canonical key lists. Tests check megamorphic slot confirmation for seeded shapes and measure instruction counts for repeated literal reads. ChangesStatic Shape Key Atom Confirmation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The seeded-key change has no identified merge-blocking issue and is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects startup-time key identity within each runtime agent. The inspected production path uses compiler-generated keys, and no new external entrypoint or privilege was identified. Exposure through native callers is not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
1 participant
Fixes a regression from #11653 (link-time static ShapeIds): megamorphic reads on object-literal receivers stopped hitting the confirm and fell back to walking the keys. lead_mega1 went from 225.8 to 435.8 instructions per iteration.
Root cause
canonical_keys_for_names(object/static_shapes.rs, used byjs_shape_seed_plain) builds the seeded literal shapes' canonical key lists right afterjs_gc_init, before any module's string pool has created its key atoms.Appended::atomizedswaps in an atom only if one already exists. At seed time none did, so every seeded key list held its own private strings.slot_guess_confirmed) compares the listed key's pointer against the site's key, which is the pool's atom. It never matched.Fix
canonical_keys_for_namescreates each name's atom throughjs_string_pool_atombefore building the list. It covers exactly the names a pool would atomize: non-empty, valid UTF-8, at mostINTERN_MAX_BYTE_LENbytes. The pool later finds that atom, and the canonical key list holds it.Results (instructions per iteration, median of 3, outputs identical to node)
tsc +0.1% and Zod −0.17%, both within noise. The static-id census is unchanged.
Tests
a_seeded_literal_shape_answers_the_megamorphic_confirm_like_a_minted_one(runtime): fails without the fix.crates/perry/tests/megamorphic_static_literal_confirm.rs: lead_mega1 must stay ≤ 260 instructions per read. It fails at 392.9 without the fix. It needs a perf counter, so it skips underCIwhen none is available; the runtime test is the deterministic guard.Verification
runtime 4809/0 (serial); codegen 2336/0; the static-shape e2e tests pass; gc-root-dominance 40/40 seeded, stale 0; gc_call_effects
--check, wasm abi, fmt and file size pass.Possible follow-up (pre-existing, not this regression): class key lists are built at their module's init and could miss atoms in the same way; the fix would be the same atom creation in
build_longlived_keys_array.Summary by CodeRabbit