jit: emit the lazy virtualizable store, and publish walk-time locals at a traceback escape - #1060
Conversation
WalkthroughThe PR unifies lazy field and array store emission, publishes virtualizable locals during traceback creation, adds explicit virtual-reference root scanning, adds exception traceback regression coverage, updates JIT statistics, and relaxes singleton enumeration test assumptions. ChangesLazy-store unification
Traceback frame state
Immortal singleton enumeration test
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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 9b9f539). 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
|
`walk_pyframe_roots_area` skips the PyObject-shaped raw walks for a `locals_cells_stack_w` slot holding a `JitVirtualRef`, whose leading word is the vtable magic rather than an `ob_type`. That guard sat inline in the loop body with nothing in `cargo test` reaching it, and the bench fixture that exercised it does not run in the suite. Extract the slot body as `walk_frame_value_slot` — behaviour unchanged, both existing comments carried over — and test it against a hand-built vref. The test asserts the visitor is handed exactly the slot and then the vref's own `forced` field. Removing the early return aborts the test with SIGABRT on the type-pointer read, so the assertion is a gate rather than a restatement. Assisted-by: Claude
…at a traceback escape `OptHeap::emit_lazy_setfield` returned without emitting whenever the stored value was still virtual and the target was the standard virtualizable frame, so the per-slot `SETARRAYITEM_GC` that `gen_store_back_in_vable` (`rpython/jit/metainterp/pyjitpl.py:3489-3520`) writes at a trace exit was deleted for every slot holding a virtual box. `force_lazy_set` (`rpython/jit/metainterp/optimizeopt/heap.py:122-145`) emits unconditionally; the virtual-rhs skip belongs to `force_lazy_sets_for_guard` (heap.py:608-637), which routes such an op into `rd_pendingfields` instead of dropping it. Removing the carve-out leaves the `get_rhs` parameter, `field_get_rhs`, `array_get_rhs` and `writes_into_virtualizable` unused; they are deleted. The recording walk steps a snapshot of the frame, so a `STORE_FAST` it performs does not reach the live frame. `record_top_level_application_traceback` and `record_inline_application_traceback` store that frame into a `PyTraceback`, which escapes the virtualizable, so both now write the locals region first — the top level through `flush_locals_region_to_frame`, the inlined level through a new `CalleeLocalsShadow`-backed twin. The neighbouring `disarm_folded_inline_callee_after_escape` covers only the strict fresh-frame fold and is inert on the multiframe path. A callee that binds a local on one branch and raises answered `tb_frame.f_locals` with its parameters alone: 859 of 4000 iterations before, 0 after, identical on dynasm and cranelift. Adds `pyre/bench/synth/exception_vable_frame_virtual_local.py`. Re-records `closure_per_call.wasm` (guard_failures 471 -> 468), `pickle_ctor_args.cranelift` (436 -> 201), and the missing `exception_escape_hot_callee_tb_node_once.wasm` baseline. Assisted-by: Claude
…lity `immortal_singleton_enumeration_reports_created_and_forces_none` asserted that two consecutive `for_each_immortal_exception_singleton` passes report the same set. The slots are process-global `OnceLock`s and the test binary runs its cases concurrently, so a sibling case creating another `ExcKind` between the two passes fails the equality on its own; it did on the ubuntu runner. Assert instead that the second pass still reports every singleton the first one did and stays within `EXC_KIND_COUNT + 1`. The reads go through `OnceLock::get`, which is what makes the enumeration allocation-free. Assisted-by: Claude
youknowone
changed the title
![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: 1dca827562
ℹ️ 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".
| // publish the walk's locals first. `virtualizable.py:101-138 | ||
| // write_boxes` makes that write unconditional, and `pyopcode.py:148` | ||
| // performs it before attaching the application traceback. | ||
| crate::state::flush_locals_region_to_frame(ctx.trace_ctx, frame_ptr); |
There was a problem hiding this comment.
When a full-body walk records a traceback and is later discarded, this new concrete flush writes the walk snapshot's locals into the live frame without the escape-flush undo that state::flush_locals_region_to_frame expects its callers to have armed. The traceback head itself is journaled and rolled back for non-committing walks, but these frame-slot writes are not, so a declined/rolled-back trace can leave the interpreter replaying with speculative locals already published. Please snapshot/restore the frame state around this traceback-escape flush, as the residual escape path does before calling the same helper.
Useful? React with 👍 / 👎.
…ete half `flush_callee_locals_region_to_frame` skipped every slot absent from `CalleeLocalsShadow::concrete`. That map is cleared by `set_concrete` when the stored value carries no concrete half, so "absent" covered both a slot the walk never wrote and a slot it wrote with an unknown value; the second left the frame's stale value visible in the traceback. `opref` witnesses the write in both cases, so the validation pass now declines when a locals slot has an `opref` entry without a `concrete` one, and also on `Value::Ref(GcRef::NO_CONCRETE)`. Extracted as `callee_locals_region_is_publishable` with a unit test. Assisted-by: Claude
Codex parity review — disposition§1 Regressions: none reported. §2 Other mismatches (1) — fixed in-session.
The finding is real. Upstream's
Only the second case is a defect, and §3 Pre-existing (2) — both deferred, blocker named.
§4 Structural adaptations (2) — both won't-fix, documentation confirmed present.
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5becf19bd
ℹ️ 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".
| // first. `virtualizable.py:101-138 write_boxes` makes that | ||
| // write unconditional before `pyopcode.py:148` attaches the | ||
| // application traceback. | ||
| flush_callee_locals_region_to_frame(ctx, frame_ptr, frame_reg); |
There was a problem hiding this comment.
When an inlined callee has a materialized frame but flush_callee_locals_region_to_frame declines (for example a locals slot was written with no concrete half, a foreign frame register, or an unavailable shadow), this branch still records that live frame into the traceback. Because the decline leaves the frame array untouched, tb_frame.f_locals can expose stale locals from before the walk; the branch should fall back/decline instead of attaching the unpublishable frame.
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mod.rs`:
- Around line 880-886: In the frame-array update path, replace the direct
`as_mut_slice()[abs] = boxed` assignment with `FixedObjectArray::set_ref`,
ensuring the value is rooted and the array barrier runs before the store.
Preserve the existing `frame_array_write_barrier` call afterward so the
enclosing frame remains marked.
In `@pyre/pyre-object/src/interp_exceptions.rs`:
- Around line 2227-2241: Strengthen the test around
for_each_immortal_exception_singleton by preventing concurrent singleton
creation: pre-initialize every reportable exception slot or acquire the shared
test lock used for singleton creation. Then require the second enumeration to
contain exactly the previously observed pointers, while retaining the existing
boundedness and real-object checks.
🪄 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: 4077a0a4-8287-41d6-9738-ba2ec91a6737
📒 Files selected for processing (12)
majit/majit-metainterp/src/optimizeopt/heap.rspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.wasm.jitstatspyre/bench/synth/exception_vable_frame_virtual_local.cranelift.jitstatspyre/bench/synth/exception_vable_frame_virtual_local.dynasm.jitstatspyre/bench/synth/exception_vable_frame_virtual_local.pypyre/bench/synth/exception_vable_frame_virtual_local.wasm.jitstatspyre/bench/synth/pickle_ctor_args.cranelift.jitstatspyre/pyre-interpreter/src/eval.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-object/src/interp_exceptions.rs
| let boxed = crate::state::boxed_slot_value_for_type(Type::Ref, &concrete.value); | ||
| unsafe { | ||
| (*arr_ptr).as_mut_slice()[abs] = boxed; | ||
| } | ||
| // Boxing an Int/Float slot allocates, and each minor collection | ||
| // consumes the array's remembered-set entry, so re-arm per store. | ||
| crate::state::frame_array_write_barrier(frame as *mut u8, arr_ptr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the GC-aware array store before the frame barrier.
Line 882 stores boxed into an old frame array before Line 886 marks the array. A concurrent minor collection can run between these operations and miss the new young reference. This can leave a dangling frame-local pointer.
Use FixedObjectArray::set_ref for the slot store. It roots the value and performs the array barrier before the store. Keep frame_array_write_barrier afterward to mark the enclosing frame.
Proposed fix
- unsafe {
- (*arr_ptr).as_mut_slice()[abs] = boxed;
- }
+ unsafe {
+ (&mut *arr_ptr).set_ref(abs, boxed);
+ }
// Boxing an Int/Float slot allocates, and each minor collection
// consumes the array's remembered-set entry, so re-arm per store.
crate::state::frame_array_write_barrier(frame as *mut u8, arr_ptr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let boxed = crate::state::boxed_slot_value_for_type(Type::Ref, &concrete.value); | |
| unsafe { | |
| (*arr_ptr).as_mut_slice()[abs] = boxed; | |
| } | |
| // Boxing an Int/Float slot allocates, and each minor collection | |
| // consumes the array's remembered-set entry, so re-arm per store. | |
| crate::state::frame_array_write_barrier(frame as *mut u8, arr_ptr); | |
| let boxed = crate::state::boxed_slot_value_for_type(Type::Ref, &concrete.value); | |
| unsafe { | |
| (&mut *arr_ptr).set_ref(abs, boxed); | |
| } | |
| // Boxing an Int/Float slot allocates, and each minor collection | |
| // consumes the array's remembered-set entry, so re-arm per store. | |
| crate::state::frame_array_write_barrier(frame as *mut u8, arr_ptr); |
🤖 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/mod.rs` around lines 880 - 886, In
the frame-array update path, replace the direct `as_mut_slice()[abs] = boxed`
assignment with `FixedObjectArray::set_ref`, ensuring the value is rooted and
the array barrier runs before the store. Preserve the existing
`frame_array_write_barrier` call afterward so the enclosing frame remains
marked.
| // Enumerating must not initialize a slot: a second pass still reports | ||
| // every singleton the first one did, stays within the bound, and hands | ||
| // back real exception objects. The slots are process-global and the | ||
| // test binary runs its cases concurrently, so a sibling case creating | ||
| // another kind in between makes the second set a superset — comparing | ||
| // the two for equality would fail on that alone. | ||
| let mut again = Vec::new(); | ||
| for_each_immortal_exception_singleton(|exc| again.push(exc as usize)); | ||
| assert_eq!(seen, again, "enumeration must not create singletons"); | ||
| for raw in &seen { | ||
| assert!(again.contains(raw), "enumeration must not drop a singleton"); | ||
| } | ||
| assert!( | ||
| again.len() <= EXC_KIND_COUNT + 1, | ||
| "enumeration is bounded by the per-kind slots plus MemoryError" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the no-initialization assertion.
The superset check is necessary for concurrent sibling tests, but it also allows for_each_immortal_exception_singleton to create a new singleton during the second pass and still pass. Lines [2235-2237] prove only that previously observed pointers remain. The bound does not distinguish a concurrent addition from an enumeration-created entry. Pre-initialize every reportable slot or use a shared test lock for singleton creation, then assert that the second pass adds no pointer.
🤖 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-object/src/interp_exceptions.rs` around lines 2227 - 2241,
Strengthen the test around for_each_immortal_exception_singleton by preventing
concurrent singleton creation: pre-initialize every reportable exception slot or
acquire the shared test lock used for singleton creation. Then require the
second enumeration to contain exactly the previously observed pointers, while
retaining the existing boundedness and real-object checks.
Codex parity review, round 2 (
|
…tribute__ deviations (#1079) * _io: port BytesIO and StringIO from app level to interp level `class BytesIO` and `class StringIO` in `_io_app.py` are replaced by `W_BytesIO` (bytesio.rs) and `W_StringIO` (stringio.rs), following `pypy/module/_io/interp_bytesio.py` and `interp_stringio.py`. `_io_app.py` keeps only `IncrementalNewlineDecoder`. Both types hold their storage in a GC object field: `W_BytesIO` a `bytearray`, `W_StringIO` an `array('w')` of code points, standing in for the `RStringIO`/`UnicodeIO` split that exists because RPython strings are immutable. The two classes are registered at the tail of the three GC censuses (`build_gc`, `all_subclass_range_aliases`, `SUBCLASS_RANGE_HIERARCHY`) as ids 160 and 161. `tag_io_instance_with_finalizer` is split so `W_BytesIO` can pass `add_to_autoflusher=False` (interp_bytesio.py:70). Methods that can run Python (`buffer_w`, `__index__`, `dict.update`) re-derive the receiver from a pinned root afterwards, because a collection inside such a callback moves the stream and leaves the entered `&mut self` behind the forwarding pointer. lib-python `test_memoryio` goes from IMPORTERROR to 183 tests, 0 errors, 0 failures. `synth/pickle_ctor_args` runs 0.80s -> 0.28s (dynasm) and 0.84s -> 0.28s (cranelift); its jitstats and those of `synth/pickle_terminal_raise_resume` are re-recorded, both losing the function-entry loops that traced the removed app-level methods. Assisted-by: Claude * objspace: run the canonical type.__getattribute__ body directly `getattr_str_impl` reaches the metatype `__getattribute__` slot for every type receiver. `type` defines `__getattribute__`, so `getattribute_if_not_from_object` returns it and the slot was invoked through `get_and_call_function` — wrapping the name into a `w_str`, entering callable dispatch, and re-validating the name through `core::str::from_utf8` — only to reach `typeobject.py:811-828` `W_TypeObject.descr_getattribute`, whose body `object_getattr_miss` already inlines below. `is_type_getattribute_descr` recognises that descriptor by identity against `type`'s own slot (typeobject.py:1322), the same shape `is_object_getattribute_descr` uses for `object`. A metaclass that overrides `__getattribute__` keeps the descriptor-call path. 800k `getattr(SubClass, name)`, medians of 7 interleaved runs: ascii names 0.344s -> 0.238s (-31%), lone-surrogate names 0.451s -> 0.443s (the surrogate path never entered this dispatch). A 54-case type-attribute conformance probe — metaclass `__getattr__` hooks, `__getattribute__` overrides, metatype data descriptors, descriptor `__get__` raising AttributeError, abc/enum, attribute mutation, and installing `__getattribute__` on the metaclass after the fact — produces byte-identical output before and after, and matches cpython3.14 on 52 of those 54 lines. `synth/type_metatype_method_call` loses one wasm guard failure with the residual call. Assisted-by: Claude * objspace: object.__getattribute__ reads the receiver namespace, not a type's MRO `object_getattribute`'s non-instance tail delegated to `getattr_str_impl`, so a type receiver ran `typeobject.py:811-828` `W_TypeObject.descr_getattribute` — the class-MRO walk. `object.__getattribute__(Sub, "b")` therefore returned the value inherited from `Base`; cpython3.14 and pypy3 both raise AttributeError. descroperation.py:88-112 `Object.descr__getattribute__` looks the name up with `space.lookup(w_obj, name)` — the metatype for a type object — and reads only `w_obj.getdictvalue`, never the receiver type's own MRO. The type receiver now shares the instance arm with the metatype as lookup type and the type's own namespace as the receiver dict. `type.__getattribute__` keeps the MRO walk: typedef.rs routes its slot to a named `type_getattribute` instead of the object default. `attr_error_wtf8` reported `'type' object has no attribute` for a type receiver where the `&str` path already reported `type object 'Sub' has no attribute`. Both now share `missing_attribute_subject`, and the message is built as WTF-8 so a lone surrogate survives into `AttributeError.name` and `.obj`. The 54-case type-attribute conformance probe now matches cpython3.14 on every line, on dynasm and cranelift alike; it matched on 52 before. Vendored test_descr (162), test_funcattrs (35), test_descrtut, test_super (40), test_enum (1081), test_abc (72) and test_property (31) report identical counts to a build without this change. Assisted-by: Claude * jitstats: re-record the pickle fixtures the _io port moves `pickle_ctor_args` and `pickle_terminal_raise_resume` lose the function-entry loops that traced the app-level `_io.BytesIO` methods: loops_compiled 4 -> 2 and 36 -> 31 (wasm 73 -> 68), with `pickle_ctor_args` cranelift also dropping its one bridge and its guard failures 201 -> 1. `loops_aborted` is unchanged on every backend. Assisted-by: Claude * jitstats: record the four wasm guard-failure counts the rebase base moves `closure_per_call` 470 -> 468, `exception_traceback_frame_lineno` 820 -> 819, `recursive_call_frame_relocation` 649 -> 648 and `gc_iterator_source_drop` 613 -> 614 on wasm. These are not this branch's: check.py ran wasm 383/383 on the previous base with both objspace commits already applied, and the four moved only after rebasing onto 1de95e0, which carries #1060, #1072 and #1047 — all three change guard emission. Each count reproduces exactly across repeated runs, so it is a transition and not the back-edge poll oscillation. dynasm and cranelift are 388/388 either way. Assisted-by: Claude * _io: cite the close-while-exported divergence in W_BytesIO::close `interp_bytesio.py:194` `close_w` delegates straight to `RStringIO.close` with no export check, so it releases the storage under a live `getbuffer()` result. `_io.BytesIO.close` raises `BufferError: Existing exports of data: object cannot be re-sized` in that state, which the `check_exports()` call here already reproduced; only the comment naming the upstream line was missing. Comment-only change. Assisted-by: Claude
1 participant
Three fbw fixes, plus the regression fixture and the jit-stats re-records they imply.
OptHeap::emit_lazy_setfielddropped a virtualizable store. It returnedwithout emitting whenever the stored value was still virtual and the target was
the standard virtualizable frame, so the per-slot
SETARRAYITEM_GCthatgen_store_back_in_vable(rpython/jit/metainterp/pyjitpl.py:3489-3520) writesat a trace exit was deleted for every slot holding a virtual box. Upstream
force_lazy_set(optimizeopt/heap.py:122-145) emits unconditionally — thevirtual-rhs skip belongs to
force_lazy_sets_for_guard(heap.py:608-637), whichroutes the op into
rd_pendingfieldsinstead of dropping it.The walk-time traceback attach named a frame it had not published. The
recording walk steps a snapshot, so a
STORE_FASTit performs does not reachthe live frame; storing that frame into a
PyTracebackescapes thevirtualizable, so both attach sites now write the locals region first. The
inlined level needed a
CalleeLocalsShadow-backed twin offlush_locals_region_to_frame— the neighbouringdisarm_folded_inline_callee_after_escapecovers only the strict fresh-framefold and is inert on the multiframe path.
Symptom for both: a callee that binds a local on one branch and raises answered
tb_frame.f_localswith its parameters alone — 859 of 4000 iterations before,0 after, identical on dynasm and cranelift, 0 under
PYRE_NO_JIT=1.pyre/bench/synth/exception_vable_frame_virtual_local.pyguards it.The immortal-singleton enumeration test raced its siblings. It compared two
consecutive enumerations for equality over process-global
OnceLockslots whilethe test binary runs cases concurrently; it failed that way on the ubuntu
runner.
pyre/check.py: dynasm 387/387, cranelift 387/387, wasm 383/383.cargo test --workspace --features dynasm: 101 suites, 0 failed.Re-records
closure_per_call.wasm(guard_failures 471 -> 468),pickle_ctor_args.cranelift(436 -> 201), and adds the missingexception_escape_hot_callee_tb_node_once.wasmbaseline.— commented by Claude
Summary by CodeRabbit
Bug Fixes
Tests