perf(codegen): #6794 follow-up (b) — skip redundant shadow-slot clears in region fast copies#6846
Conversation
…s in masked-window region fast copies A masked-window region fast copy flow-refines its numeric locals to Number and suppresses their per-statement shadow updates (`emit_shadow_slot_update_for_expr` returns early for a `masked_region_scalar_locals` member). Once such a local's shadow slot is cleared to 0 at the refinement point, suppression guarantees no later write ever sets it again — so every subsequent per-statement clear (`js_shadow_slot_set(slot, 0)`) is a redundant no-op. Each of those touches a thread-local, i.e. an `_tlv_get_addr` call, which profiling shows is the #1 runtime symbol in bcryptjs `_encipher` (the shadow-frame/slot TLS traffic is the dominant residual after follow-up (a)). Track the slots a copy has already cleared for a suppressed local (`suppressed_cleared_shadow_slots`); `emit_shadow_slot_clear` skips them. Entries are added right after the first clear, removed the instant a local leaves `masked_region_scalar_locals` (un-refine), and the set is dropped at copy end, so only region fast copies are affected and non-region code is byte-for-byte unchanged. Effect on the 16-round `_encipher` shape: ta_i32 fast copy `js_shadow_slot_set` 12 -> 4 (one clear per suppressed slot instead of one per write); whole function 29 -> 21. Sound: the skipped clears re-zero a slot that provably already holds 0, so GC roots are unchanged. Validated byte-for-byte vs Node on region16 / bcryptjs / the i32-overflow / toint32 / typed-array / crypto gap suite, AND under PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 (which panics on a missed/stale root). New gap test drives region functions while allocating each iteration so real collections fire with region frames live.
|
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 (1)
📝 WalkthroughWalkthroughMasked-window region fast copies now track cleared shadow slots, skip redundant clears, and reset suppression at refinement and copy boundaries. All ChangesShadow-slot clear suppression
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-files/test_gap_region_shadow_clear_gc.ts`:
- Around line 8-20: Restore encipher to the complete bcryptjs-style 16-round,
over-130-statement shape so it remains non-inlined with its shadow frame live,
and move the deliberate junk allocations into encipher while it is executing.
Update the regression test’s GC trigger accordingly, then validate it using the
repository parity-test scripts rather than a direct incompatible Node runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f19e391-280f-456c-8a55-69fc4a7f243a
📒 Files selected for processing (9)
changelog.d/6846-region-shadow-clear-dedup.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/src/stmt/masked_window_region.rstest-files/test_gap_region_shadow_clear_gc.ts
…w-clear GC test Addresses CodeRabbit on #6846: extend encipher to the full 16-round bcryptjs shape so it is robustly non-inlined (keeps its own shadow frame), and correct the comments — a region-versioned function is alloc-free/call-free by construction, so a GC cannot fire inside its fast copy; the skip's safety is static (suppression keeps the slot 0). The test guards the broader region+GC interaction under real and forced (PERRY_GC_FORCE_EVACUATE) collections, byte-identical to Node via the parity harness.
|
Addressed in 32e5efe:
|
1 participant
#6794 follow-up (b): skip redundant shadow-slot clears in region fast copies
Follow-up (a) (#6844) shipped the region i32-chains. Profiling bcryptjs
compareSyncunder Perry then shows the #1 runtime symbol is_tlv_get_addr— the thread-local access behind the GC shadow-frame/slot ops. This PR removes the
biggest chunk of that traffic that is provably redundant.
The redundancy
A masked-window region fast copy flow-refines its numeric locals to
Numberandsuppresses their per-statement shadow updates —
emit_shadow_slot_update_for_exprreturns early for any
masked_region_scalar_localsmember. So once such a local'sshadow slot is cleared to 0 at its refinement point, no later write ever sets it
again, yet the per-statement clear machinery re-emits
js_shadow_slot_set(slot, 0)after every write. Each is a TLS hit (_tlv_get_addr) doing nothing.The fix
Track slots a copy has already cleared for a still-suppressed local
(
suppressed_cleared_shadow_slots);emit_shadow_slot_clearskips them. Addedright after the first clear, removed the instant a local leaves
masked_region_scalar_locals, dropped at copy end — so only region fast copieschange; non-region code is byte-for-byte identical.
Effect (16-round
_enciphershape)js_shadow_slot_setjs_shadow_slot_setOne clear per suppressed slot instead of one per write. (The entry frame push +
param binds remain — eliding those for the fast path is a larger, GC-sensitive
follow-up.)
Correctness
Sound: a skipped clear re-zeros a slot that provably already holds 0, so GC roots
are unchanged. Validated byte-for-byte vs Node on region16 / bcryptjs / the
i32-overflow / toint32 / typed-array / crypto gap suite, and under
PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1(panics on a missed orstale root). New gap test
test_gap_region_shadow_clear_gc.tsdrives regionfunctions while allocating each iteration so real collections fire with region
frames live.
Wall-clock A/B was not measurable this run (the shared box was at load ~100 from
another build); the op-count reduction is the definitive metric.
Summary by CodeRabbit
Performance
Bug Fixes
Tests
encipherloop with forced garbage collection and stress allocation to verify behavior under evacuation.