wasm: inline CA fast paths and fully GC-managed PyFrame#522
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (19)
WalkthroughThe PR adds nursery-object queries and shadow-stack limit addresses, introduces inline CA frame allocation and trampoline-aware bridge decisions, moves PyFrame auxiliary storage into explicit GC allocation regimes, updates GC tracing and registrations, and adds wasm guest profiling plus a CA regression benchmark. ChangesRuntime nursery and shadow-stack plumbing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Interpreter
participant WasmLoop
participant GC
participant ShadowStack
Interpreter->>WasmLoop: execute CA trace
WasmLoop->>GC: query nursery and allocate frame
WasmLoop->>ShadowStack: push JitFrame
ShadowStack-->>WasmLoop: provide top and limit state
WasmLoop-->>Interpreter: reload frame and continue
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 292bce7). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19473814ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // and always targets the strictly older node. Thus an old-gen block | ||
| // never points to a younger block and needs no write barrier. | ||
| unsafe { (*node).previous = previous }; | ||
| self.lastblock = node; |
There was a problem hiding this comment.
When self is a GC-managed old-gen frame, alloc_frame_block(..., OldGenGc) normally returns a nursery FRAME_BLOCK and this assignment creates an old-to-young edge without remembering the frame. Active frames are scanned as roots, but after a generator or escaped frame suspends inside a try/finally/with block, a later minor collection can miss lastblock once the frame’s earlier remembered-set entry has been consumed, allowing the block stack node to be collected or left stale. Call the frame write barrier after publishing a GC-owned node here.
Useful? React with 👍 / 👎.
![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 `@pyre/pyre-wasm-runner/src/main.rs`:
- Around line 306-313: Update the guest profiling cleanup block around
guest_profiler.take() so File::create and p.finish errors are logged and ignored
instead of propagated with ?. Preserve processing and returning run_result even
when profile output fails, matching the error-tolerant behavior of the JIT stats
section.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd866115-33a3-4437-9835-305c32549133
📒 Files selected for processing (19)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/shadow_stack.rspyre/bench/synth/wasm_ca_trampoline_decline.pypyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-wasm-runner/src/main.rs
| if let Some(path) = &guest_profile_out { | ||
| if let Some(p) = store.data_mut().guest_profiler.take() { | ||
| let f = std::fs::File::create(path).context("create guest profile output")?; | ||
| p.finish(std::io::BufWriter::new(f)) | ||
| .context("write guest profile")?; | ||
| eprintln!("[guest-profile] wrote {path}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Profile write failure suppresses the guest run result.
File::create and finish() use ? to propagate errors. If the profile write fails, run() returns Err and the guest output stored in run_result (line 299) is never processed. This is inconsistent with the JIT stats section below, which tolerates errors via unwrap_or. Since profiling is diagnostic, its failure shouldn't lose the main result.
🛡️ Suggested fix: log profile errors instead of propagating
if let Some(path) = &guest_profile_out {
if let Some(p) = store.data_mut().guest_profiler.take() {
- let f = std::fs::File::create(path).context("create guest profile output")?;
- p.finish(std::io::BufWriter::new(f))
- .context("write guest profile")?;
- eprintln!("[guest-profile] wrote {path}");
+ match std::fs::File::create(path).context("create guest profile output") {
+ Ok(f) => match p.finish(std::io::BufWriter::new(f)) {
+ Ok(()) => eprintln!("[guest-profile] wrote {path}"),
+ Err(e) => eprintln!("[guest-profile] write failed: {e:#}"),
+ },
+ Err(e) => eprintln!("[guest-profile] create output failed: {e:#}"),
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(path) = &guest_profile_out { | |
| if let Some(p) = store.data_mut().guest_profiler.take() { | |
| let f = std::fs::File::create(path).context("create guest profile output")?; | |
| p.finish(std::io::BufWriter::new(f)) | |
| .context("write guest profile")?; | |
| eprintln!("[guest-profile] wrote {path}"); | |
| } | |
| } | |
| if let Some(path) = &guest_profile_out { | |
| if let Some(p) = store.data_mut().guest_profiler.take() { | |
| match std::fs::File::create(path).context("create guest profile output") { | |
| Ok(f) => match p.finish(std::io::BufWriter::new(f)) { | |
| Ok(()) => eprintln!("[guest-profile] wrote {path}"), | |
| Err(e) => eprintln!("[guest-profile] write failed: {e:#}"), | |
| }, | |
| Err(e) => eprintln!("[guest-profile] create output failed: {e:#}"), | |
| } | |
| } | |
| } |
🤖 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 `@pyre/pyre-wasm-runner/src/main.rs` around lines 306 - 313, Update the guest
profiling cleanup block around guest_profiler.take() so File::create and
p.finish errors are logged and ignored instead of propagated with ?. Preserve
processing and returning run_result even when profile output fails, matching the
error-tolerant behavior of the JIT stats section.
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Keep frozen frame geometry complete with a tail call area while allocating CA callee JitFrames only through their Ref homes. Assisted-by: Claude
Assisted-by: Claude
Gate a wasmtime GuestProfiler behind PYRE_WASM_GUEST_PROFILE=<out.json>: epoch interruption drives sample() from the deadline callback with a 200us ticker thread, and the profile is written in the Firefox processed format after the run. Main-module symbols only; samples land on epoch checkpoints, so bulk ops are attributed to the following checkpoint. Assisted-by: Claude
Assisted-by: Claude
Frame auxiliary resources now follow their owning frame regime: GC frames use old-gen debug data and nursery block nodes, while tracer-private snapshots retain Box allocations. Transitional try_gc_owns_object gates keep the PyFrame destructor and explicit clearing paths from freeing managed resources until the destructor is removed. Assisted-by: Claude
All frame-owned resources are GC-managed as of the two prior commits, so the PYFRAME destructor freed nothing. PYFRAME_GC_TYPE_ID now satisfies type_alloc_is_plain. Assisted-by: Claude
Tracer callbacks now enumerate slots identically in minor and major collections, matching the phase-agnostic custom-trace contract. Old-generation payload item walks at majors are idempotent duplicate marking. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cf9959a5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `previous` is written only here (and in clone/unpickle construction) | ||
| // and always targets the strictly older node. Thus an old-gen block | ||
| // never points to a younger block and needs no write barrier. | ||
| unsafe { (*node).previous = previous }; |
There was a problem hiding this comment.
When the nursery is full, alloc_frame_block can return an old-gen FRAME_BLOCK via the no-collect allocation fallback. In that case this store can put a still-nursery older block into an old-gen node without remembering the node; a later minor collection that reaches the frame's lastblock edge to this old node will not scan previous, so nested try/with block stacks can lose the rest of the chain. Add a write barrier for node after storing previous when it is GC-owned; the same fallback shape also applies to the clone-chain construction.
Useful? React with 👍 / 👎.
| // it immediately with w_globals or a freshly allocated mapping. | ||
| // Remembering the completed object is harmless for Box fallback | ||
| // and keeps the old-gen debug payload visible to the next minor. | ||
| remember_frame_debug_data(self.debugdata); |
There was a problem hiding this comment.
This barrier only runs when debugdata is first created, but FrameDebugData is now a GC-managed old-gen object. After a minor collection resets its TRACK_YOUNG_PTRS flag and removes it from the remembered set, existing-debugdata writers such as setdictscope, fset_f_trace, and descr_clear mutate fields through getorcreate_debug_data without going through getorcreatedebug; storing a fresh locals dict or trace function then creates an unremembered old-to-young edge that a later minor can reclaim. Remember the debugdata object before returning it for mutation, or after each field write, not just on creation.
Useful? React with 👍 / 👎.
1 participant
Two related performance cycles on the wasm CALL_ASSEMBLER path, plus the diagnostic tooling used to find them. Cumulative interleaved A/B on fib(32) with
PYRE_WASM_CA=1: 0.70s → 0.37s (medians; each cycle measured against its own saved pre-cycle baseline under identical load).Part 1 — wasm CA residual levers (follow-up to #501)
59f275fe24f): the direct CA arm previously made six helpercall_indirects per call (alloc, reload ×4, pop), each through a thread-local RefCell. Now the nursery bump,GcHeader/JitFrame init,memory.fillpayload clear, and jf shadow-stack push/pop/reload are emitted inline (assembler.py:1122-1136_call_header/footer_shadowstack, rewrite.py nursery fast path). Gated off under gc_stress, non-plain types, and over-size frames.c27531164d0,cf1b5e42204): the hostjit_calltrampoline writes results through the pre-call frame pointer and never reloads local 0, so a non-i64-ABI residual call (CallF/void/Newstr) on a movable CA frame could use a stale frame after a collecting call. CA compilation is now declined when the source loop, the CA bridge, or any chained bridge (cumulative census, OR-ed at bridge registration) uses the trampoline. Diag slot 15 (decl_ca_trampoline) + regression benchwasm_ca_trampoline_decline.py.88b967cdcb1): tail-call-area layout[value slots | dispatch key | Ref homes | call area]— the full geometry keeps the arena/bridge-compatible size, while CA callee allocation stops at the homes (544 → 392 bytes at the frozen floors). Sound because the trampoline decline guarantees no trampoline op runs on a CA frame. fib(32) minors 1243 → 987.eee449661a1): the generic post-collecting-call frame reloads (helpercall_indirect, ~14M/run on fib(32)) now emit the jf-top load inline when the top-cell address is baked.c5586739ba7):PYRE_WASM_GUEST_PROFILE=<out.json>drives a wasmtime epoch-based sampling profiler (200µs ticker, Firefox processed format). This is what attributed the remaining GC subtree cost to the PyFrame destructor complex below.Part 2 — fully GC-managed PyFrame (destructor removal)
Profiling showed ~30% of the fib(32) CA run inside the PyFrame destructor complex: every frame allocation was pushed onto
young_objects_with_destructorsand swept per minor (~7M dead frames/run), and the destructor made PYFRAME ineligible fortype_alloc_is_plain, forcing every JIT-built frame through the runtime alloc helper. UpstreamPyFrame(W_Root)has no destructor: its locals list,FrameDebugData, and block-stack nodes are plain GC objects. The destructor existed only to free the three malloc'd resources of interpreter/generator call frames:89491b1ba67— call-frame locals arrays move fromstd::allocblocks to old-gen GC arrays (PY_OBJECT_ARRAY), same generation and lifetime as the owning frame, so the existing frame-level barrier discipline is preserved unchanged. Tracer-private snapshot frames keepstd::allocarrays (they are not GC-traced). The frame custom trace visits the array field slot and walks old-gen items in place (interpreter stores do not write-barrier the array).06200a87645—FrameDebugDatabecomes an old-gen GC object (tid 103) and block-stack nodes become nursery GC objects (tid 104,previousis construction-only and always strictly older, so it needs no write barrier). Representation follows the owning frame's regime; snapshot clones stay boxed.hidden_operationerris now traced (no writer can store a heap ref today; traced defensively).74b4a720a41— with all frame-owned resources GC-managed, the destructor freed nothing; it is deleted. PYFRAME now satisfiestype_alloc_is_plain: no destructor-list bookkeeping per frame, and the wasm/dynasm/cranelift plain-allocation fast paths inline-bump frame allocation.19473814add— removes the collection-phase thread-local the first slice introduced: RPython custom tracers are phase-agnostic (jitframe.py:104jitframe_trace; the collection kind lives in the callback, base.py:335), so the in-place item walk now runs in both collection kinds — at majors it is idempotent duplicate marking.Verification
Per slice and at tip: wasm / dynasm / cranelift synthetic suites 153/153 each; fib(32) output
2178309withgc_minors=987,gc_majors=0invariant; trampoline-decline bench exact output withdecl_ca_trampoline=1; generator/try-finally parity benches;MAJIT_GC_STRESS=1 cargo test -p pyre-jit --features dynasm,gc_stress -- --test-threads=1(serial; forced collections shake out the moving-GC paths).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests