fix(runtime): root async trap state across step bodies - #7836
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAsync step invocation now uses rooted mutable handles for captured promises, trap state, closures, and results. Resume paths share this logic. Relocation tests verify pointer rewriting, rejection forwarding, and trap restoration. ChangesAsync step relocation safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AsyncStep
participant RuntimeHandleScope
participant StepClosure
participant MovingGC
AsyncStep->>RuntimeHandleScope: root activation, trap, closure, and result
RuntimeHandleScope->>StepClosure: invoke async step
StepClosure->>MovingGC: allocate during step execution
MovingGC-->>RuntimeHandleScope: rewrite mutable roots
RuntimeHandleScope-->>AsyncStep: provide relocated pointers
AsyncStep->>AsyncStep: restore INLINE_TRAP and forward rejection
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
Quiet-M1 performance check completed against base 82f0e96 and PR head a0f158e. Methodology:
Results (median wall time):
The async-heavy regression is below 1% in both subjects. Marking ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/perry-runtime/src/promise/async_step.rs (2)
1408-1412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
RELOCATION_CASEneeds no mutable-root scanner.
RELOCATION_CASEis a thread-local side table that holds raw heap addresses of promises and closures. The repository rule requires agc_register_mutable_root_scannerregistration for such tables. This table is#[cfg(test)]-only and must retain pre-forwarding addresses for the test to work, so a scanner would defeat it. Add a short comment stating that exemption so a later reader does not treat it as a missed registration.Note also that this test mutates the process-global
INLINE_TRAP, so it must run underRUST_TEST_THREADS=1.As per coding guidelines: "When adding a runtime or side-table cache containing raw heap pointers, register a mutable-root scanner with
gc_register_mutable_root_scannerin the same change." and "Runperry-runtimetests single-threaded withRUST_TEST_THREADS=1because they share process-global side tables and are not parallel-safe."🤖 Prompt for 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. In `@crates/perry-runtime/src/promise/async_step.rs` around lines 1408 - 1412, Add a concise comment immediately above RELOCATION_CASE documenting that its cfg(test)-only raw-address table intentionally lacks a mutable-root scanner because the test requires pre-forwarding addresses; also note that the associated test must run with RUST_TEST_THREADS=1 due to mutating the process-global INLINE_TRAP.Source: Coding guidelines
908-924: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the
stepclosure pointer needs no re-read.
call_async_step_bodyrootscaptured_trap_nextbut notstep. This is correct only ifstepis never used afterjs_closure_call2returns. The current body satisfies that. Add a short comment so a later change does not reintroduce a stalestepuse after the call.🤖 Prompt for 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. In `@crates/perry-runtime/src/promise/async_step.rs` around lines 908 - 924, Add a short comment in call_async_step_body documenting that step is intentionally not rooted or re-read because it is not used after js_closure_call2 returns. Keep the existing rooting of captured_trap_next unchanged and make the comment explicit that future post-call use of step would require re-reading it.crates/perry-runtime/src/gc/mod.rs (1)
61-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the helper’s root coverage.
The symbols resolve in test builds. Add a doc comment stating that this helper rewrites runtime handle roots only and does not rewrite the installed
INLINE_TRAP.🤖 Prompt for 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. In `@crates/perry-runtime/src/gc/mod.rs` around lines 61 - 66, Update the test helper test_rewrite_runtime_handles_for_forwarded_objects with a doc comment stating that it rewrites runtime handle roots only and does not rewrite the installed INLINE_TRAP.
🤖 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 `@changelog.d/7836-async-step-roots.md`:
- Around line 3-4: Update the final clause of the changelog entry to describe
preventing use of pre-collection addresses or stale pointers, rather than
preventing restoration or dereferencing of relocated objects. Preserve the
existing explanation of mutable roots and moving collections, and do not add a
version header.
In `@crates/perry-runtime/src/promise/async_step.rs`:
- Around line 1442-1467: Update the relocation test teardown around
relocating_step and TrapGuard::drop to snapshot each forwarded source object’s
first payload word before forwarding, restore those words afterward, and
explicitly clear GC_FLAG_FORWARDED on all three source objects. Add the minimal
local cleanup needed since the forwarding API has no clear helper, while
preserving the existing INLINE_TRAP and RELOCATION_CASE restoration.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 61-66: Update the test helper
test_rewrite_runtime_handles_for_forwarded_objects with a doc comment stating
that it rewrites runtime handle roots only and does not rewrite the installed
INLINE_TRAP.
In `@crates/perry-runtime/src/promise/async_step.rs`:
- Around line 1408-1412: Add a concise comment immediately above RELOCATION_CASE
documenting that its cfg(test)-only raw-address table intentionally lacks a
mutable-root scanner because the test requires pre-forwarding addresses; also
note that the associated test must run with RUST_TEST_THREADS=1 due to mutating
the process-global INLINE_TRAP.
- Around line 908-924: Add a short comment in call_async_step_body documenting
that step is intentionally not rooted or re-read because it is not used after
js_closure_call2 returns. Keep the existing rooting of captured_trap_next
unchanged and make the comment explicit that future post-call use of step would
require re-reading it.
🪄 Autofix
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: 6cc758c4-2681-4c7a-a4e2-1fdfd18e1de3
📒 Files selected for processing (3)
changelog.d/7836-async-step-roots.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/promise/async_step.rs
|
Addressed the review findings in 32b8034:
Validation:
|
1 participant
Closes #7794
Summary
Validation
Performance
Quiet-mini timing is pending because /Users/perry/bench.lock is owned by triage-agent. This PR remains draft until the async hot-path cost is measured.
Summary by CodeRabbit
Bug Fixes
Tests