jit: walker-native range FOR_ITER-next specialization with polymorphic demotion#545
jit: walker-native range FOR_ITER-next specialization with polymorphic demotion#545youknowone wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 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 (7)
WalkthroughAdds walker-native specialization for ChangesRange FOR_ITER specialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FOR_ITER dispatch
participant Walker specialization
participant W_IntRangeIterator
participant Trace failure tracking
participant JIT driver
FOR_ITER dispatch->>Walker specialization: specialize ForIterNext
Walker specialization->>W_IntRangeIterator: update current and remaining
W_IntRangeIterator-->>FOR_ITER dispatch: boxed item or NULL
FOR_ITER dispatch->>Trace failure tracking: record specialized guard failure
Trace failure tracking->>JIT driver: decline and invalidate loop
JIT driver-->>FOR_ITER dispatch: resume in blackhole
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 6b9a19b). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 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: e3f69bf2f3
ℹ️ 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".
| let specialized = | ||
| RANGE_FORITER_SPECIALIZED_GUARDS.with(|s| s.borrow().contains(&(green_key, fail_index))); | ||
| if specialized { | ||
| fbw_decline(green_key); |
There was a problem hiding this comment.
When this fires for a polymorphic FOR_ITER, it puts the green key into FBW_DECLINED_KEYS; however trace_bytecode checks that set before calling full_body_walk_trace and otherwise returns TraceAction::Abort through the retired trait path (trace.rs lines 621-651). That means a site that first traced as range and later sees a list/generator never gets the intended retrace with the generic ForIterNext residual after invalidation; every hot retry just skips the walker and aborts. This needs a separate range-demotion set that only makes try_walker_specialize_for_iter_next return None.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
single-walker
branch
from
e3f69bf to
d08bbf1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d08bbf170f
ℹ️ 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".
| let guard_fail_index = ctx.trace_ctx.num_guards() as u32; | ||
| let type_const = ctx.trace_ctx.const_int(range_iter_type_addr); | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardClass, &[iter_op, type_const], 0); |
There was a problem hiding this comment.
This captures the range guard's ordinal from the recorder before optimizeopt runs, but handle_fail compares it with the backend fail_index assigned later by build_guard_metadata over the optimized compiled_ops. When any earlier guard is removed or folded (for example a heapcached class/null guard), the range guard's runtime index shifts, so a polymorphic list/seq iterator will miss the demotion path, or an unrelated guard at the old ordinal can be treated as the range guard. The demotion marker needs to be tied to a stable guard descriptor/trace identity or filled after guard indices are finalized.
Useful? React with 👍 / 👎.
…c demotion Replace the opaque jit_range_iter_next_or_null residual (EffectInfo::MOST_GENERAL, w_int_new-boxing every iteration) with inline getfield/IntAdd/SetfieldGc IR for range iterators, mirroring W_IntRangeIterator.next. The produced item stays boxed (Phase 1); the cursor advance is forward-delivered through fbw_foriter_inflight_capture, not journaled, so abort reconciliation is unchanged. Non-range iterators keep the residual. A GuardClass(RANGE_ITER) at the FOR_ITER loop header storms when the site is later driven by a non-range iterator: the guard fails on every re-entry and the bridge cannot close. A monomorphic range loop never fails its own class guard, so the first failure of that guard witnesses polymorphism. On that failure handle_fail invalidates and removes the compiled loop and demotes the FOR_ITER green key. Demotion records the key in a separate RANGE_FORITER_DEMOTED set rather than fbw_decline: the full-body walk still runs on retrace and compiles the generic polymorphic-tolerant residual — only try_walker_specialize_for_iter_next declines the demoted site — so a demoted polymorphic site compiles one generic loop instead of going interpret-only. The range class guard's per-trace ordinal is tracked per green key in a green_key -> fail_index map that a retrace overwrites in place, so a non-range body guard that happens to share a stale ordinal cannot be conflated with the range guard and spuriously demote a monomorphic range site. Function-scoped `for i in range(30_000_000): s += i` drops from 0.47s to 0.14s. Assisted-by: Claude
The single-walker compile path (compile_loop_body via UnrollOptimizer) collected quasi-immutable field dependencies in its phase-1/phase-2 optimizers but never returned them, so last_quasi_immutable_deps stayed empty and register_quasi_immutable_deps registered no module-dict version watcher. A new module-global key insert then bumped the dict version without flipping the compiled loop token's invalidation flag, so the loop was neither re-enterable (is_compatible mismatch -> run-compiled-skip every iteration) nor recompiled, and it interpreted under JIT tax. UnrollOptimizer::optimize_trace_with_constants_and_inputs_vable_at now returns the merged phase-1/phase-2 quasi_immutable_deps; compile_loop_body stores them into last_quasi_immutable_deps (compile.py:234) before register_quasi_immutable_deps runs. maybe_compile / maybe_compile_with_key evict a cell whose token has been invalidated (has_seen_a_procedure_token && get_procedure_token() is None) via cleanup_chain, and a retrace no longer reuses an invalidated loop's front_target_tokens, so the next entry compiles a fresh loop against the current version (warmstate.py:483-500). global_reassign 0.56s->0.03s, global_reassign_obj 0.53s->0.02s; the loop recompiles once per version bump (loops_compiled=4). run-compiled-skip storm 900000->0. check.py 181/181 on dynasm and cranelift. Assisted-by: Claude
youknowone
force-pushed
the
single-walker
branch
from
d08bbf1 to
310fa58
Compare
1 participant
The
single-walkerline, containing a single commit on top ofmain.jit: walker-native range FOR_ITER-next specialization with polymorphic demotion
jit_range_iter_next_or_nullresidual (EffectInfo::MOST_GENERAL,w_int_new-boxing every iteration) with inlinegetfield/IntAdd/SetfieldGcIR for range iterators, mirroringW_IntRangeIterator.next. The produced item stays boxed (Phase 1); the cursor advance is forward-delivered throughfbw_foriter_inflight_capture, not journaled, so abort reconciliation is unchanged. Non-range iterators keep the residual.GuardClass(RANGE_ITER)at the FOR_ITER loop header: when the site is later driven by a non-range iterator the guard storms and the bridge cannot close. Since a monomorphic range loop never fails its own class guard, the first failure of that guard witnesses polymorphism.handle_failinvalidates the loop and declines the FOR_ITER green key (fbw_decline), retracing as the polymorphic-tolerant residual; the specialization declines a demoted site on retrace. Keyed on(green_key, fail_index)so an unrelated body guard in the same loop is not conflated.for i in range(30_000_000): s += idrops from 0.47s to 0.14s.Touches
pyre/pyre-jit-trace/src/jitcode_dispatch.rs,pyre/pyre-jit-trace/src/trace.rs, andpyre/pyre-jit/src/eval.rs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes