jit: restrict the LOAD_LOCALS residual to portal jitcodes; record three converged jitstats baselines - #1014
Conversation
In a non-portal callee `frame_var` aliases the outermost frame, the same aliasing the LoadGlobal namespace split documents, where it resolved the caller's `names` table. Threading the callee's own frame is not available as a remedy: an inlined callee has no materialised frame at all (`frame_ptr == 0`, `portal_frame_reg` unseeded), which is what inlining a virtualizable means. The two opcodes differ in whether the receiver's identity can change the answer, so they no longer share one arm: `load_locals` returns that frame's own `w_locals` via get_or_create_w_locals, so an aliased receiver is a wrong value that no guard can catch, and there is no frame-free way to compute it. It keeps the residual in portal jitcodes and declines otherwise. `load_build_class` reads frame.get_builtin(), which with honor__builtins__ false is space.builtin for every frame (baseobjspace::frame_builtin_obj), so the answer does not depend on which frame asks. It stays unrestricted. That is a property of the flag rather than of this arm, so a const assertion on HONOR_BUILTINS now fails the build, naming this site, if the flag is ever flipped. Restricting both instead was measured and rejected: it cost slots_class_var_conflict 0 -> 5, list_length_hint_validate guard_failures 1 -> 13427, and two fixtures' loops_compiled. Splitting them costs none of that — every fixture the residual improved keeps its value. check.py: dynasm 371/371, cranelift 371/371, wasm 366 passed. The one wasm failure in that run, exception_try_call_inlined_callee_raise loops_compiled 3 -> 2, is inherited: that fixture's bytecode contains neither opcode, and the same failure appears with the rejected variant above. Assisted-by: Claude
…and #960 Three baselines moved when those two landed. Each is a backend converging onto values the other backends already recorded, not a loss, so each is recorded rather than investigated as a regression. exception_try_call_inlined_callee_raise: cranelift and wasm both drop loops_compiled 3 -> 2 and guard_failures to 201, which is what the dynasm baseline already held. All three backends now carry byte-identical numbers for this fixture (bridges_compiled=1 guard_failures=201 loops_aborted=0 loops_compiled=2), and wasm additionally loses its extra loops_aborted=1. The declining walk reports the same census reason on dynasm and wasm, P2Drain::CompileRootRaise=1. exc_in_loop_divzero_continue: wasm was the outlier at bridges_compiled=0, guard_failures=360 against dynasm's and cranelift's 3 and 603, and now matches both. The guard_failures rise is a consequence of compiling those three bridges rather than none, so it accompanies more compiled coverage, not less. Both fixtures' output is byte-identical to python3.14 on every backend. check.py at this tree: dynasm 371/371, cranelift 371/371, wasm 367/367. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (4)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2db6410938
ℹ️ 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".
| if is_locals && !is_true_portal { | ||
| push_fresh_ref(&mut current_state, &mut graph); | ||
| current_depth += 1; | ||
| emit_abort_permanent!(py_pc); |
There was a problem hiding this comment.
When a hot portal reaches an inlined non-portal class body containing LOAD_LOCALS, this branch puts abort_permanent in the callee jitcode; loop_inlines_abort_permanent_callee then declines resolvable caller loops up front, while other paths abort when they reach the opcode, so these loops can never JIT. This avoids the wrong-locals result but leaves the underlying frame-identity collapse in place as a permanent performance workaround. Non-portal jitcodes need their own red frame and should execute load_locals against that frame rather than retiring the trace.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2db6410). 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
|
Two commits, both follow-ups to #1009.
jit: restrict the LOAD_LOCALS residual to portal jitcodesAddresses the P1 left on #1009's review.
In a non-portal inlined callee
frame_varaliases the outermost frame — thesame aliasing the
LoadGlobalnamespace split documents, where it resolved thecaller's
namestable and produced a silent miscompile. Threading the callee'sown frame is not available as a remedy: an inlined callee has no materialised
frame at all (
frame_ptr == 0,portal_frame_regunseeded), which is whatinlining a virtualizable means.
The two opcodes differ in whether the receiver's identity can change the answer,
so they no longer share one arm:
load_localsreturns that frame's ownw_localsviaget_or_create_w_locals, so an aliased receiver is a wrong value that noguard can catch, and there is no frame-free way to compute it. Residual in
portal jitcodes, permanent decline otherwise.
load_build_classreadsframe.get_builtin(), which withhonor__builtins__false isspace.builtinfor every frame(
baseobjspace::frame_builtin_obj), so the answer does not depend on whichframe asks. Unrestricted. That is a property of the flag rather than of this
arm, so a
constassertion onHONOR_BUILTINSnow fails the build, namingthis site, if the flag is ever flipped.
Restricting both was measured and rejected — it cost
slots_class_var_conflictloops_aborted 0 -> 5,list_length_hint_validateguard_failures 1 -> 13427, and two fixtures'loops_compiled. Splitting themcosts none of that: every fixture the residual improved keeps its value.
bench: record the cranelift and wasm jitstats converged onto by #1003 and #960Three baselines moved after #1003 and #960 merged. All three are a backend
converging onto a value another backend already recorded, not a regression:
exception_try_call_inlined_callee_raise(cranelift)guard_failuresloops_compiledexception_try_call_inlined_callee_raise(wasm)guard_failuresloops_abortedloops_compiledexc_in_loop_divzero_continue(wasm)bridges_compiledguard_failuresEvidence that these are convergence:
exception_try_call_inlined_callee_raisebaselines are nowbyte-identical (
bridges_compiled=1 guard_failures=201 loops_aborted=0 loops_compiled=2).(
P2Drain::CompileRootRaise: 1) — the numbers and the reason agree.exc_in_loop_divzero_continue'sguard_failuresrise accompaniesbridges_compiled 0 -> 3: more compiled coverage, not less.Verification
check.py: dynasm 371/371, cranelift 371/371, wasm 367/367.Measured on a full rebuild at HEAD with all four LLBC fingerprints verified, and
artifact mtimes re-checked before and after each run to confirm no concurrent
rebuild intervened.