Repository navigation
jit: virtualize empty-list first append (gh#389 (b)) - #598
Conversation
WalkthroughThe PR adds Empty-list first-append promotion with typed storage and rollback journaling, tightens FOR_ITER comprehension safety checks, rematerializes list receivers from vable state, and reconciles non-monotonic vstack lowering. ChangesList promotion and comprehension execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FOR_ITER
participant Codewriter
participant JITTrace
participant ListObject
participant PromotionJournal
FOR_ITER->>Codewriter: process comprehension append
Codewriter->>JITTrace: reload list receiver from vable slot
JITTrace->>ListObject: promote Empty list to typed strategy
JITTrace->>PromotionJournal: record promoted list reference
PromotionJournal-->>JITTrace: restore Empty strategy on rollback
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)pyre/pyre-jit-trace/src/jitcode_dispatch.rsast-grep timed out on this file 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 39546b8). 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
https://github.com/youknowone/pyre/blob/b417c1c4014ddfc1546d05c915d089af52106686/pyre-jit-trace/src/jitcode_dispatch.rs#L20596
When a trace recorded from an Integer/Float first append later sees a value with a different class, the value GuardClass/w_class guards below can fail after this helper has already emitted SetfieldGc mutations that promote the real list away from Empty. Those guards capture op.pc, so deopt re-runs the LIST_APPEND on an already-promoted receiver instead of the original Empty-list state; for example, an int-specialized trace that gets a float resumes through Integer→Object rather than Empty→Float, leaving a different list strategy/capacity visible to later operations such as list.__sizeof__. Guard the appended value's type before emitting the promotion, or resume at a state that does not re-execute the append.
https://github.com/youknowone/pyre/blob/b417c1c4014ddfc1546d05c915d089af52106686/pyre-jit-trace/src/helpers.rs#L1236
For the Integer and Float arms this hard-codes a capacity-1 block, but the concrete interpreter path this is replacing does IntArray::from_vec(Vec::new()) / FloatArray::from_vec(Vec::new()) with capacity 0 and then the first push grows to INT_ARRAY_INLINE_CAP/FLOAT_ARRAY_INLINE_CAP (8). A compiled first append therefore materializes a list with capacity 1 where the interpreter would have capacity 8; that is user-visible via list.__sizeof__ (typedef.rs uses w_list_capacity) and can also make later same-trace append decisions disagree with the concrete heap used by the walker.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 037341694a
ℹ️ 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".
| } else if !value.is_null() && pyre_object::is_plain_float_strict(value) { | ||
| ListStrategy::Float | ||
| } else { | ||
| ListStrategy::Object |
There was a problem hiding this comment.
When the trace-time first append falls through to ListStrategy::Object, the code only guards that the receiver is still Empty; it never guards the negative predicates that made Object the right target (not plain int and not strict float). A trace recorded with an object/string first item can therefore be reused with an int or float first item, and the compiled path will install Object storage even though switch_to_correct_strategy would choose Integer/Float. That changes observable behavior such as list.__sizeof__() and subsequent strategy-dependent element handling, so the Object promotion needs guards excluding the typed cases or should decline this specialization.
Useful? React with 👍 / 👎.
| }; | ||
| use crate::state::{float_gcarray_descr, int_gcarray_descr, pyobject_gcarray_descr}; | ||
|
|
||
| let cap_ref = ctx.const_int(1); |
There was a problem hiding this comment.
For Integer/Float empty-list promotion this hard-codes the traced backing block capacity to 1, but the interpreter path is switch_to_correct_strategy followed by IntArray::push/FloatArray::push; from an empty typed array that grow path allocates INT_ARRAY_INLINE_CAP/FLOAT_ARRAY_INLINE_CAP slots (8). After compiling a one-element int/float append, code that observes list capacity via list.__sizeof__() will see the JIT-created capacity-1 list instead of the interpreter's capacity-8 list, so the typed arms need to emit the same first-append allocation size as the concrete append path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f48d38937
ℹ️ 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".
| // Emit the transition IR mutating the existing wrapper (helpers.rs). | ||
| // The emitter seeds the new block's capacity getfield cache so the | ||
| // append body sub-walk's spare-capacity `0 < capacity` check folds. | ||
| crate::helpers::emit_promote_empty_list_inline(ctx.trace_ctx, self_ref, target); |
There was a problem hiding this comment.
When this specialization records an Integer/Float first append, the value-class and w_class guards are emitted only after this call has already recorded the strategy/block SetfieldGc writes. If the trace is later entered with a different first value type (for example traced with an int but reached with a float), the guard failure resumes at the append site after the live list has already been switched out of Empty, so the interpreter re-executes w_list_append from the wrong starting strategy and can leave the list in Object storage instead of the Float/Integer strategy the interpreter would have chosen from Empty. Move the target value guards ahead of the promotion writes, or make the promotion happen only after all guards that can fail have passed.
Useful? React with 👍 / 👎.
Add `empty_append_virt_enabled()` reading `PYRE_EMPTY_APPEND_VIRT` (default-off), mirroring `newlist_virt_enabled()`. The gate will admit the empty-list first append into the orthodox `w_list_append` fold in a later slice. No caller yet; `#[allow(dead_code)]` until wired. Assisted-by: Claude
Add FBW_APPEND_PROMOTE_JOURNAL recording each list promoted Empty→typed during a speculative full-body walk, wired parallel to FBW_APPEND_JOURNAL through reset/commit/rollback and the GC root walker area. Rollback drains it after the length-rewind and calls w_list_clear to restore the Empty strategy + drop the typed backing block — the strategy switch + realloc the length journal alone cannot undo. No caller yet (fbw_append_promote_journal_push is #[allow(dead_code)] until the recognize gate admits the empty append); strict no-op with the gate off. Removable when single-executor tracing lands (gh#73/#34). Assisted-by: Claude
…ated) Admit the Empty-strategy first LIST_APPEND on the FBW walker under the PYRE_EMPTY_APPEND_VIRT gate (default off). recognize classifies the append by the value's type in switch_to_correct_strategy's int->float->object order; commit installs the typed backing block on the concrete receiver, emits the Empty->typed transition as inline heap IR mutating the existing wrapper, and journals the rewind to Empty for a non-commit walk. emit_promote_empty_list_inline returns the transition's NewArray block OpRef. commit stamps it with the concrete backing-block address (new w_list_items_block_ptr) so the append body sub-walk's `list.items_block.capacity` getfield folds via field_sanity_load to the concrete capacity (1); the spare-capacity `0 < capacity` branch then resolves instead of aborting with a symbolic GOTO_IF_NOT condition. The concrete strategy switch runs before the IR emit so the backing block exists when the block OpRef is stamped. Assisted-by: Claude
Flip the PYRE_EMPTY_APPEND_VIRT gate to default-on (set PYRE_EMPTY_APPEND_VIRT=0 to restore the residual abort), matching the newlist_virt_enabled default-on convention. dynasm and cranelift both pass check.py 194/194 with the gate on. Assisted-by: Claude
…mp path Replace the NewArray-block concrete-address stamp with a heapcache capacity seed. emit_promote_empty_list_inline now seeds the new block's ItemsBlock.capacity getfield cache with the const (1) on the Integer and Float arms, so the append body sub-walk's spare-capacity `0 < capacity` check folds to the const instead of reading a symbolic getfield. This is the same shape the heapcache uses for a const-length new_array; the capacity is read through a getfield (not arraylen), so the field-index channel is seeded explicitly. Removes the emit_promote_empty_list_inline `Option<OpRef>` return, the commit-site reorder + set_opref_concrete stamp, and the now-unused w_list_items_block_ptr accessor. Object storage needs no seed (its capacity read resolves through list.items already). A/B verified both levers independently compile the empty-append loop; the seed is the smaller and matches the heapcache const-length model. dynasm+cranelift 194/194, list repros byte-identical under MAJIT_GC_STRESS. Assisted-by: Claude
…IST_APPEND admit to call-free bodies
The py3.14 inlined comprehension emits a SWAP/BUILD_LIST/SWAP preamble
before its FOR_ITER, lowered non-monotonically in jitcode: the JitCode-PC
floor pivot maps a later jit_pc back to an earlier py_pc, so the walk
re-visits those preamble py_pcs out of order (a backward py transition,
then a forward re-walk). reconcile_vstack_at_boundary replayed the SWAP
effect and reused a stale vstack_last_ref across that region, dropping the
just-built list from the vstack_boxes mirror; the stack_sync overlay then
copied the corrupt mirror into the resume snapshot, so a deopt from the
comprehension body reconstructed a frame whose FOR_ITER iterator slot held
a duplicated iterator and a dropped list ("not an iterator").
Add WalkContext.vstack_reorder_ceiling: when the previous opcode is a
Swap/Copy and the py transition is backward (a codewriter layout artifact,
never a real branch), reseed the whole mirror from the virtualizable
shadow (VstackOpClass::ShadowReseed) until the walk advances past the
py_pc it backed off from.
Admit LIST_APPEND in for_iter_bodies_all_jit_safe only when its FOR_ITER
body performs no CALL. A per-element user-frame call (a class ctor / user
function) sets the FOR_ITER in-flight body-effect flag, and a mid-body
abort then routes through fbw_foriter_inflight_take, which refuses
delivery to avoid a double-apply and drops the trace-attempt iteration's
item. Call-free comprehension bodies never enter a user frame and stay
admissible; call-bearing bodies decline to interpretation.
Assisted-by: Claude
…ack slot The codewriter lowered LIST_APPEND's list operand by cloning the peeked FlowValue from the compile-time stack. The inlined-comprehension accumulator lives on the operand stack, which is not loop-carried in registers (the jit_merge_point reds are [frame, ec]), so the preamble BUILD_LIST value is not a live register inside the resident loop. The jit_list_append residual then read an unbound register for its receiver and aborted ResidualCallArgUnbound, so the comprehension inner loop never compiled and the #171 empty-append fold was never reached. Reload the list receiver from its value-stack slot via getarrayitem_vable_r each iteration, the same per-iteration re-materialization the FOR_ITER iterator reload already uses. Call-free comprehension inner loops (`[j for j in range(n)]`) now compile with the append folded. A comprehension whose body enters a user frame (e.g. `[C(i) for i in ...]`) still declines via for_iter_bodies_all_jit_safe's call-free LIST_APPEND gate, since its in-flight FOR_ITER delivery is a separate concern. Assisted-by: Claude
A comprehension whose LIST_APPEND lands in a list Object-strategy (str/None/tuple/list/dict/f-string element) lowers the append to a Void setarrayitem_gc residual. The FBW body-effect gate marks that Void write as a body effect, so fbw_foriter_inflight_take refuses delivery of the in-flight FOR_ITER item on the trace-attempt abort and drops one iteration (len 999 vs 1000). int/float-strategy appends fold via the pure length-rewind journal and stay correct. Replace the body_has_call scan with a body_has_list_append scan: when a FOR_ITER body contains LIST_APPEND, admit only the bare-accumulator ops (LoadFast/StoreFast family, ListAppend, stack shuffles, backward jumps, ExtendedArg/Cache). Any value-producing op declines the body to the interpreter. Bodies without LIST_APPEND keep the existing predicate. Assisted-by: Claude
![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
🤖 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-trace/src/jitcode_dispatch.rs`:
- Around line 21993-22009: The empty-append strategy classification is
duplicated and can diverge between recognition, commit, and concrete promotion.
Add a shared classify_empty_append_strategy(value) -> Option<ListStrategy>
helper, use it in orthodox_list_append_recognize by mapping Some to Some(0), and
replace the local target computation in the commit path with it. Ensure
switch_to_correct_strategy uses the same classification rules, including
fits-int W_LongObject values.
🪄 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: 93b77009-d511-44ed-9ace-c70dfaba91da
📒 Files selected for processing (5)
pyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/listobject.rs
| use pyre_object::listobject::ListStrategy; | ||
| let promote_empty = empty_append_virt_enabled() | ||
| && unsafe { pyre_object::w_list_uses_empty_storage(inner_self) }; | ||
| if promote_empty { | ||
| let target = unsafe { | ||
| let int_ok = pyre_object::is_plain_int1(value) | ||
| && !pyre_object::pyobject::is_long(value) | ||
| && !(pyre_object::tagged_int::CAN_BE_TAGGED | ||
| && pyre_object::tagged_int::is_tagged_int(value)); | ||
| if int_ok { | ||
| ListStrategy::Integer | ||
| } else if !value.is_null() && pyre_object::is_plain_float_strict(value) { | ||
| ListStrategy::Float | ||
| } else { | ||
| ListStrategy::Object | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Centralize the Empty→typed strategy classification to remove a latent drift risk.
The int→float→object classification now lives in three places that must stay in lockstep:
orthodox_list_append_recognize(theint_ok/float_ok/obj_okgate),- this
targetcomputation in the commit path, and switch_to_correct_strategyinpyre-object/src/listobject.rs(the concrete promotionw_list_switch_to_strategy_forruns).
They only agree today because recognize declines the one value class where they diverge: a fits-int W_LongObject is is_plain_int1 == true, so switch_to_correct_strategy would install Integer, but this target computes Object (its int_ok adds !is_long). If recognize is ever relaxed to admit that class, the emitted IR strategy (Object) would disagree with the concrete strategy the wrapper installs (Integer), silently miscompiling the trace. Extracting a single classify_empty_append_strategy(value) -> Option<ListStrategy> shared by recognize (map Some → Some(0)) and this site removes one divergence axis and makes the remaining coupling with switch_to_correct_strategy explicit.
🤖 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/jitcode_dispatch.rs` around lines 21993 - 22009, The
empty-append strategy classification is duplicated and can diverge between
recognition, commit, and concrete promotion. Add a shared
classify_empty_append_strategy(value) -> Option<ListStrategy> helper, use it in
orthodox_list_append_recognize by mapping Some to Some(0), and replace the local
target computation in the commit path with it. Ensure switch_to_correct_strategy
uses the same classification rules, including fits-int W_LongObject values.
Summary
Resolves item (b) of #389 — a hot loop that grows an empty list via
per-iteration
LIST_APPEND(inlined comprehension or explicitfor … append)no longer aborts the FBW walker; it now compiles.
The orthodox fix follows
EmptyListStrategy.append→switch_to_correct_strategy(
listobject.py:1154): on the first append the walker promotes theEmptyreceiver to the value-typed strategy (Integer/Float/Object), records that
transition as inline heap IR, then falls through to the existing spare-capacity
append fold. No list virtualization across the back-edge (PyPy doesn't do that
for a growing accumulator either — it materializes and appends for real).
Commits (5, stacked on
main)057fe3ca9fbgate scaffoldPYRE_EMPTY_APPEND_VIRT(default-off)af7f0ec8cafdormant append-promote restore journal (FBW rollback safety; removable once single-executor tracing lands, Eliminatepc_mapand per-PC labels by switching the tracer to interpret JitCode bytecode #73/Fix merge points in fannkuch loop #34)29d6fb0c4c6recognize gate + commit promotion + Empty→typed transition IRd6d33754be8flip gate default-onb417c1c4014cleanup: fold the sub-walk's spare-capacity0 < capacitycheck via a heapcache capacity seed (const 1) on the Integer/Float arms; drops the earlier concrete-stamp pathKey detail
The append body sub-walk reads
list.items_block(heapcache-hits the promotion'sNewArrayop) thenblock.capacity. That capacity read is agetfield, adifferent heapcache channel from
new_array's arraylen seed, so it is seededexplicitly with the const (1) — matching how the heapcache tracks a const-length
new_array. The seeded const resolves throughconcrete_of_opref→inline_const_to_value, so theGOTO_IF_NOTon0 < capacityfolds instead ofaborting
GotoIfNotValueNotConcrete.Verification
check.py194/194, gate on and off.vs
PYRE_NO_JIT=1and underMAJIT_GC_STRESS=1.exec_mf) to virtualized (exec_v).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes