interp: correct four comments about why the frame virtualizable forces exist - #1044
Conversation
…s exist The four doc comments around the hand-placed `force_frame` calls attributed upstream's freedom from them to escape analysis, and attributed pyre's own materialization to the frame-chain walk. Both are wrong, and they are the statements that read as a licence to delete the calls. Upstream injects the force: `rvirtualizable.py:49-53 hook_access_field` genops `jit_force_virtualizable` on every redirected field access, `virtualizable.py:288-292` rewrites those into a `force_virtualizable_if_necessary` call across `translator.graphs`, and `jtransform.py:2164-2172 rewrite_op_jit_force_virtualizable` drops it again only in the graphs the codewriter looks inside. `executioncontext.rs:13-26` already said this; `:36-43` a few lines below said escape analysis instead. Pyre has no such injection: `rclass.rs buildinstancerepr` returns an error for any `_virtualizable_` class, naming `FieldListAccessor` / `_parse_field_list` parity as the blocker, and `rvirtualizable.rs should_force_field` has no callers. So the hand-placed calls are that mechanism relocated to a hand-picked consumer set. `gettopframe_nohidden` does not substitute for them. It forces the vref of the frame it starts from and then walks `f_backref` unforced, so it materializes the top frame only. That is why `locals()` works — it reports on the top frame — and `builtins.rs topframe_for_locals` claimed a per-frame `force_frame` instead, citing `executioncontext.rs:409-421`, which is now `clone_for_thread` internals. `typedef.rs` f_locals claimed `sys._getframe` gets the materialization "for free" from the same walk; it comes from `getframe`'s own `force_frame`. Comments only. Assisted-by: Claude
WalkthroughThe changes update documentation for virtualizable frame forcing, JIT behavior, ChangesFrame Materialization
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 d5b8108). 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
|
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-interpreter/src/executioncontext.rs`:
- Around line 54-59: The comments describing traceback-frame forcing are
inconsistent. In pyre/pyre-interpreter/src/executioncontext.rs lines 54-59,
clarify that tb_frame bypasses the frame-chain force but f_locals explicitly
calls force_frame_before_locals_read; make the equivalent wording change in
pyre/pyre-interpreter/src/typedef.rs lines 7214-7218 by replacing “gets neither”
with this scoped description.
🪄 Autofix
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 Plus
Run ID: 3e28717e-e2f3-4e0f-84df-1ce51a39d8ba
📒 Files selected for processing (4)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/typedef.rs
| /// [`PyExecutionContext::gettopframe_nohidden`] does not substitute. It forces | ||
| /// the VREF of the frame it starts from and then walks `f_backref` unforced, so | ||
| /// it materializes the top frame only — enough for `locals()`, which reports on | ||
| /// that frame, and nothing for a frame handed out some other way (a traceback's | ||
| /// `tb_frame`), which reaches `fast2locals` unforced; its null slots render as | ||
| /// an EMPTY mapping rather than a stale one. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one consistent description of traceback-frame forcing.
Both comments imply that a traceback-derived frame reaches f_locals without any force. The implementation explicitly calls force_frame_before_locals_read(f) before that access.
pyre/pyre-interpreter/src/executioncontext.rs#L54-L59: Clarify thattb_framedoes not receive the frame-chain force, butf_localsaccess performs an explicit force.pyre/pyre-interpreter/src/typedef.rs#L7214-L7218: Replace “gets neither” with the same scoped description.
📍 Affects 2 files
pyre/pyre-interpreter/src/executioncontext.rs#L54-L59(this comment)pyre/pyre-interpreter/src/typedef.rs#L7214-L7218
🤖 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-interpreter/src/executioncontext.rs` around lines 54 - 59, The
comments describing traceback-frame forcing are inconsistent. In
pyre/pyre-interpreter/src/executioncontext.rs lines 54-59, clarify that tb_frame
bypasses the frame-chain force but f_locals explicitly calls
force_frame_before_locals_read; make the equivalent wording change in
pyre/pyre-interpreter/src/typedef.rs lines 7214-7218 by replacing “gets neither”
with this scoped description.
Comments only — no code change, no counter moves.
Follow-up to #1025. While root-causing that defect I had to establish, from the
upstream sources, why pyre carries hand-placed
force_framecalls whereupstream's
sys._getframecarries none. Four comments in the tree answer thatquestion wrongly, and they are exactly the statements that read as a licence to
delete those calls — a change that has now been attempted, measured, and
reverted twice.
The two wrong claims
1. "Upstream needs no force because escape analysis materializes the frame."
Upstream injects the force.
rvirtualizable.py:49-53 hook_access_fieldgenopsjit_force_virtualizableon every redirected FIELD access;virtualizable.py:288-292rewrites those into aforce_virtualizable_if_necessarycall acrosstranslator.graphs; andjtransform.py:2164-2172 rewrite_op_jit_force_virtualizabledrops it again onlyin the graphs the codewriter looks inside. That is why a traced-through
getframeis correct without one.executioncontext.rs:13-26already stated this correctly. Ten lines below,:36-43stated escape analysis instead, andmodule/sys/vm.rs'sgetframedocstring repeated it while citing
:36-43as its authority.pyre has no such injection:
rclass.rs buildinstancereprreturns an error forany
_virtualizable_class —"VirtualizableInstanceRepr integration requires rclass.py FieldListAccessor/_parse_field_list parity"— andrvirtualizable.rs should_force_fieldhas no callers outside#[cfg(test)].So the hand-placed calls are that mechanism relocated to a hand-picked consumer
set, which is why they are load-bearing.
2. "
gettopframe_nohiddenrunsforce_frameon every frame it walks."It does not. Its first line is
force_vref(self.topframeref); the walk afterthat follows
f_backrefand testshide()with no force at all. So itmaterializes the top frame only.
That still explains why
locals()works —locals()reports on the top frame,verified:
locals()andvars()inside a 20000-iteration hot loop matchPYRE_NO_JIT=1on both native backends. Butbuiltins.rs topframe_for_localsclaimed a per-frame
force_frameand citedexecutioncontext.rs:409-421, whichis now
clone_for_threadinternals; andtypedef.rs'sf_localsgetset claimedsys._getframegets its materialization "for free" from the same walk, when itcomes from
getframe's own explicitforce_frame.Files
executioncontext.rsforce_frame_before_locals_readgettopframe_nohiddenscoped to the top framebuiltins.rstopframe_for_localsforce_frame→ the vref force; dead line citation droppedtypedef.rsf_localsgettopframe_nohidden" →getframe's ownforce_framemodule/sys/vm.rsgetframeVerification
cargo fmt --all -- --checkandcargo check --all --no-default-features --features dynasmboth clean. Nothing else runs differently — these are doc andline comments.
— authored by Claude
Summary by CodeRabbit
sys._getframe.