Repository navigation
jit: cut JIT merge-point to the upstream marker path; delete the direct hook (#205 C1) - #597
Conversation
|
Warning Review limit reached
Next review available in: 44 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 (10)
WalkthroughThe PR refactors interpreter JIT marker feeding and synchronous trace compilation, adds bridge-specific register reconstruction for full-body walks, removes obsolete tracing-depth state, and updates tracing comments and marker-lowering tests. ChangesJIT tracing pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant eval_loop_jit
participant PyPyJitDriver
participant compile_and_run_once
participant trace_bytecode
eval_loop_jit->>PyPyJitDriver: jit_merge_point and can_enter_jit markers
PyPyJitDriver->>compile_and_run_once: start back-edge or function-entry tracing
compile_and_run_once->>trace_bytecode: drive synchronous trace walk
trace_bytecode-->>compile_and_run_once: return trace outcome
compile_and_run_once-->>eval_loop_jit: dispatch LoopResult
Possibly related PRs
Suggested reviewers: 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 2333808). 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)
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: c999c65677
ℹ️ 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".
| frame_root.frame(), | ||
| marker_ec, | ||
| pc, | ||
| marker_pycode, | ||
| marker_profiled, |
There was a problem hiding this comment.
The source-marker lowering slices positional arguments as greens first and reds second in try_handle_jit_marker (the new test uses [next_instr, profiled, pycode, frame, ec]), but this call passes [frame, ec, pc, pycode, profiled]. As a result every translated jit_merge_point treats the frame as gr[0] (the pycode green) and treats pycode/profiled as reds, so the walker builds green keys from the frame address and closes loops with the wrong live red arguments instead of (pycode, next_instr) and [frame, ec]. Hot loops traced through the new source-marker path will fail to attach under the runtime key or compile with a malformed entry contract.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/trace.rs (1)
590-619: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not bind a bridge resume to a future merge point.
When
entryprecedes the first marker, theor_elseselects a marker that has not executed.run_perfn_walkthen seeds and reserves its register colors instead of using the portal-metadata fallback, potentially corrupting guard-resume operands. ReturnNonewhen no preceding marker exists.Proposed fix
let mp_pc = merge_point_pcs() .filter(|&pc| pc <= entry) - .max() - .or_else(|| merge_point_pcs().filter(|&pc| pc >= entry).min())?; + .max()?;Also applies to: 1555-1628
🤖 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-jit-trace/src/trace.rs` around lines 590 - 619, Update bridge_resume_merge_point_regs so it only selects the latest jit_merge_point at or before entry; remove the fallback that chooses the earliest future marker. Return None when no preceding marker exists, allowing run_perfn_walk to use portal metadata instead.
🤖 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 `@majit/majit-translate/src/codewriter/jtransform.rs`:
- Around line 7772-7780: Update the test around try_handle_jit_marker to
construct real OpKind::Call operations using the production argument order
[receiver, frame, ec, next_instr, pycode, profiled], including the can_enter_jit
arguments, and run the complete transform path. Preserve the upstream call
structure and strict structural parity, then assert the exact marker register
bindings and that no residual calls remain.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 590-619: Update bridge_resume_merge_point_regs so it only selects
the latest jit_merge_point at or before entry; remove the fallback that chooses
the earliest future marker. Return None when no preceding marker exists,
allowing run_perfn_walk to use portal metadata instead.
🪄 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: fd44fb3c-6971-4e09-babb-e2d7182a046c
📒 Files selected for processing (8)
majit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/warmstate.rsmajit/majit-translate/src/codewriter/jtransform.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
💤 Files with no reviewable changes (1)
- majit/majit-metainterp/src/pyjitpl.rs
| let merge_target = CallTarget::method("jit_merge_point", Some("PyPyJitDriver".into())); | ||
| let merge_key = jit_marker_key_from_target(&merge_target) | ||
| .expect("source jit_merge_point method must be recognized"); | ||
| let merge_ops = transformer | ||
| .try_handle_jit_marker( | ||
| merge_key, | ||
| &[receiver, next_instr, profiled, pycode, frame, ec], | ||
| &graph, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise the actual source-call argument order.
The production calls use [receiver, frame, ec, next_instr, pycode, profiled], but this test supplies canonical [receiver, next_instr, profiled, pycode, frame, ec] and no arguments for can_enter_jit. It can therefore pass while source lowering misbinds greens/reds. Build real OpKind::Call operations and run the full transform, then assert the exact marker registers and absence of residual calls.
As per coding guidelines, preserve the upstream call structure and strict structural parity. <coding_guidelines>
Also applies to: 7794-7799
🤖 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 `@majit/majit-translate/src/codewriter/jtransform.rs` around lines 7772 - 7780,
Update the test around try_handle_jit_marker to construct real OpKind::Call
operations using the production argument order [receiver, frame, ec, next_instr,
pycode, profiled], including the can_enter_jit arguments, and run the complete
transform path. Preserve the upstream call structure and strict structural
parity, then assert the exact marker register bindings and that no residual
calls remain.
Source: Coding guidelines
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-jit/src/eval.rs`:
- Around line 4628-4634: The source call in pyre/pyre-jit/src/eval.rs lines
4628-4634 must retain its existing RPython argument order; update the lowering
logic so driver declarations reorder the post-receiver payload into the correct
green/red bindings. In majit/majit-translate/src/codewriter/jtransform.rs lines
7772-7806, construct real OpKind::Call markers in production order, run the
complete transform, and assert the exact green/red bindings for both methods
while preserving strict structural parity.
🪄 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: 828df438-7aa3-48df-8350-a6ad209eed30
📒 Files selected for processing (8)
majit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/warmstate.rsmajit/majit-translate/src/codewriter/jtransform.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
💤 Files with no reviewable changes (1)
- majit/majit-metainterp/src/pyjitpl.rs
| pypyjitdriver.jit_merge_point( | ||
| frame_root.frame(), | ||
| marker_ec, | ||
| pc, | ||
| marker_pycode, | ||
| marker_profiled, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Map source-method arguments into the driver’s declared green/red order.
The lowerer splits the post-receiver payload as the first three greens, but production emits [frame, ec, pc, pycode, profiled]. This binds frame/ec as greens and pycode/profiled as reds, corrupting merge-point register metadata.
pyre/pyre-jit/src/eval.rs#L4628-L4634: preserve the source call structure; make lowering reorder arguments by the driver declarations.majit/majit-translate/src/codewriter/jtransform.rs#L7772-L7806: build realOpKind::Callmarkers in production order, run the full transform, and assert exact green/red bindings for both methods.
As per coding guidelines, preserve the RPython call structure and strict line-by-line structural parity.
📍 Affects 2 files
pyre/pyre-jit/src/eval.rs#L4628-L4634(this comment)majit/majit-translate/src/codewriter/jtransform.rs#L7772-L7806
🤖 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-jit/src/eval.rs` around lines 4628 - 4634, The source call in
pyre/pyre-jit/src/eval.rs lines 4628-4634 must retain its existing RPython
argument order; update the lowering logic so driver declarations reorder the
post-receiver payload into the correct green/red bindings. In
majit/majit-translate/src/codewriter/jtransform.rs lines 7772-7806, construct
real OpKind::Call markers in production order, run the complete transform, and
assert the exact green/red bindings for both methods while preserving strict
structural parity.
Source: Coding guidelines
Add source-level merge markers, a synchronous marker-entry trace helper, and marker-aware main-trace reconstruction while retaining the legacy direct hook as the default path. Assisted-by: Claude Assisted-by: Codex
Make static JitCode markers the sole trace-entry path and retain runtime merge-point reconstruction only for bridge resumes. Assisted-by: Claude Assisted-by: Codex
…t per field w_code_new_with_hidden_applevel checked `!code_ptr.is_null() && (code_ptr as i64) & align_mask == 0` separately at each of its five field initializers. Hoist the check into a single `code_ptr_aligned` binding and reference it from every initializer. Assisted-by: Claude
init_typeobjects ran patch_object_class_descriptor and its four sibling patch_* passes after the TYPEOBJECT_CACHE get_or_init closure returned. get_or_init serializes only the type construction: once it is initialized, the concurrent ExecutionContext::new callers it had blocked reach the patch passes together, so the first-time type_dict_store races a sibling thread's type_dict_contains read on the shared object-type dict and panics inside indexmap on a torn index table. Guard the patch sequence with a dedicated std::sync::Once. Assisted-by: Claude
build_gc_global gated build+publish+store on a plain is_initialized()
check-then-act, so on a fresh process every thread that reached it before
the first store_singleton observed the flag unset and each ran build_gc().
build_gc calls freeze_types() and the subclass-range writeback, which
mutate the shared global PyType GC-tid table and subclassrange_{min,max}
atomics; concurrent writebacks raced a sibling thread reading those
ranges. Wrap the build in std::sync::Once.
Assisted-by: Claude
Cut the JIT merge-point to the upstream marker path and delete the legacy direct hook (#205 C1).
Commits
jit: wire marker merge entry behind hook default— add source-leveljit_merge_point/can_enter_jitmarkers (translator-lowered toJitMergePoint/LoopHeader), a synchronous marker-entry trace helper (compile_and_run_once), and marker-aware main-trace reconstruction. Gated behind a temporaryPYRE_JIT_MERGE_PATH=hook(default)|markerswitch so the marker path was proven identical to the direct hook before flipping.jit: remove legacy merge-point hook— flip to marker-only: deletejit_merge_point_hookand both feed sites, thePYRE_JIT_MERGE_PATH/JitMergePathswitch,eval_loop_jit_bridge(zero callers), and the hook-only call-depth / trace-continuation-suspension dispatch. Main traces now always enter before the static marker; runtime marker-coordinate scanning remains only for bridge resumes.Net:
434 insertions(+), 660 deletions(-). The direct runtime merge-point discovery hook is gone; the merge point is driven entirely by the marker OP the way the upstream warmspot/codewriter protocol does.Verification
pyre/check.pygreen on all three backends on the rebased tree:Marker-only reproduces the pre-flip marker reference JIT-stats exactly across the 11 named benchmarks on both native backends (
loops/bridges/aborts/guard_failures/panics), andgc_stressis 6/0.Rebased onto current
main; integrates theWalkJournalswalk-journal handoff (#592) and the env-gate cleanup (#594) —full_body_walk_tracenow carries both thejournalsparameter and the marker entry it used to gate.Unblocks #205 C2 (multi-portal driver model): no
jdindex==0hardcode; per-driver JitCode/descriptor identity and portal red-register metadata are preserved.— commented by Claude
Summary by CodeRabbit