jit: trace a nested _getframe chain through its virtualref pairs - #1401
jit: trace a nested _getframe chain through its virtualref pairs#1401youknowone wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used all 2 included reviews currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (33)
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 ab29330). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`try_walker_specialize_load_attr`'s `f_locals` arm folds the getter for the
standard virtualizable as well as for an inline callee's own frame. The
residual getter it replaces carries a read barrier, and that force was the
only writer of `locals_cells_stack_w` out of the virtualizable image. pyre's
`FrameLocalsProxy` reads that array rather than copying out of it at the call,
so a local the traced body had assigned read back UNBOUND and the proxy
dropped it -- `frame_inlined_callee_own_image_regression` reported `own_locals`
as `('x',)` once the trace compiled.
`pyframe.py fast2locals` is `@jit.unroll_safe`, so upstream reaches the same
mapping by reading the virtualizable boxes and neither forces nor touches the
array. The fold now performs the `pyjitpl.py synchronize_virtualizable`
(`virtualizable.py write_boxes`) write-back for the locals/cells region
itself, mirrored onto the recording-time frame and emitted into the trace
through the new `TraceCtx::vable_array_item_write_back`. A slot the shadow
cannot answer declines the whole write-back and the fold with it, leaving the
residual force; the validation pass runs before the first emission. The
operand-stack region above `nlocals` is not written back: it is unreachable
through the proxy and its shadow slots read NULL outside a merge point.
Re-records `blackhole_inlined_callee_local_after_escape_declined` on all three
backends: loops_aborted 5 -> 0, loops_compiled 1 -> 2,
fbw_blackhole_adopted_single_frame 5 -> 0.
Assisted-by: Claude
be5f12d to
1ba94b1
Compare
![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: 1ba94b1fb9
ℹ️ 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_standard_frame | ||
| && !walker_write_back_standard_frame_locals(ctx, obj, concrete_obj as usize) | ||
| { | ||
| return Ok(None); |
There was a problem hiding this comment.
Folding f_locals for the standard frame performs only this one-time shadow-to-array write-back, but the returned FrameLocalsProxy remains live and reads and writes the frame array directly. For example, after p = sys._getframe().f_locals, a later x = 2 updates only the virtualizable shadow, so p["x"] can read the value present when the proxy was created; conversely, p["x"] = 2 updates the array while a subsequent compiled LOAD_FAST x still reads the old shadow. The standard-frame fold must either provide bidirectional synchronization for the proxy's lifetime or decline/escape the virtualizable.
AGENTS.md reference: AGENTS.md:L12-L15
Useful? React with 👍 / 👎.
| for _ in 0..depth_value { | ||
| let raw = unsafe { (*scan).f_backref }; |
There was a problem hiding this comment.
When an inline frame chain contains a hidden_applevel frame, this census decrements the requested depth once per raw f_backref, whereas ExecutionContext::getnextframe_nohidden skips hidden frames without consuming a depth level. A depth whose raw-hop endpoint happens to be the standard frame can therefore pass the positive-depth gate and return that frame even though _getframe(depth) should have continued to the next visible caller; other depths unnecessarily decline. The census and emitted traversal need to implement the same hidden-frame loop as getnextframe_nohidden.
AGENTS.md reference: AGENTS.md:L184-L185
Useful? React with 👍 / 👎.
`executioncontext.py getnextframe_nohidden` hops `f_backref` and then keeps hopping while the result is hidden, without consuming a depth level. The pre-emission census took exactly one raw hop per level, so it reproduces `getframe`'s walk only on a chain that carries no hidden frame; the emitted traversal already pins that with a per-hop `guard_false(hidden_applevel)`. The census also left the emit loop's `unreachable!` reachable: that arm covers a hidden hop, and nothing ahead of it had rejected one. Assisted-by: Claude
1 participant
sys._getframe(n)withn > 0from an inlined MIFrame was residual: the walkerdeclined as soon as the recording-time
f_backrefchain crossed aJitVirtualRef, so the specialized arm covered only depth 0. The genericresidual then forced the published callee frame and the loop aborted.
What changed
try_walker_specialize_sys_getframenow walks the whole concrete chainbefore emitting anything, and admits a vref hop when the vref is still one of
MetaInterp.virtualref_boxes. For each such hop it runs the orthodox bracketaround the concrete force —
vrefs_before_residual_call, theCALL_MAY_FORCE+GUARD_NOT_FORCED, thenvrefs_after_residual_call, whichpublishes
VIRTUAL_REF_FINISH(vref, virtual)and replaces the tracked vrefwith
CONST_NULL(pyjitpl.py vrefs_after_residual_call). With that proof inthe trace,
optimize_jit_force_virtualforwards the force to the pairedvirtual frame instead of materialising a vref with a null
forcedfield, andthe
JIT_FORCE_VIRTUAL/GUARD_NOT_FORCEDpair leaves the optimized loop.Two supporting resolvers on
TraceCtx:live_virtualref_pair_for_ptr(thelist lookup implicit in
pyjitpl.py vrefs_after_residual_call) andvirtualref_virtual_for_object_ptr, which still finds the virtual box of apair whose vref half
stop_tracking_virtualrefhas already replaced withCONST_NULL.The chain census is all-or-nothing and runs before the first emission, so a
decline never leaves the residual
getframea shorter chain than theinterpreter's.
Scope of the positive-depth admission. It is gated on the walk landing on
the standard portal frame whose result is immediately consumed by
f_locals—statically preflighted by
next_op_is_f_locals_for_getframe_result. Genericpositive-depth consumers, and inline
f_lineno/f_lasti, stay residual:their single live-coordinate slot cannot describe a nested caller chain.
try_walker_specialize_load_attr'sf_localsarm accordingly accepts a secondprovable receiver — the standard virtualizable, gated on both its red box
and its concrete pointer, so no arbitrary inline callee collapses onto the
portal anchor.
The locals write-back that admission needs.
pyframe.py fast2locals— thebody behind
getdictscope, and so behindf_locals— is@jit.unroll_safe,and the
locals_cells_stack_w[i]reads it unrolls aregetarrayitem_vable_ragainst the virtualizable boxes. Upstream therefore neither forces the
virtualizable nor reads its array, which is what makes folding the getter
legitimate.
pyre answers
f_localswith the 3.14FrameLocalsProxy, which reads theframe's array lazily instead of copying out of it at the call. The residual
getter's read barrier was the only thing writing that region out, so folding
it silently dropped every local the traced body had assigned:
frame_inlined_callee_own_image_regressioncaught exactly that, withown_localsgrowing a second('x',)entry. The fold now performspyjitpl.py synchronize_virtualizable(virtualizable.py write_boxes) for thelocals/cells region itself — mirrored onto the recording-time frame and emitted
into the trace — and declines, leaving the residual force, when the shadow
cannot supply a slot. The operand-stack region above
nlocalsis deliberatelyexcluded: it is not reachable through the proxy and its shadow slots read NULL
outside a merge point.
Hidden frames decline.
executioncontext.py getnextframe_nohiddenhopsf_backrefand then keeps hopping while the result is hidden, withoutconsuming a depth level, so one raw hop per level reproduces
getframe's walkonly on a chain that carries no hidden frame — which is also what the emitted
traversal's per-hop
guard_false(hidden_applevel)pins. The census declines ona hidden hop; that arm is what the emit loop's
unreachable!had beenassuming. No fixture covers it: nothing in the tree ever sets
PyCode.hidden_applevel, and__pypy__.hidden_applevelis not exposed, soframe.hide()is currently a constantfalse.optimizer.rs—drain_extra_operations_fromdiscarded the level's pendingqueue when the drain returned
InvalidLoop.InvalidLoopunwinds the recursiveRust drain in place of RPython's exception unwinding, so the operations the
failing propagation emitted, plus this level's untouched tail, must stay visible
to the caller's unwind. They now move to
extra_operations_after.Measured
Uniform across dynasm, cranelift and wasm:
getframe_bridge_force_after_store_declinedgetframe_bridge_force_plain_declinedgetframe_inline_subwalk_multiframegetframe_residual_callee_own_frame_declinedgetframe_root_loop_force_blackhole_crn_declined..._crn_nonidempotent_declinedblackhole_inlined_callee_local_after_escape_declinedgetframe_inline_subwalk_multiframeis the regression fixture for the new path:leafreads_getframe(0).f_locals["x"]and_getframe(2).f_locals["base"],so collapsing either lookup onto the portal loses a distinct name. Its one
remaining guard failure is also present in the depth-zero-only control.
The five
*_declinedfixtures keep their historical names; each header recordswhat its shape now does and why the
_declinedhalf of the name is history.Notes
Rebased onto
origin/mainafter #1399. Every upstream citation this branch addsnames a symbol;
scripts/check-new-line-citations.py --base origin/mainisclean. Resolving each number against the vendored tree before dropping it found
two that had already rotted:
pyjitpl.py:3317pointed atinitialize_virtualizable, not thevirtualref_boxeswalk it claimed, andvm.py:54sat four lines past thef.mark_as_escaped()call it named, ongetnextframe_nohidden. A third,pyjitpl.py:3358-3367, straddledvable_and_vrefs_before_residual_callandvrefs_after_residual_call. Theremaining three resolved correctly.
A third commit on this branch fixed the two dead-token
warmstatefixtures; itwas dropped during the rebase as a duplicate of #1398, which had landed the same
fix. The resolution was verified byte-identical to
origin/main.— authored by Claude