perf(codegen): an indexed store skips the addref for a provably non-string value - #9195
Conversation
…tring value emit_jsvalue_slot_store_on_block and its scalar-aware twin pass layout_note_needed for BOTH the layout note and the string-addref demote, so every store needing a layout note also emitted js_string_addref_if_heap_string -- including `sieve[j] = false`, whose value is a boolean and can never be a heap string. The flagged emitter that separates the two already existed (array_push uses it); this adds the scalar-aware twin and threads store_needs_string_addref, the same predicate the push path trusts, from the one caller holding the value expression. 11_prime_sieve drops from 8 emitted addref calls to 1. Mini, both binaries built in one run, interleaved, min of 5, self-timed: a boolean-store loop 215 -> 207 ms, 11_prime_sieve 28 -> 26 ms -- about 4% each. Worth stating plainly: that is not where the gap lives. The same run puts node at 12 ms on the boolean-store loop against perry's 207, so the per-store typed-feedback guard CALL is the real cost; this removes dead work beside it. Differential vs node targets the exact hazard the addref prevents: a refcount-1 string stored into a slot then mutated through the source local (directly and via a loop-written array), boolean/number/null/undefined stores, the sieve shape, and a string slot overwritten by a boolean and back. Byte-identical. 31 codegen suites pass. Claude-Session: https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
|
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 (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughIndexed array stores now derive whether the value may be a heap string and pass that flag separately from the layout-note flag. New emitter wiring skips unnecessary string addref generation for provably non-string values. ChangesIndexed Store Optimization
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The PR removes redundant string-ownership work for provably non-string indexed stores while preserving protection for values that may be strings. No actionable merge-blocking risk remains after 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, on-topic, and covers the change, motivation, measurements, correctness checks, and formatting results. It does not use the repository template headings or include the checklist, related-issue status, or exact required test commands, but the core information is mostly complete. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Gate on the Linux box, commit |
|
Merged. The risk in "skip the addref for a provably non-string value" is entirely in the word provably — a value that is actually a string, stored through a path that skipped its refcount, is a use-after-free that surfaces cycles later somewhere unrelated. So I probed exactly that: 100 strings written through the indexed store, then 40,000 object allocations with a rolling live set to force collections, then every one read back and compared. 100/100 survived under default, Validation: One thing that is not yours: |
…loop -33%, prime_sieve -23%) layout_note_slot_aware opens with `if !value_is_pointer && !old_is_pointer`, and js_array_note_numeric_write returns once the receiver's raw-f64 bits are clear. lower_index_set_fast emitted both calls unconditionally, so a boolean[] store loop paid two calls per element to be declined -- ~82% of such a loop is per-store bookkeeping against 16% for the loop (PerryTS#9237). Both early returns are now inline, under two SEPARATE gates: * layout note + string addref + write barrier: behind may_carry_heap_pointer(new) || may_carry_heap_pointer(old). `new || old`, not `new` alone -- overwriting a pointer with a boolean is a pointer->scalar transition the runtime must still see, which is why the existing new-value-only emit_jsvalue_slot_store_pointer_tested is not usable here; * numeric-write note: keeps its own raw-f64-bits gate, because that note is what DOWNGRADES the array on its first non-numeric store -- gating it on pointer-ness would skip it forever. Mini, both binaries built in one run, interleaved, min of 5, self-timed: boolean store loop 206 -> 137 ms (-33%), 11_prime_sieve 26 -> 20 ms (-23%, 4.3x -> 3.3x node). Nested-loop read benchmark unchanged. Node is 12 and 6 ms -- the per-store guard CALL is the larger remaining piece, still open in PerryTS#9237. pointer_store_into_numeric_array_keeps_layout_note_and_barrier caught this twice and is the reason to trust it; both its assertions now follow the EDGE through the new gate blocks, the treatment PerryTS#7715 already gave the barrier assertion. Verified independently that inbounds -> gc_bookkeeping -> numnote -> barrier.maybe -> barrier is intact for a pointer store, byte-identical to node. Also drops emit_jsvalue_slot_store_scalar_aware_with_flags_on_block, added in PerryTS#9195 and left callerless here. 31 codegen suites pass; five differentials byte-identical to node. Claude-Session: https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
…loop -33%, prime_sieve -23%) layout_note_slot_aware opens with `if !value_is_pointer && !old_is_pointer`, and js_array_note_numeric_write returns once the receiver's raw-f64 bits are clear. lower_index_set_fast emitted both calls unconditionally, so a boolean[] store loop paid two calls per element to be declined -- ~82% of such a loop is per-store bookkeeping against 16% for the loop (PerryTS#9237). Both early returns are now inline, under two SEPARATE gates: * layout note + string addref + write barrier: behind may_carry_heap_pointer(new) || may_carry_heap_pointer(old). `new || old`, not `new` alone -- overwriting a pointer with a boolean is a pointer->scalar transition the runtime must still see, which is why the existing new-value-only emit_jsvalue_slot_store_pointer_tested is not usable here; * numeric-write note: keeps its own raw-f64-bits gate, because that note is what DOWNGRADES the array on its first non-numeric store -- gating it on pointer-ness would skip it forever. Mini, both binaries built in one run, interleaved, min of 5, self-timed: boolean store loop 206 -> 137 ms (-33%), 11_prime_sieve 26 -> 20 ms (-23%, 4.3x -> 3.3x node). Nested-loop read benchmark unchanged. Node is 12 and 6 ms -- the per-store guard CALL is the larger remaining piece, still open in PerryTS#9237. pointer_store_into_numeric_array_keeps_layout_note_and_barrier caught this twice and is the reason to trust it; both its assertions now follow the EDGE through the new gate blocks, the treatment PerryTS#7715 already gave the barrier assertion. Verified independently that inbounds -> gc_bookkeeping -> numnote -> barrier.maybe -> barrier is intact for a pointer store, byte-identical to node. Also drops emit_jsvalue_slot_store_scalar_aware_with_flags_on_block, added in PerryTS#9195 and left callerless here. 31 codegen suites pass; five differentials byte-identical to node. Claude-Session: https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
…loop −33%, prime_sieve −23%) (#9246) * perf(codegen): gate array-store GC bookkeeping inline (boolean-store loop -33%, prime_sieve -23%) layout_note_slot_aware opens with `if !value_is_pointer && !old_is_pointer`, and js_array_note_numeric_write returns once the receiver's raw-f64 bits are clear. lower_index_set_fast emitted both calls unconditionally, so a boolean[] store loop paid two calls per element to be declined -- ~82% of such a loop is per-store bookkeeping against 16% for the loop (#9237). Both early returns are now inline, under two SEPARATE gates: * layout note + string addref + write barrier: behind may_carry_heap_pointer(new) || may_carry_heap_pointer(old). `new || old`, not `new` alone -- overwriting a pointer with a boolean is a pointer->scalar transition the runtime must still see, which is why the existing new-value-only emit_jsvalue_slot_store_pointer_tested is not usable here; * numeric-write note: keeps its own raw-f64-bits gate, because that note is what DOWNGRADES the array on its first non-numeric store -- gating it on pointer-ness would skip it forever. Mini, both binaries built in one run, interleaved, min of 5, self-timed: boolean store loop 206 -> 137 ms (-33%), 11_prime_sieve 26 -> 20 ms (-23%, 4.3x -> 3.3x node). Nested-loop read benchmark unchanged. Node is 12 and 6 ms -- the per-store guard CALL is the larger remaining piece, still open in #9237. pointer_store_into_numeric_array_keeps_layout_note_and_barrier caught this twice and is the reason to trust it; both its assertions now follow the EDGE through the new gate blocks, the treatment #7715 already gave the barrier assertion. Verified independently that inbounds -> gc_bookkeeping -> numnote -> barrier.maybe -> barrier is intact for a pointer store, byte-identical to node. Also drops emit_jsvalue_slot_store_scalar_aware_with_flags_on_block, added in #9195 and left callerless here. 31 codegen suites pass; five differentials byte-identical to node. Claude-Session: https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT * test: track the raised barrier binding in the inventory's own self-test Raising CODEGEN_BARRIERED_BINDINGS to 3 without updating the fixture made the green baseline dirty (it plants 2) and stopped V-P9 firing (it plants a third to trigger the drift error). Baseline now plants 3 and V-P9 adds a fourth, so both cases still test what they are named for. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
A small, principled one: an indexed array store no longer emits a call that provably cannot do anything.
The gap
emit_jsvalue_slot_store_on_blockand its scalar-aware twin passlayout_note_neededfor both the layout note and the string-addref demote:So every store needing a layout note also emitted
js_string_addref_if_heap_string— includingsieve[j] = false, whose value is a boolean and can never be a heap string. The flagged emitter that separates the two already exists andarray_pushalready uses it; this adds the scalar-aware twin and threadsstore_needs_string_addref— the same predicate the push path trusts — from the one caller that has the value expression in hand.benchmarks/suite/11_prime_sieve.ts: 8 emitted addref calls → 1.Measurements, and what they honestly say
Idle Mac mini, both binaries built in one run, interleaved, min of five, self-timed:
11_prime_sieveAbout 4% each, reproducible to the millisecond across runs. This is not where either benchmark's gap lives, and the same table says why: Node does that boolean-store loop in 12 ms against perry's 207. The per-store typed-feedback guard call is the cost that matters there — a separate, larger piece of work. This change removes dead work sitting beside it.
Correctness
The differential targets the exact hazard the addref exists to prevent — a refcount-1 string aliased into a slot and then mutated through the source local:
s = "abc"; a[0] = s; s += "def"—a[0]must stay"abc";b[i] = t; t += "!");null/undefinedstores into an array (the values that now skip the addref);prime_sieveboolean-array shape itself;Byte-identical to Node. 31
perry-codegensuites pass;cargo fmt --checkclean.https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
Summary by CodeRabbit
Performance Improvements
Bug Fixes
null, andundefined.Validation