jit: three witnessed wrong-code fixes, builtin-subclass mapdict layouts, and a measured FOR_ITER gate widening - #1103
Merged
Conversation
|
Warning Review limit reached
Next review available in: 20 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 (18)
WalkthroughThe changes update GC subclass-range metadata, native builtin-subclass layouts, MapDict storage access, JIT specialization outcomes, FOR_ITER safety analysis, virtualizable synchronization, parity tests, and benchmark statistics. ChangesRuntime layouts and storage
JIT control flow and specialization
Validation and recorded outputs
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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 1d84385). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 177a32baa1
ℹ️ 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".
Comment on lines
+217
to
+221
| pub loop_region_jit_safe: HashMap<(usize, usize), bool>, | ||
|
|
||
| /// Immutable bytecode-derived facts used by the temporary FOR_ITER gate, | ||
| /// kept with the other per-graph codewriter metadata. | ||
| pub for_iter_bodies_jit_safe: HashMap<usize, bool>, |
There was a problem hiding this comment.
These parallel HashMap caches add three pyre-only storage attributes even though the change explicitly documents that the FOR_ITER gate has no upstream counterpart. The repository requires each new map to match a corresponding RPython/PyPy owner and container; storing these immutable facts on the equivalent existing per-graph/JitCode metadata would avoid extending the temporary gate into a second persistent data model.
AGENTS.md reference: AGENTS.md:L115-L124
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/objspace/std/mapdict.rs (1)
5018-5038: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReload the forwarded storage block before scanning it.
The visitor can forward a GC-owned
ItemsBlockand updatestorage_slot. Lines 5029-5031 then dereference the pre-forwardingstoragepointer. A moving nursery collection can make this a read from retired memory.Proposed fix
- let storage = *storage_slot; + let mut storage = *storage_slot; if pyre_object::gc_hook::try_gc_owns_object(storage as *mut u8) { f(storage_slot as *mut PyObjectRef); + storage = *storage_slot; }🤖 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/objspace/std/mapdict.rs` around lines 5018 - 5038, Update instance_walk_boxed_storage to reload the storage pointer from storage_slot after invoking f on the GC-owned storage slot, then use the reloaded pointer for capacity, base, and slot scanning. Keep the initial null check and forwarding callback behavior unchanged.
🤖 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 `@majit/majit-metainterp/src/optimizeopt/virtualize.rs`:
- Around line 3825-3855: Update
test_fresh_virtual_unwritten_fields_are_typed_zero and its run_pass test helper
to expose the value forwarded when the getfield operation is removed. Assert the
expected typed zero for each opcode: Value::Int(0), Value::Ref(GcRef::NULL), or
Value::Float(0.0), while retaining the existing assertion that no operations
remain.
In `@pyre/extra_tests/parity_tests/builtin_subclass_attr_mapdict.py`:
- Around line 115-117: Update the referent assertion in the loop over int_user,
str_user, and tuple_user to call gc.get_referents(value) directly instead of
wrapping value in a temporary list. Assert that the builtin-subclass instance
dictionary or required direct mapdict carrier is among the referents, while
preserving the existing gc.get_objects assertion.
In `@pyre/extra_tests/parity_tests/for_iter_exception_handler_comprehension.py`:
- Around line 1-25: Update Payload with stable string rendering, such as a
__str__ or __repr__ implementation, so its computed value can be compared
deterministically. Change run() to return the out and seen collections rather
than their lengths, then update the assertion to compare both against the
expected per-iteration sequences while preserving the existing loop and
comprehension behavior.
In `@pyre/pyre-interpreter/src/_structseq.rs`:
- Around line 519-535: Re-read the class from the shadow stack after tuple
allocation and pinning before assigning it to the non-hasdict object. Update the
post-allocation `w_class` assignment in the surrounding structseq construction
flow to use `shadow_stack_get(cls_slot)` rather than the cached `rooted_cls`,
while preserving the existing hasdict branching.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 3379-3382: Update the comment near emit_exception_new_inline to
accurately describe field initialization: name kind, w_class, args_w,
suppress_context, and w_context as explicitly initialized, or clarify that
w_context is written separately by walker_emit_recorded_builtin_raise. Preserve
the existing field list and offset-based lookup code unchanged.
- Around line 4204-4215: Extend the test
native_user_mapdict_fields_replace_prepass_placeholder_indices to assert the two
omitted fields: W_UNICODE_USER_DESCR_GROUP.field_descrs[1] must equal
0x6100_0011, and W_TUPLE_USER_DESCR_GROUP.field_descrs[0] must equal
0x6100_0020, covering all six minted indices.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 750-757: Rename the mapdict wrapper int_object_custom_trace to
int_user_object_custom_trace to reflect the W_INT_USER_GC_TYPE_ID layout and
match the sibling *_user_object_custom_trace naming. Update its registration
accordingly, while leaving object_object_custom_trace unchanged.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 5018-5038: Update instance_walk_boxed_storage to reload the
storage pointer from storage_slot after invoking f on the GC-owned storage slot,
then use the reloaded pointer for capacity, base, and slot scanning. Keep the
initial null check and forwarding callback behavior unchanged.
🪄 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: ddd2dc24-09cc-4b16-936c-3d7b3b60244b
📒 Files selected for processing (73)
majit/majit-gc/src/trace.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rspyre/bench/synth/binary_int_overflow_local_resume.cranelift.jitstatspyre/bench/synth/binary_int_overflow_local_resume.dynasm.jitstatspyre/bench/synth/binary_int_overflow_local_resume.wasm.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/enumerate_bignum_start.cranelift.jitstatspyre/bench/synth/enumerate_bignum_start.dynasm.jitstatspyre/bench/synth/enumerate_bignum_start.wasm.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.cranelift.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.dynasm.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.wasm.jitstatspyre/bench/synth/exception_group_type.cranelift.jitstatspyre/bench/synth/exception_group_type.dynasm.jitstatspyre/bench/synth/exception_group_type.wasm.jitstatspyre/bench/synth/exception_reused_object_tb_not_doubled.wasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstatspyre/bench/synth/exception_traceback_loop_forms.dynasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.wasm.jitstatspyre/bench/synth/global_store_plain_dict_globals.wasm.jitstatspyre/bench/synth/list_append_virtual_payload.cranelift.jitstatspyre/bench/synth/list_append_virtual_payload.dynasm.jitstatspyre/bench/synth/list_append_virtual_payload.wasm.jitstatspyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstatspyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstatspyre/bench/synth/list_append_write_barrier_gc.wasm.jitstatspyre/bench/synth/loops_comprehension.wasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstatspyre/bench/synth/minmax_key_rooting.cranelift.jitstatspyre/bench/synth/minmax_key_rooting.dynasm.jitstatspyre/bench/synth/minmax_key_rooting.wasm.jitstatspyre/bench/synth/newslice_step_hot.wasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/range_ctor_in_loop.cranelift.jitstatspyre/bench/synth/range_ctor_in_loop.dynasm.jitstatspyre/bench/synth/range_ctor_in_loop.wasm.jitstatspyre/bench/synth/reversed_disabled.cranelift.jitstatspyre/bench/synth/reversed_disabled.dynasm.jitstatspyre/bench/synth/reversed_disabled.wasm.jitstatspyre/bench/synth/unpack_ex_hot.wasm.jitstatspyre/extra_tests/parity_tests/bigint_div_raising_specialization.pypyre/extra_tests/parity_tests/bigint_shift_raising_specialization.pypyre/extra_tests/parity_tests/builtin_raise_context_specialization.pypyre/extra_tests/parity_tests/builtin_subclass_attr_mapdict.pypyre/extra_tests/parity_tests/exception_inline_scalar_fields_jit.pypyre/extra_tests/parity_tests/float_div_raising_specialization.pypyre/extra_tests/parity_tests/for_iter_exception_handler_comprehension.pypyre/extra_tests/parity_tests/int_div_mod_raising_specialization.pypyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.pypyre/extra_tests/parity_tests/range_zero_step_raising_specialization.pypyre/pyre-interpreter/src/_structseq.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/call.rspyre/pyre-object/src/boolobject.rspyre/pyre-object/src/float_array.rspyre/pyre-object/src/int_array.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/tupleobject.rspyre/pyre-object/src/unicodeobject.rs
Comment on lines
+3825
to
+3855
| #[test] | ||
| fn test_fresh_virtual_unwritten_fields_are_typed_zero() { | ||
| // virtualize.py:184-190 optimize_GETFIELD_GC_I: GC allocations are | ||
| // zero-filled, so an unset virtual field folds through | ||
| // optimizer.new_const(fielddescr) without forcing the allocation. | ||
| for (get_opcode, field_descr) in [ | ||
| (OpCode::GetfieldGcI, field_descr(0)), | ||
| (OpCode::GetfieldGcR, ref_field_descr(0)), | ||
| (OpCode::GetfieldGcF, float_field_descr(0)), | ||
| ] { | ||
| let mut ops = vec![ | ||
| Op::with_descr(OpCode::NewWithVtable, &[], size_descr(1)), | ||
| Op::with_descr( | ||
| get_opcode, | ||
| &[crate::history::test_support::rooted_resop_operand( | ||
| Type::Ref, | ||
| 0, | ||
| )], | ||
| field_descr, | ||
| ), | ||
| ]; | ||
| assign_positions(&mut ops); | ||
| let result = run_pass(&ops); | ||
| assert!( | ||
| result.is_empty(), | ||
| "{get_opcode:?} of an unset fresh field forced the virtual: {:?}", | ||
| result.iter().map(|op| op.opcode).collect::<Vec<_>>() | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed zero value, not only operation removal.
result.is_empty() confirms only that the operations were removed. It does not confirm that the result contains Value::Int(0), Value::Ref(GcRef::NULL), or Value::Float(0.0). A regression that drops the result or assigns the wrong value type would still pass. Expose the forwarded value in the test helper and assert the expected value for each opcode.
🤖 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 `@majit/majit-metainterp/src/optimizeopt/virtualize.rs` around lines 3825 -
3855, Update test_fresh_virtual_unwritten_fields_are_typed_zero and its run_pass
test helper to expose the value forwarded when the getfield operation is
removed. Assert the expected typed zero for each opcode: Value::Int(0),
Value::Ref(GcRef::NULL), or Value::Float(0.0), while retaining the existing
assertion that no operations remain.
Comment on lines
+115
to
+117
| for value in (int_user, str_user, tuple_user): | ||
| assert any(candidate is value for candidate in gc.get_objects()) | ||
| assert any(candidate is value for candidate in gc.get_referents([value])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inspect referents of the builtin subclass.
gc.get_referents([value]) always reports value because the temporary list owns it. This does not inspect the native builtin-subclass object. A broken mapdict-carrier GC trace can still pass this assertion.
Inspect gc.get_referents(value) and assert that the instance dictionary, or the required direct carrier, is present.
Proposed fix
for value in (int_user, str_user, tuple_user):
assert any(candidate is value for candidate in gc.get_objects())
- assert any(candidate is value for candidate in gc.get_referents([value]))
+ assert any(candidate is value.__dict__ for candidate in gc.get_referents(value))📝 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.
Suggested change
| for value in (int_user, str_user, tuple_user): | |
| assert any(candidate is value for candidate in gc.get_objects()) | |
| assert any(candidate is value for candidate in gc.get_referents([value])) | |
| for value in (int_user, str_user, tuple_user): | |
| assert any(candidate is value for candidate in gc.get_objects()) | |
| assert any(candidate is value.__dict__ for candidate in gc.get_referents(value)) |
🤖 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/extra_tests/parity_tests/builtin_subclass_attr_mapdict.py` around lines
115 - 117, Update the referent assertion in the loop over int_user, str_user,
and tuple_user to call gc.get_referents(value) directly instead of wrapping
value in a temporary list. Assert that the builtin-subclass instance dictionary
or required direct mapdict carrier is among the referents, while preserving the
existing gc.get_objects assertion.
Comment on lines
+1
to
+25
| class Payload: | ||
| def __init__(self, value): | ||
| self.value = value | ||
|
|
||
|
|
||
| def make_payload(value): | ||
| return Payload(value * 3 + 1) | ||
|
|
||
|
|
||
| def run(items): | ||
| out = [] | ||
| seen = [] | ||
| for index in items: | ||
| try: | ||
| items[index + 100] | ||
| except IndexError: | ||
| out.extend([str(make_payload(value)) for value in range(1)]) | ||
| seen.append(len(range(index))) | ||
| out.append(index) | ||
| return len(out), len(seen) | ||
|
|
||
|
|
||
| items = [index % 5 for index in range(60)] | ||
| for _ in range(400): | ||
| assert run(items) == (120, 60) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the computed loop data.
run() returns only two lengths. A compiled path can produce wrong payload values or wrong seen values while still performing one extend and one append per iteration.
Make Payload rendering stable. Return out and seen. Compare both collections with the expected sequence.
Proposed fix
class Payload:
def __init__(self, value):
self.value = value
+
+ def __str__(self):
+ return str(self.value)
@@
- return len(out), len(seen)
+ return out, seen
@@
items = [index % 5 for index in range(60)]
+expected_out = [item for index in items for item in ("1", index)]
+expected_seen = list(items)
for _ in range(400):
- assert run(items) == (120, 60)
+ assert run(items) == (expected_out, expected_seen)📝 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.
Suggested change
| class Payload: | |
| def __init__(self, value): | |
| self.value = value | |
| def make_payload(value): | |
| return Payload(value * 3 + 1) | |
| def run(items): | |
| out = [] | |
| seen = [] | |
| for index in items: | |
| try: | |
| items[index + 100] | |
| except IndexError: | |
| out.extend([str(make_payload(value)) for value in range(1)]) | |
| seen.append(len(range(index))) | |
| out.append(index) | |
| return len(out), len(seen) | |
| items = [index % 5 for index in range(60)] | |
| for _ in range(400): | |
| assert run(items) == (120, 60) | |
| class Payload: | |
| def __init__(self, value): | |
| self.value = value | |
| def __str__(self): | |
| return str(self.value) | |
| def make_payload(value): | |
| return Payload(value * 3 + 1) | |
| def run(items): | |
| out = [] | |
| seen = [] | |
| for index in items: | |
| try: | |
| items[index + 100] | |
| except IndexError: | |
| out.extend([str(make_payload(value)) for value in range(1)]) | |
| seen.append(len(range(index))) | |
| out.append(index) | |
| return out, seen | |
| items = [index % 5 for index in range(60)] | |
| expected_out = [item for index in items for item in ("1", index)] | |
| expected_seen = list(items) | |
| for _ in range(400): | |
| assert run(items) == (expected_out, expected_seen) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 2-2: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
🤖 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/extra_tests/parity_tests/for_iter_exception_handler_comprehension.py`
around lines 1 - 25, Update Payload with stable string rendering, such as a
__str__ or __repr__ implementation, so its computed value can be compared
deterministically. Change run() to return the out and seen collections rather
than their lengths, then update the assertion to compare both against the
expected per-iteration sequences while preserving the existing loop and
comprehension behavior.
Comment on lines
+4204
to
+4215
| #[test] | ||
| fn native_user_mapdict_fields_replace_prepass_placeholder_indices() { | ||
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[0].index(), 0x6100_0000); | ||
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[1].index(), 0x6100_0001); | ||
| assert_eq!( | ||
| W_UNICODE_USER_DESCR_GROUP.field_descrs[0].index(), | ||
| 0x6100_0010 | ||
| ); | ||
| assert_eq!( | ||
| W_TUPLE_USER_DESCR_GROUP.field_descrs[1].index(), | ||
| 0x6100_0021 | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert all six mapdict indices.
The test checks four of the six minted indices. It omits W_UNICODE_USER_DESCR_GROUP.field_descrs[1] (0x6100_0011) and W_TUPLE_USER_DESCR_GROUP.field_descrs[0] (0x6100_0020).
The purpose of this change is that all six indices stay distinct. Assert the full set so a future tag edit cannot alias an unchecked pair.
♻️ Proposed additions
assert_eq!(
W_UNICODE_USER_DESCR_GROUP.field_descrs[0].index(),
0x6100_0010
);
+ assert_eq!(
+ W_UNICODE_USER_DESCR_GROUP.field_descrs[1].index(),
+ 0x6100_0011
+ );
+ assert_eq!(
+ W_TUPLE_USER_DESCR_GROUP.field_descrs[0].index(),
+ 0x6100_0020
+ );
assert_eq!(
W_TUPLE_USER_DESCR_GROUP.field_descrs[1].index(),
0x6100_0021
);📝 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.
Suggested change
| #[test] | |
| fn native_user_mapdict_fields_replace_prepass_placeholder_indices() { | |
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[0].index(), 0x6100_0000); | |
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[1].index(), 0x6100_0001); | |
| assert_eq!( | |
| W_UNICODE_USER_DESCR_GROUP.field_descrs[0].index(), | |
| 0x6100_0010 | |
| ); | |
| assert_eq!( | |
| W_TUPLE_USER_DESCR_GROUP.field_descrs[1].index(), | |
| 0x6100_0021 | |
| ); | |
| #[test] | |
| fn native_user_mapdict_fields_replace_prepass_placeholder_indices() { | |
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[0].index(), 0x6100_0000); | |
| assert_eq!(W_INT_USER_DESCR_GROUP.field_descrs[1].index(), 0x6100_0001); | |
| assert_eq!( | |
| W_UNICODE_USER_DESCR_GROUP.field_descrs[0].index(), | |
| 0x6100_0010 | |
| ); | |
| assert_eq!( | |
| W_UNICODE_USER_DESCR_GROUP.field_descrs[1].index(), | |
| 0x6100_0011 | |
| ); | |
| assert_eq!( | |
| W_TUPLE_USER_DESCR_GROUP.field_descrs[0].index(), | |
| 0x6100_0020 | |
| ); | |
| assert_eq!( | |
| W_TUPLE_USER_DESCR_GROUP.field_descrs[1].index(), | |
| 0x6100_0021 | |
| ); |
🤖 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/descr.rs` around lines 4204 - 4215, Extend the test
native_user_mapdict_fields_replace_prepass_placeholder_indices to assert the two
omitted fields: W_UNICODE_USER_DESCR_GROUP.field_descrs[1] must equal
0x6100_0011, and W_TUPLE_USER_DESCR_GROUP.field_descrs[0] must equal
0x6100_0020, covering all six minted indices.
Comment on lines
+750
to
+757
| unsafe fn object_object_custom_trace(obj_addr: usize, f: &mut dyn FnMut(*mut majit_ir::GcRef)) { | ||
| unsafe { mapdict_storage_custom_trace(obj_addr, f) }; | ||
| } | ||
|
|
||
| unsafe fn int_object_custom_trace(obj_addr: usize, f: &mut dyn FnMut(*mut majit_ir::GcRef)) { | ||
| unsafe { mapdict_storage_custom_trace(obj_addr, f) }; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename the two mapdict wrappers to name the layout they trace.
int_object_custom_trace is registered only for W_INT_USER_GC_TYPE_ID (line 3718). Exact W_IntObject is a GC leaf registered at line 1422. The current name suggests the opposite. The sibling wrappers added in the same change use the *_user_object_custom_trace form.
♻️ Proposed rename
-unsafe fn int_object_custom_trace(obj_addr: usize, f: &mut dyn FnMut(*mut majit_ir::GcRef)) {
+unsafe fn int_user_object_custom_trace(obj_addr: usize, f: &mut dyn FnMut(*mut majit_ir::GcRef)) {
unsafe { mapdict_storage_custom_trace(obj_addr, f) };
}Update the registration at line 3718 to match.
🤖 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/src/eval.rs` around lines 750 - 757, Rename the mapdict wrapper
int_object_custom_trace to int_user_object_custom_trace to reflect the
W_INT_USER_GC_TYPE_ID layout and match the sibling *_user_object_custom_trace
naming. Update its registration accordingly, while leaving
object_object_custom_trace unchanged.
youknowone
force-pushed
the
perf-exc
branch
2 times, most recently
from
August 8, 2026 09:21
9e4408b to
fa362d5
Compare
There was a problem hiding this comment.
💡 Codex Review
pyre/majit/majit-metainterp/src/optimizeopt/virtualize.rs
Lines 1004 to 1005 in fa362d5
Materialize folded scalar defaults before escape
When an unset Int or Float field is read from a virtual that is later forced or escapes, these lines replace the load with zero but never add that zero to the virtual's field set. force_box_impl in optimizeopt/info.rs emits stores only for entries present in vinfo.fields, while MiniMark's native nursery is deliberately not zero-filled (majit-gc/src/nursery.rs:127-146), so the materialized object retains recycled scalar bytes and a later residual read can observe garbage instead of the folded zero. Either record the default in the virtual or emit missing scalar defaults during materialization.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ 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".
Comment on lines
+573
to
+577
| if w_class.is_null() | ||
| || !unsafe { pyre_object::is_type(w_class) } | ||
| || !unsafe { pyre_object::w_type_get_hasdict(w_class) } | ||
| { | ||
| return false; |
There was a problem hiding this comment.
hasdict
For a native subclass that deliberately omits __dict__—for example class S(str): __slots__ = ('x',)—this returns false even though the constructor now always allocates W_UnicodeObjectUser. The registered unicode_user_object_custom_trace unconditionally reaches instance_walk_boxed_storage, whose new mapdict_carrier has debug_assert!(has_mapdict_storage(obj)), so the next GC panics in debug/test builds for a live slots-only subclass. Detect the physical user layout from its GC type independently of whether dictionary routing is enabled.
AGENTS.md reference: AGENTS.md:L109-L111
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pyre/pyre-jit-trace/src/helpers.rs (1)
1110-1131: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe loop writes zero to the field line 1130 immediately overwrites.
items_len_descris alwayslist_int_items_len_descr()orlist_float_items_len_descr(), so the loop at lines 1113-1121 stores zero into the active typed length, and line 1130 storeslen_refinto the same field. TwoSetfieldGcops target one field. The result is correct, but the trace carries a dead store on every typed-list allocation.Skip the active descr in the loop.
♻️ Proposed change
let zero = ctx.const_int(0); + let items_len_idx = items_len_descr.index(); for scalar_descr in [ list_length_descr(), list_int_items_len_descr(), list_float_items_len_descr(), ] { let scalar_idx = scalar_descr.index(); + if scalar_idx == items_len_idx { + // Written with the real length below. + continue; + } ctx.record_op_with_descr(OpCode::SetfieldGc, &[list, zero], scalar_descr); ctx.heapcache_setfield_cached(list, scalar_idx, zero); }Then drop the duplicate
let items_len_idx = items_len_descr.index();at line 1129.🤖 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/helpers.rs` around lines 1110 - 1131, Update the scalar-field initialization loop in the list allocation flow to skip the descriptor equal to the active items_len_descr, preventing a zero write that is immediately overwritten by the subsequent len_ref assignment. Keep the later items_len_descr SetfieldGc and cache update, and remove the now-redundant local declaration used only for the duplicate index.pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs (1)
3622-3627: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the sibling fold and skip the receiver
GuardValuewhenobjis already constant.
try_walker_specialize_builtin_type_getattrat Lines 7179-7189 emits the same receiver pin only underif !obj_ref.is_constant(), and treats the guard on a constant operand as a removable tautology. This fold emits it unconditionally and then callsreplace_box(obj, w_type_const)on an operand that may already be that constant.The optimizer removes the redundant guard, so this is a consistency point rather than a defect.
🤖 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/specialize.rs` around lines 3622 - 3627, Match the conditional receiver-pinning behavior in try_walker_specialize_builtin_type_getattr: in the fold containing w_type_const, emit GuardValue, replace_box, and walker_pin_type_version_tag only when obj is not already constant. Skip the receiver guard and replacement for constant obj operands, treating that guard as a removable tautology.pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs (2)
3470-3484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that
writes_gc_liveness_root_onlyis deliberately narrower thanis_idempotent_gc_barrier.The comment places
ClearInFlightExceptionin the same category asis_idempotent_gc_barrier, but the two flags are applied at different sets of sites.is_idempotent_gc_barrieralso suppressesfbw_mark_executed_body_residual(Line 3116) andbody_effect_candidate(Line 2989).writes_gc_liveness_root_onlysuppresses only this odometer bump. AClearInFlightExceptionresidual therefore still refuses in-flight FOR_ITER delivery and still marks the walk as having run a non-pure body residual.If the narrower scope is intentional, state it in the comment. If not, apply the flag at the other two sites as well.
🤖 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/residual_call.rs` around lines 3470 - 3484, Clarify in the comment near writes_gc_liveness_root_only that its narrower scope is intentional: it only suppresses the heap-write odometer check, while ClearInFlightException must still participate in the existing is_idempotent_gc_barrier-related handling at fbw_mark_executed_body_residual and body_effect_candidate. Preserve the current behavior unless the implementation confirms those sites should also recognize this helper.
2764-2807: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the
null_or_selfsentinel check to thestrandordreplay-safe arms.
bh_call_fn_N(callable, null_or_self, args...)prependsnull_or_selfas arg0 when it is non-null.observed_exact_scalar_strandobserved_exact_str_orddo not testargs[1] == 0, so a bound-receiver call whose receiver happens to bestror the builtinordand whoseargs[2]happens to be an exact scalar is classified replay-safe. In that shapeargs[2]is not the sole operand, so the "exact immutable scalar formatting allocates only its fresh result" argument does not hold.The sibling
replay_safe_tuple_from_listarm directly below already applies this check, and so dotry_walker_specialize_builtin_lenandtry_walker_specialize_builtin_type_getattrinspecialize.rs. A residual wrongly classified asprovably_side_effect_freeskipsfbw_abort_nested_unjournaled_residualand is excluded from the in-flight FOR_ITER body-effect accounting.🐛 Proposed fix
let observed_exact_scalar_str = - helper == majit_ir::PyreHelperKind::CallFn && args.len() == 3 && { + helper == majit_ir::PyreHelperKind::CallFn && args.len() == 3 && args[1] == 0 && { let callable = args[0] as pyre_object::PyObjectRef;let observed_exact_str_ord = native_exact_str_replay && helper == majit_ir::PyreHelperKind::CallFn && args.len() == 3 + && args[1] == 0 && pyre_interpreter::builtins::is_builtin_ord_function(args[0] as pyre_object::PyObjectRef)🤖 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/residual_call.rs` around lines 2764 - 2807, Add the existing null_or_self sentinel condition to both observed_exact_scalar_str and observed_exact_str_ord, requiring args[1] == 0 before classifying either call as replay-safe. Keep the current callable/operand identity checks unchanged and align these arms with replay_safe_tuple_from_list and the builtin specialization checks.
🤖 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/objspace/std/mapdict.rs`:
- Around line 2857-2990: Remove the duplicate MapdictObject implementation for
MapdictCarrier, including its storage, map, attribute-population, growth, and
getdict methods, and rely on the existing W_ObjectObject implementation reached
through the slots() fallback. Update get_empty_storage to route its remaining
direct _set_mapdict_map call through mapdict_carrier, preserving the existing
behavior without maintaining two copies of the delicate logic.
- Around line 2830-2838: Update the tuple predicate in slots() to require both
is_tuple(obj) and !is_specialised_tuple(obj), matching has_mapdict_storage and
mapdict_storage_descr. Ensure the specialised-tuple exclusion is applied before
casting to W_TupleObjectUser and accessing map or storage, keeping all three
guards consistent.
- Around line 579-582: Update has_mapdict_storage to perform the existing
exact-native-object/class and hasdict checks before calling header_of, then use
try_gc_owns_object to verify collector ownership before reading the GC header.
Ensure malloc_typed fallback instances are rejected without dereferencing an
unowned header, while preserving mapdict detection for collector-owned
user-subclass objects.
In `@pyre/pyre-jit-trace/src/helpers.rs`:
- Around line 1499-1511: Update the comments around the SetfieldGc flags
initialization to remove the virtual-object heapcache concern and its claim that
omitting the heapcache seed prevents zero folding. Retain only the accurate
explanation about constructor-time escape state and the GC-reference slots
initialized by clear_gc_fields.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 2665-2683: Eliminate the deferred CodeObject dereference in
census_dump_foriter_inflight by storing qualname and source_path in
ForiterInflightCensusCounts when census_record_foriter_inflight is called. In
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs lines 863-870, continue
resolving the labels at record time and pass them alongside code_ptr; in
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs lines 2665-2683, print the
stored labels and remove the unsafe lookup.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 49-58: Extract the repeated raise-arm setup into a shared helper
returning Option<(PyObjectRef, OpRef)>, covering
walker_execute_may_force_boxed_outcome with required Err, backend exception-cell
draining, payload casting to PyObjectRef,
walker_recorded_builtin_raise_is_supported validation, and execution-context
resolution. Replace the four duplicated sequences at the identified raising arms
with this helper before calling walker_emit_recorded_builtin_raise; keep the
range arm’s err.to_exc_object() acquisition and reuse only the shared tail where
applicable.
- Around line 120-129: Update the exception handling in
walker_emit_recorded_builtin_raise to stop republishing exc into
BH_LAST_EXC_VALUE after the residual exception has been consumed. Preserve the
ctx.last_exc_value and ctx.last_exc_value_concrete assignments, and clear the
BH_LAST_EXC_VALUE TLS after reading or using the consumed value so the walker’s
shadow owners remain authoritative.
- Around line 7265-7281: Update the zero-step range handling around
call_function_impl_result to collect the raised PyError and drain the backend
exception cells via drain_backend_jit_exc() before inspecting it. Ensure the
drain occurs for both supported and unsupported ValueError paths, including
every early return after the plain-eval call.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 6713-6721: Extract the repeated backward-jump target match into a
shared backward_jump_target(code, pc, instr, op_arg) -> Option<usize> helper,
preserving skip_caches(code, pc + 1) for JumpBackward and plain pc + 1 for
JumpBackwardNoInterrupt. Replace all four matching implementations, including
the test sites, with calls to this helper.
- Around line 12660-12703: The test
call_bearing_later_comprehension_is_safe_for_the_earlier_loop currently
exercises only a frame that already passes the whole-frame safety check. Add a
fixture containing a disjoint unsafe loop alongside the safe escaping-range
loop, then assert for the resulting code that for_iter_bodies_all_jit_safe is
false while frame_has_traceable_escaping_range_loop is true, retaining the
region-safety assertions to verify the backedge gate.
- Around line 7763-7802: Evict entries from the three pointer-keyed safety
caches when their associated CodeObject is destroyed. Update pycode_destructor
to remove the code pointer from for_iter_bodies_jit_safe and remove all matching
(code pointer, loop_header_pc) entries from loop_region_jit_safe and
escaping_range_loop_regions, reusing the existing CallControl cleanup pattern
used for graph_jit_shapes.
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 2129-2192: Add a test covering switch_to_object_strategy using an
Integer-strategy list, append or insert a mixed non-integer value to trigger the
transition, then inspect W_ListObject and assert the strategy is Object with
both int_items.block and float_items.block null. Keep the existing construction
and clear coverage unchanged.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/helpers.rs`:
- Around line 1110-1131: Update the scalar-field initialization loop in the list
allocation flow to skip the descriptor equal to the active items_len_descr,
preventing a zero write that is immediately overwritten by the subsequent
len_ref assignment. Keep the later items_len_descr SetfieldGc and cache update,
and remove the now-redundant local declaration used only for the duplicate
index.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 3470-3484: Clarify in the comment near
writes_gc_liveness_root_only that its narrower scope is intentional: it only
suppresses the heap-write odometer check, while ClearInFlightException must
still participate in the existing is_idempotent_gc_barrier-related handling at
fbw_mark_executed_body_residual and body_effect_candidate. Preserve the current
behavior unless the implementation confirms those sites should also recognize
this helper.
- Around line 2764-2807: Add the existing null_or_self sentinel condition to
both observed_exact_scalar_str and observed_exact_str_ord, requiring args[1] ==
0 before classifying either call as replay-safe. Keep the current
callable/operand identity checks unchanged and align these arms with
replay_safe_tuple_from_list and the builtin specialization checks.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 3622-3627: Match the conditional receiver-pinning behavior in
try_walker_specialize_builtin_type_getattr: in the fold containing w_type_const,
emit GuardValue, replace_box, and walker_pin_type_version_tag only when obj is
not already constant. Skip the receiver guard and replacement for constant obj
operands, treating that guard as a removable tautology.
🪄 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: 3963020b-5e4e-4508-b3ec-a32b27d87375
📒 Files selected for processing (21)
pyre/bench/synth/foriter_inlined_callee_build_list.cranelift.jitstatspyre/bench/synth/foriter_inlined_callee_build_list.dynasm.jitstatspyre/bench/synth/foriter_inlined_callee_build_list.pypyre/bench/synth/foriter_inlined_callee_build_list.wasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.cranelift.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.dynasm.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.pypyre/bench/synth/subscr_user_getitem_stack_index.wasm.jitstatspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/listobject.rs
Comment on lines
+579
to
+582
| let type_id = unsafe { (*majit_gc::header::header_of(obj as usize)).type_id() }; | ||
| type_id == pyre_object::intobject::W_INT_USER_GC_TYPE_ID.get() | ||
| || type_id == pyre_object::unicodeobject::W_UNICODE_USER_GC_TYPE_ID.get() | ||
| || type_id == pyre_object::tupleobject::W_TUPLE_USER_GC_TYPE_ID.get() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the allocation paths for the int/unicode/tuple user layouts.
set -euo pipefail
rg -n -C 10 'W_IntObjectUser|W_UnicodeObjectUser|W_TupleObjectUser' pyre/pyre-object/src \
-g '!**/tests/**'
rg -n -C 4 'try_gc_alloc_stable_raw|malloc_typed' pyre/pyre-object/src/intobject.rs \
pyre/pyre-object/src/unicodeobject.rs pyre/pyre-object/src/tupleobject.rsRepository: youknowone/pyre
Length of output: 37609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mapdict relevant code =="
sed -n '520,605p' pyre/pyre-interpreter/src/objspace/std/mapdict.rs
echo
echo "== gc ownership helpers/usages =="
rg -n -C 5 'try_gc_owns_object|malloc_typed|alloc_with_gc_header|header_of|GcHeader::SIZE' pyre/pyre-*.rs pyre/pyre-object/src pyre/pyre-interpreter/src/objspace/std/mapdict.rs
echo
echo "== gc_hook definitions =="
rg -n -C 8 'fn try_gc_alloc_stable_raw|fn try_gc_owns_object|malloc_typed|malloc_raw|alloc_with_gc_header' pyre -g '*.rs'Repository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lltype malloc_typed_managed implementation =="
sed -n '302,368p' pyre/pyre-object/src/lltype.rs
echo
echo "== gc_hook trait/functions =="
rg -n -C 8 'pub trait|try_gc_alloc_stable_raw|try_gc_owns_object|register.*type|malloc_typed_managed|try_gc_write_barrier' pyre/gc pyre/pyre-object/src/gc_hook.rs pyre/pyre-object/src -g '*.rs' --max-count 200
echo
echo "== mapdiff allocation fallback comment/context =="
sed -n '315,328p' pyre/pyre-object/src/lltype.rs
echo
echo "== tuple exact/native path hasdict/user_layout context =="
sed -n '320,390p' pyre/pyre-object/src/tupleobject.rsRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lltype.rs relevant snippets =="
sed -n '1,90p' pyre/pyre-object/src/lltype.rs
sed -n '302,358p' pyre/pyre-object/src/lltype.rs
echo
echo "== header.rs relevant snippets =="
sed -n '1,140p' pyre/gc/src/header.rs
rg -n -C 5 'struct GcHeader|tid_and_flags|type_id|fn header_of|alloc_with_gc_header|impl GcType' pyre/gc/src/header.rs
echo
echo "== gc_hook.rs relevant snippets =="
rg -n -C 6 'try_gc_alloc_stable_raw|try_gc_owns_object|impl.*GcHook|try_alloc|try_gc_write_barrier' pyre/pyre-object/src/gc_hook.rs pyre/pyre-interpreter/src -g '*.rs' --max-count 200
echo
echo "== tuple exact vs user allocation =="
sed -n '390,460p' pyre/pyre-object/src/tupleobject.rsRepository: youknowone/pyre
Length of output: 7069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)header\.rs$|gc_hook\.rs$|alloc_with_gc_header|GcHeader' || true
echo
echo "== header definitions/usages =="
rg -n -C 8 'struct GcHeader|tid_and_flags|type_id\(\)|fn header_of|header_of\(|alloc_with_gc_header|impl GcType' -g '*.rs' .
echo
echo "== mapdict exact checks =="
rg -n -C 5 'is_int\(|is_str\(|is_tuple\(|w_type_get_hasdict|W_IntObjectUser|W_UnicodeObjectUser|W_TupleObjectUser|try_gc_owns_object' pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src -g '*.rs' | head -n 240Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== header.rs locations =="
git ls-files | rg 'majit/.*/header\.rs$|header\.rs$'
echo
echo "== header.rs relevant snippets =="
for f in $(git ls-files | rg 'majit/.*/header\.rs$|header\.rs$'); do
if rg -q 'struct GcHeader|fn header_of|fn alloc_with_gc_header|GcType' "$f"; then
echo "--- $f"
rg -n -C 8 'struct GcHeader|pub fn header_of|pub fn alloc_with_gc_header|impl GcType|try_gc_alloc_stable_raw|try_gc_owns_object|is_managed_heap_object' "$f"
fi
done
echo
echo "== allocation fallback snippets =="
for f in \
pyre/pyre-object/src/intobject.rs \
pyre/pyre-object/src/unicodeobject.rs \
pyre/pyre-object/src/tupleobject.rs \
pyre/pyre-object/src/lltype.rs
do
echo "--- $f"
rg -n -C 4 'try_gc_alloc_stable_raw|malloc_typed\(|try_gc_alloc_stable_stable|malloc_typed_managed\(' "$f"
done
echo
echo "== precise has_mapdict_storage guard feasibility =="
python3 - <<'PY'
from pathlib import Path
p = Path("pyre/pyre-interpreter/src/objspace/std/mapdict.rs")
text = p.read_text()
start = text.index("pub unsafe fn has_mapdict_storage")
end = text.index("unsafe fn type_terminator_or_create", start)
body = text[start:end]
print("exists at mapdict.rs:")
print("try_gc_owns_object:", "try_gc_owns_object" in body)
print("has_mapdict_storage body first 180 lines:")
print("\n".join(body.splitlines()[:180]))
PYRepository: youknowone/pyre
Length of output: 10766
Check GC ownership before reading the header.
header_of(obj as usize) reads obj - GcHeader::SIZE, but has_mapdict_storage can reach it from int/str/tuple user-subclass instances created on the malloc_typed fallback path. Those boxes have a header, but they are not owned by the collector; only use try_gc_owns_object after ruling out exact native objects through the existing class/hasdict checks.
🤖 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/objspace/std/mapdict.rs` around lines 579 - 582,
Update has_mapdict_storage to perform the existing exact-native-object/class and
hasdict checks before calling header_of, then use try_gc_owns_object to verify
collector ownership before reading the GC header. Ensure malloc_typed fallback
instances are rejected without dereferencing an unowned header, while preserving
mapdict detection for collector-owned user-subclass objects.
Comment on lines
+2857
to
+2990
| impl MapdictObject for MapdictCarrier { | ||
| fn _get_mapdict_map(&self) -> MapRef { | ||
| unsafe { *self.slots().0 as MapRef } | ||
| } | ||
|
|
||
| fn _set_mapdict_map(&mut self, map: MapRef) { | ||
| unsafe { *self.slots().0 = map as *const u8 }; | ||
| } | ||
|
|
||
| fn _mapdict_read_storage(&self, storageindex: usize) -> PyObjectRef { | ||
| unsafe { | ||
| let base = pyre_object::object_array::items_block_items_base(self.storage()); | ||
| *base.add(storageindex) | ||
| } | ||
| } | ||
|
|
||
| fn _mapdict_write_storage(&mut self, storageindex: usize, value: PyObjectRef) { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let self_slot = pyre_object::gc_roots::pin_roots(&[self.obj, value]); | ||
| let value_slot = self_slot + 1; | ||
| instance_write_barrier(pyre_object::gc_roots::shadow_stack_get(self_slot)); | ||
| unsafe { | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| let carrier = mapdict_carrier(owner); | ||
| let base = pyre_object::object_array::items_block_items_base(carrier.storage()); | ||
| *base.add(storageindex) = pyre_object::gc_roots::shadow_stack_get(value_slot); | ||
| self.obj = owner; | ||
| } | ||
| } | ||
|
|
||
| fn _mapdict_storage_length(&self) -> usize { | ||
| unsafe { (*self._get_mapdict_map()).storage_needed() } | ||
| } | ||
|
|
||
| fn _mapdict_pop_attribute(&mut self, map: MapRef) { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let self_slot = pyre_object::gc_roots::pin_roots(&[self.obj]); | ||
| let current_map = self._get_mapdict_map(); | ||
| let unboxed_slot: Option<(usize, usize)> = unsafe { | ||
| match &(*current_map).as_plain().unboxed { | ||
| Some(u) if !u.firstunwrapped => { | ||
| Some(((*current_map).as_plain().storageindex, u.listindex)) | ||
| } | ||
| _ => None, | ||
| } | ||
| }; | ||
| match unboxed_slot { | ||
| Some((storageindex, listindex)) => { | ||
| let slot = self._mapdict_read_storage(storageindex); | ||
| let new_list = unsafe { unboxed_items(slot)[..listindex].to_vec() }; | ||
| let unboxed = erase_unboxed(&new_list); | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| let mut carrier = unsafe { mapdict_carrier(owner) }; | ||
| carrier._mapdict_write_storage(storageindex, unboxed); | ||
| } | ||
| None => unsafe { | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| let carrier = mapdict_carrier(owner); | ||
| let storage = carrier.storage(); | ||
| let storage_needed = (*map).storage_needed(); | ||
| let cap = pyre_object::object_array::items_block_capacity(storage); | ||
| if cap > storage_needed { | ||
| let base = pyre_object::object_array::items_block_items_base(storage); | ||
| for i in storage_needed..cap { | ||
| *base.add(i) = pyre_object::PY_NULL; | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| self.obj = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| self._set_mapdict_map(map); | ||
| } | ||
|
|
||
| fn _set_mapdict_increase_storage1(&mut self, map: MapRef, value: PyObjectRef) { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let self_slot = pyre_object::gc_roots::pin_roots(&[self.obj, value]); | ||
| let value_slot = self_slot + 1; | ||
| let needed = unsafe { (*map).storage_needed() }; | ||
| unsafe { | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| let mut carrier = mapdict_carrier(owner); | ||
| let old = carrier.storage(); | ||
| let old_cap = pyre_object::object_array::items_block_capacity(old); | ||
| let block = if needed <= old_cap { | ||
| old | ||
| } else { | ||
| let grown = | ||
| pyre_object::object_array::grow_instance_items_block(old, needed, old_cap); | ||
| pyre_object::gc_roots::pin_root(grown as PyObjectRef); | ||
| pyre_object::gc_roots::shadow_stack_get( | ||
| pyre_object::gc_roots::shadow_stack_len() - 1, | ||
| ) as *mut pyre_object::object_array::ItemsBlock | ||
| }; | ||
| let base = pyre_object::object_array::items_block_items_base(block); | ||
| *base.add(needed - 1) = pyre_object::gc_roots::shadow_stack_get(value_slot); | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| carrier = mapdict_carrier(owner); | ||
| carrier.set_storage(block); | ||
| carrier._set_mapdict_map(map); | ||
| instance_write_barrier(owner); | ||
| if !std::ptr::eq(block, old) { | ||
| pyre_object::object_array::dealloc_instance_items_block(old); | ||
| } | ||
| self.obj = owner; | ||
| } | ||
| } | ||
|
|
||
| fn _set_mapdict_storage_and_map(&mut self, storage: Vec<PyObjectRef>, map: MapRef) { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let self_slot = pyre_object::gc_roots::pin_roots(&[self.obj]); | ||
| unsafe { | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| let mut carrier = mapdict_carrier(owner); | ||
| let old = carrier.storage(); | ||
| let cap = storage | ||
| .len() | ||
| .max(pyre_object::object_array::items_block_capacity(old)); | ||
| let fresh = pyre_object::object_array::alloc_instance_items_block(&storage, cap); | ||
| pyre_object::gc_roots::pin_root(fresh as PyObjectRef); | ||
| let fresh_slot = pyre_object::gc_roots::shadow_stack_len() - 1; | ||
| let owner = pyre_object::gc_roots::shadow_stack_get(self_slot); | ||
| carrier = mapdict_carrier(owner); | ||
| carrier.set_storage(pyre_object::gc_roots::shadow_stack_get(fresh_slot) as *mut _); | ||
| carrier._set_mapdict_map(map); | ||
| instance_write_barrier(owner); | ||
| pyre_object::object_array::dealloc_instance_items_block(old); | ||
| self.obj = owner; | ||
| } | ||
| } | ||
|
|
||
| fn getdict(&self) -> PyObjectRef { | ||
| _obj_getdict(self.obj) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
MapdictCarrier's MapdictObject impl duplicates the W_ObjectObject impl.
The fallback arm of slots() resolves a W_ObjectObject receiver, so MapdictCarrier already covers everything the impl at lines 2607-2788 covers. The two copies now hold the same pin, reload, write-barrier, grow, and dealloc sequences. This logic is delicate; two copies will diverge.
get_empty_storage (line 4676) is the only remaining direct W_ObjectObject _set_mapdict_map caller. Route it through mapdict_carrier and remove the duplicate impl, or state in a comment why both must stay.
🤖 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/objspace/std/mapdict.rs` around lines 2857 - 2990,
Remove the duplicate MapdictObject implementation for MapdictCarrier, including
its storage, map, attribute-population, growth, and getdict methods, and rely on
the existing W_ObjectObject implementation reached through the slots() fallback.
Update get_empty_storage to route its remaining direct _set_mapdict_map call
through mapdict_carrier, preserving the existing behavior without maintaining
two copies of the delicate logic.
Comment on lines
+2665
to
+2683
| fn census_dump_foriter_inflight() { | ||
| FORITER_INFLIGHT_CENSUS.with(|c| { | ||
| for (&(code_ptr, body_pc), counts) in c.borrow().iter() { | ||
| let (name, source) = if code_ptr == 0 { | ||
| ("<unknown>", "<unknown>") | ||
| } else { | ||
| // The code object is owned by the live frame/JitCode for the | ||
| // duration of the process; the census only reads its labels. | ||
| let code = unsafe { &*(code_ptr as *const pyre_interpreter::CodeObject) }; | ||
| (code.qualname.as_str(), code.source_path.as_str()) | ||
| }; | ||
| eprintln!( | ||
| "[fbw-foriter-census] code=0x{code_ptr:x} name={name:?} source={source:?} \ | ||
| body_pc={body_pc} DELIVERED={} REFUSED={}", | ||
| counts.delivered, counts.refused | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The FOR_ITER in-flight census stores a raw CodeObject pointer and dereferences it later. The map key holds the pointer, and every dump resolves qualname and source_path through it. CodeObject is GC-managed with a destructor (pycode_destructor, registered in pyre/pyre-jit/src/eval.rs at Line 3383), so a code object collected between the record and a later dump makes that read a use-after-free. The safety comment asserts process-lifetime ownership rather than enforcing it. The census is env-gated, so the impact is limited to a diagnostic run.
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L2665-L2683: store thequalnameandsource_pathlabels inForiterInflightCensusCountsat record time and print them from there, instead of dereferencingcode_ptrincensus_dump_foriter_inflight.pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs#L863-L870: keep resolving the identity here, but pass the resolved labels tocensus_record_foriter_inflightalongside the pointer so the dump needs no dereference.
📍 Affects 2 files
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L2665-L2683(this comment)pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs#L863-L870
🤖 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 2665 - 2683,
Eliminate the deferred CodeObject dereference in census_dump_foriter_inflight by
storing qualname and source_path in ForiterInflightCensusCounts when
census_record_foriter_inflight is called. In
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs lines 863-870, continue
resolving the labels at record time and pass them alongside code_ptr; in
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs lines 2665-2683, print the
stored labels and remove the unsafe lookup.
Comment on lines
+6713
to
+6721
| let target = match instr { | ||
| I::JumpBackward { delta } => { | ||
| Some(skip_caches(code, pc + 1).saturating_sub(delta.get(op_arg).as_usize())) | ||
| } | ||
| has_call | ||
| }; | ||
| let mut body_state = pyre_interpreter::OpArgState::default(); | ||
| let mut body_pc = pc + 1; | ||
| while body_pc < exit && body_pc < instructions.len() { | ||
| let (body_instr, body_arg) = body_state.get(instructions[body_pc]); | ||
| if let I::ForIter { delta } = body_instr { | ||
| // Validate the nested body exactly once, under its own | ||
| // lexical FOR_ITER. Scanning it again as part of the | ||
| // outer body conflates unrelated calls with its | ||
| // LIST_APPEND and declines safe PEP 709 comprehensions. | ||
| body_pc = pyre_interpreter::jump_target_forward( | ||
| instructions, | ||
| body_pc + 1, | ||
| delta.get(body_arg).as_usize(), | ||
| ); | ||
| body_state = pyre_interpreter::OpArgState::default(); | ||
| continue; | ||
| I::JumpBackwardNoInterrupt { delta } => { | ||
| Some((pc + 1).saturating_sub(delta.get(op_arg).as_usize())) | ||
| } | ||
| let permitted = for_iter_body_op_is_jit_safe(body_instr) | ||
| || matches!( | ||
| body_instr, | ||
| I::StoreSubscr | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the backward-jump-target computation into one helper.
The same match appears four times in this file, each computing JumpBackward with skip_caches(code, pc + 1) and JumpBackwardNoInterrupt with a plain pc + 1: Lines 6713-6721, Lines 12678-12687, Lines 12718-12726, and Lines 12753-12761.
The asymmetry between the two opcodes is load-bearing and easy to copy incorrectly into a fifth site. Extract a backward_jump_target(code, pc, instr, op_arg) -> Option<usize> helper and call it from all four places, including the tests.
🤖 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/src/eval.rs` around lines 6713 - 6721, Extract the repeated
backward-jump target match into a shared backward_jump_target(code, pc, instr,
op_arg) -> Option<usize> helper, preserving skip_caches(code, pc + 1) for
JumpBackward and plain pc + 1 for JumpBackwardNoInterrupt. Replace all four
matching implementations, including the test sites, with calls to this helper.
Comment on lines
+7763
to
+7802
| fn cached_loop_region_for_iter_bodies_all_jit_safe( | ||
| code: &pyre_interpreter::CodeObject, | ||
| loop_header_pc: usize, | ||
| ) -> bool { | ||
| let key = (code as *const _ as usize, loop_header_pc); | ||
| let callcontrol = crate::jit::codewriter::CodeWriter::instance().callcontrol(); | ||
| if let Some(&safe) = callcontrol.loop_region_jit_safe.get(&key) { | ||
| return safe; | ||
| } | ||
| let safe = loop_region_for_iter_bodies_all_jit_safe(code, loop_header_pc); | ||
| callcontrol.loop_region_jit_safe.insert(key, safe); | ||
| safe | ||
| } | ||
|
|
||
| fn cached_for_iter_bodies_all_jit_safe(code: &pyre_interpreter::CodeObject) -> bool { | ||
| let key = code as *const _ as usize; | ||
| let callcontrol = crate::jit::codewriter::CodeWriter::instance().callcontrol(); | ||
| if let Some(&safe) = callcontrol.for_iter_bodies_jit_safe.get(&key) { | ||
| return safe; | ||
| } | ||
| let safe = for_iter_bodies_all_jit_safe(code); | ||
| callcontrol.for_iter_bodies_jit_safe.insert(key, safe); | ||
| safe | ||
| } | ||
|
|
||
| fn cached_loop_region_contains_escaping_range_append( | ||
| code: &pyre_interpreter::CodeObject, | ||
| loop_header_pc: usize, | ||
| ) -> bool { | ||
| let key = (code as *const _ as usize, loop_header_pc); | ||
| let callcontrol = crate::jit::codewriter::CodeWriter::instance().callcontrol(); | ||
| if let Some(&contains) = callcontrol.escaping_range_loop_regions.get(&key) { | ||
| return contains; | ||
| } | ||
| let contains = loop_region_contains_escaping_range_append(code, loop_header_pc); | ||
| callcontrol | ||
| .escaping_range_loop_regions | ||
| .insert(key, contains); | ||
| contains | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Consider eviction for the pointer-keyed safety caches.
The three new maps key on code as *const _ as usize and are never pruned. CodeObject is GC-managed with a destructor (pycode_destructor, registered at Line 3383), so two facts follow for a long-running process.
First, the maps grow for the lifetime of the thread, one entry per code object and one per (code, loop_header) pair.
Second, a freed code object's address can be reused by a new one, producing a stale hit. A stale true from cached_for_iter_bodies_all_jit_safe admits a frame whose FOR_ITER bodies were never verified, which is the outcome this gate prevents.
CallControl.graph_jit_shapes already has both properties, so this is not new. It now also governs a correctness gate rather than only a shape classification. Evicting these entries from pycode_destructor would close both.
🤖 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/src/eval.rs` around lines 7763 - 7802, Evict entries from the
three pointer-keyed safety caches when their associated CodeObject is destroyed.
Update pycode_destructor to remove the code pointer from
for_iter_bodies_jit_safe and remove all matching (code pointer, loop_header_pc)
entries from loop_region_jit_safe and escaping_range_loop_regions, reusing the
existing CallControl cleanup pattern used for graph_jit_shapes.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1b2659e07d8854a9fda77560fb0715b31aca5c36/pyre-jit/src/eval.rs#L6880
When a FOR_ITER body executes LIST_EXTEND and a later trace-walk exit reaches legacy in-flight delivery, the extend residual is explicitly marked as an irreversible body effect in residual_call.rs, so fbw_foriter_inflight_take refuses delivery and discards the consumed item; the already-applied extend remains, but the rest of that iteration is skipped. Admitting this opcode globally based on a zero-refusal corpus census therefore exposes input-dependent wrong code—for example, an extend followed by a call that triggers this abort path. Keep it declined until every post-effect exit resumes forward rather than relying on the observed census.
AGENTS.md reference: AGENTS.md:L14-L18
ℹ️ 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/objspace/std/mapdict.rs (1)
4554-4566: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPin
w_objacrossbox_str_constantbefore reading storage.
w_str_from_wtf8_immortaldelegates tocrate::lltype::malloc_typed, sobox_str_constantcan touch the collector at allocation time.plain_direct_readresolvesinst.objinmapdict_carrier(obj)after the key is boxed, which means a minor collection can forward the instance out from under the pre-allocation carrier. Root the instance and reload the carrier after boxing ininstance_node_dict_items,nth_item, andgetiterreversed, matching howpopitemroots each allocating step.🤖 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/objspace/std/mapdict.rs` around lines 4554 - 4566, Update instance_node_dict_items, nth_item, and getiterreversed to root/pin w_obj before each box_str_constant allocation, then reload the mapdict carrier before plain_direct_read or equivalent storage access. Follow popitem’s per-allocation rooting pattern so collector forwarding cannot invalidate the carrier.pyre/pyre-object/src/tupleobject.rs (1)
249-267: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRoot and reload
w_classacross collecting allocations.Both constructors retain a raw class pointer across an allocation that can move heap objects. The later object-header write can store a pre-move address. This can corrupt type and MapDict operations.
pyre/pyre-object/src/tupleobject.rs#L249-L267: pinw_classwithitemsand reload its shadow-stack slot before everywrite_tuple_layoutcall and fallback allocation.pyre/pyre-object/src/unicodeobject.rs#L420-L452: pinw_classbeforegc_alloc_storage_boxand reload it before buildingW_UnicodeObjectUser.🤖 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/tupleobject.rs` around lines 249 - 267, Root w_class alongside items in tupleobject.rs lines 249-267 and unicodeobject.rs lines 420-452, then reload it from its shadow-stack slot after each collecting allocation and before write_tuple_layout, fallback allocation, or constructing W_UnicodeObjectUser; apply the corresponding changes in both constructors so later object headers use the post-move class pointer.
🤖 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 `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 2547-2581: Add direct Rust tests for
TraceCtx::synchronize_virtualizable_static covering each early-exit guard:
absent virtualizable state, out-of-bounds index, RustVec-backed array_fields,
and Value::Ref(GcRef::NO_CONCRETE). Also cover the successful path by asserting
the selected static value is written through write_field, without modifying
unrelated synchronization behavior.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Line 2049: Restore the PYFRAME_VABLE_TOKEN_FIELD_DESCR entry to the gc_edges
list in the relevant frame descriptor construction, without adding it to the
positional fields list. Preserve the existing test’s expected GC edge so
JIT-allocated frames clear references held by vable_token.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs`:
- Around line 859-876: The code identity lookup in the inflight census path
performs metadata work even when diagnostics are disabled. Update the logic
around census_record_foriter_inflight to check the same
PYRE_FORITER_INFLIGHT_CENSUS/PYRE_FBW_DEBUG_ABORT enable predicate before
calling raw_code_for_jitcode_index, while preserving fallback_code_ptr and
existing census behavior when enabled.
- Line 888: Defer census success attribution in the fbw FOR_ITER inflight flow
until deliver_inflight_foriter_item accepts the loop-header state and completes
frame.push(item). Return the attribution alongside the item from
fbw_foriter_inflight_take, record delivered=false when the caller rejects
at_loop_header, and add a regression test covering a non-header frame.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 12747-12771: Split the instruction scan into two passes: first
inspect backward jumps to compute the final minimum outer_header, then rescan
the instructions to accumulate direct_end only when a jump target equals that
finalized outer_header. Keep the existing target calculations and direct_end max
behavior, and retain the expectation that an outer backedge is found.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 4554-4566: Update instance_node_dict_items, nth_item, and
getiterreversed to root/pin w_obj before each box_str_constant allocation, then
reload the mapdict carrier before plain_direct_read or equivalent storage
access. Follow popitem’s per-allocation rooting pattern so collector forwarding
cannot invalidate the carrier.
In `@pyre/pyre-object/src/tupleobject.rs`:
- Around line 249-267: Root w_class alongside items in tupleobject.rs lines
249-267 and unicodeobject.rs lines 420-452, then reload it from its shadow-stack
slot after each collecting allocation and before write_tuple_layout, fallback
allocation, or constructing W_UnicodeObjectUser; apply the corresponding changes
in both constructors so later object headers use the post-move class pointer.
🪄 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: 57417d47-a52e-4d78-9284-9d636bc5f37b
📒 Files selected for processing (76)
majit/majit-gc/src/trace.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/ca_bridge_multiframe_resume_double_call.wasm.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/enumerate_bignum_start.cranelift.jitstatspyre/bench/synth/enumerate_bignum_start.dynasm.jitstatspyre/bench/synth/enumerate_bignum_start.wasm.jitstatspyre/bench/synth/exception_group_type.cranelift.jitstatspyre/bench/synth/exception_group_type.dynasm.jitstatspyre/bench/synth/exception_group_type.wasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstatspyre/bench/synth/exception_traceback_loop_forms.dynasm.jitstatspyre/bench/synth/foriter_inlined_callee_build_list.cranelift.jitstatspyre/bench/synth/foriter_inlined_callee_build_list.dynasm.jitstatspyre/bench/synth/foriter_inlined_callee_build_list.pypyre/bench/synth/foriter_inlined_callee_build_list.wasm.jitstatspyre/bench/synth/global_store_plain_dict_globals.wasm.jitstatspyre/bench/synth/list_append_virtual_payload.cranelift.jitstatspyre/bench/synth/list_append_virtual_payload.dynasm.jitstatspyre/bench/synth/list_append_virtual_payload.wasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstatspyre/bench/synth/minmax_key_rooting.cranelift.jitstatspyre/bench/synth/minmax_key_rooting.dynasm.jitstatspyre/bench/synth/minmax_key_rooting.wasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/pypy_type_surface.cranelift.jitstatspyre/bench/synth/pypy_type_surface.dynasm.jitstatspyre/bench/synth/pypy_type_surface.wasm.jitstatspyre/bench/synth/range_ctor_in_loop.cranelift.jitstatspyre/bench/synth/range_ctor_in_loop.dynasm.jitstatspyre/bench/synth/range_ctor_in_loop.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstatspyre/bench/synth/reversed_disabled.cranelift.jitstatspyre/bench/synth/reversed_disabled.dynasm.jitstatspyre/bench/synth/reversed_disabled.wasm.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.cranelift.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.dynasm.jitstatspyre/bench/synth/subscr_user_getitem_stack_index.pypyre/bench/synth/subscr_user_getitem_stack_index.wasm.jitstatspyre/bench/synth/wasm_ca_trampoline_decline.wasm.jitstatspyre/extra_tests/parity_tests/bigint_div_raising_specialization.pypyre/extra_tests/parity_tests/bigint_shift_raising_specialization.pypyre/extra_tests/parity_tests/builtin_raise_context_specialization.pypyre/extra_tests/parity_tests/builtin_subclass_attr_mapdict.pypyre/extra_tests/parity_tests/exception_inline_scalar_fields_jit.pypyre/extra_tests/parity_tests/float_div_raising_specialization.pypyre/extra_tests/parity_tests/for_iter_exception_handler_comprehension.pypyre/extra_tests/parity_tests/int_div_mod_raising_specialization.pypyre/extra_tests/parity_tests/numeric_binary_subclass_specialization.pypyre/extra_tests/parity_tests/range_zero_step_raising_specialization.pypyre/gate-triage.mdpyre/pyre-interpreter/src/_structseq.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/call.rspyre/pyre-object/src/boolobject.rspyre/pyre-object/src/float_array.rspyre/pyre-object/src/int_array.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/tupleobject.rspyre/pyre-object/src/unicodeobject.rs
| "PyFrame", | ||
| "pyframe::PyFrame", | ||
| &[PYFRAME_VABLE_TOKEN_FIELD_DESCR.clone()], | ||
| &[], |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Restore the PyFrame vable-token GC edge.
Line 2049 removes the only PYFRAME_VABLE_TOKEN_FIELD_DESCR entry from gc_edges. The field is not in the positional fields list. JIT-allocated frames can therefore retain an uncleared GC reference in vable_token.
The existing test at lines 4330-4339 requires this edge and will fail.
Proposed fix
- &[],
+ &[PYFRAME_VABLE_TOKEN_FIELD_DESCR.clone()],📝 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.
Suggested change
| &[], | |
| &[PYFRAME_VABLE_TOKEN_FIELD_DESCR.clone()], |
🤖 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/descr.rs` at line 2049, Restore the
PYFRAME_VABLE_TOKEN_FIELD_DESCR entry to the gc_edges list in the relevant frame
descriptor construction, without adding it to the positional fields list.
Preserve the existing test’s expected GC edge so JIT-allocated frames clear
references held by vable_token.
youknowone
force-pushed
the
perf-exc
branch
2 times, most recently
from
August 9, 2026 15:20
6edaff6 to
3fbf7a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs (1)
999-1045: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCheck Object-strategy capacity before restoring
PopEnd.
PopEndjournals a non-empty Integer-strategy pop, solength_before - 1is within the captured length; the Object-strategy risk comes after a later promotion/seeding. Switching to Object seeds only the post-pop prefix andset_object_items_from_vecinstalls that seed as the new items block capacity. Writingw_itematlength_before - 1is valid only while the seed has enough capacity, so addll_list_obj_capacity(list_ref) >= length_beforeto this arm and use the rollback-diag path otherwise.🤖 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/fbw_state.rs` around lines 999 - 1045, Guard the Object-strategy branch in FbwListEffect::PopEnd by requiring ll_list_obj_capacity(list_ref) >= length_before before restoring the length and w_item. If capacity is insufficient, use the existing rollback diagnostic and optional debug-abort path instead of writing; keep the Integer-strategy restoration unchanged.
♻️ Duplicate comments (3)
pyre/pyre-interpreter/src/objspace/std/mapdict.rs (1)
581-588: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid reading a GC header for a fallback allocation.
is_generated_user_layout_familyaccepts native subclass objects that can take themalloc_typedfallback inpyre/pyre-object/src/intobject.rsLines 233-239. Line 584 reads the GC header before it proves collector ownership.Check
try_gc_owns_object(obj as *mut u8)beforeheader_of. Returnfalsefor non-collector allocations.Proposed fix
if !unsafe { is_generated_user_layout_family(obj) } { return false; } + if !pyre_object::gc_hook::try_gc_owns_object(obj as *mut u8) { + return false; + } let type_id = unsafe { (*majit_gc::header::header_of(obj as usize)).type_id() };🤖 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/objspace/std/mapdict.rs` around lines 581 - 588, Update the generated user-layout check around is_generated_user_layout_family to call try_gc_owns_object(obj as *mut u8) before header_of; return false when the object is not collector-owned, and only then read the GC header and perform the existing type_id checks.pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs (1)
2681-2691: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not dereference stored
CodeObjectpointers.The census retains
code_ptrin TLS and dereferences it during a later dump.CodeObjectis GC-managed, so the object can be collected between recording and printing. This diagnostic path can use freed memory.Clone
qualnameandsource_pathwhen the census entry is created. Print the stored labels instead of dereferencingcode_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 2681 - 2691, Remove the unsafe CodeObject dereference from census_dump_foriter_inflight and change the TLS census entry data to retain owned clones of qualname and source_path when the entry is created. Update the recording and iteration logic to store and print those labels directly, while preserving the unknown labels for entries without a code object.pyre/pyre-jit-trace/src/descr.rs (1)
2055-2056: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRestore the
vable_tokenGC edge.Line 2056 removes
PYFRAME_VABLE_TOKEN_FIELD_DESCRfromgc_edges.vable_tokenis not in the positional field list. A JIT-allocated frame can retain an uncleared GC reference. The existing test at Line 4395 requires this edge.Proposed fix
- &[], + &[PYFRAME_VABLE_TOKEN_FIELD_DESCR.clone()],As per coding guidelines, preserve strict line-by-line structural parity.
🤖 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/descr.rs` around lines 2055 - 2056, Restore PYFRAME_VABLE_TOKEN_FIELD_DESCR in the gc_edges list for the affected frame descriptor, preserving strict line-by-line structural parity with the existing descriptor layout. Keep the positional field list unchanged so the vable_token GC edge is retained for JIT-allocated frames and satisfies the existing test.Source: Coding guidelines
🤖 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 `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 2562-2579: Update the static-write bounds check in the surrounding
trace-context method to compare index against the virtualizable data-slot
length, excluding the trailing identity slot in virtualizable_values, while
preserving the existing num_static_extra_boxes and value validation checks.
- Around line 2547-2580: Update the static-field handling in vable_setfield to
call synchronize_virtualizable_static with the written field index instead of
synchronize_virtualizable, ensuring the _opimpl_setfield_vable path only writes
that static field. Add an integration test exercising vable_setfield that
verifies array shadow slots are not written during a static-field store.
In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 525-528: Ensure the old live-frame instruction value is journaled
before the helper that synchronizes the static field into PyFrame. Update the
fbw_publish_exit_last_instr caller ordering around FBW_EXIT_LAST_INSTR_UNDO and
the helper at flush_to_frame so rollback records the pre-walk last_instr rather
than py_pc.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 6893-6901: Extract a shared `for_iter_gate_diag_enabled() -> bool`
accessor in `eval.rs` that owns the single `OnceLock` reading
`PYRE_FOR_ITER_GATE_DIAG`. Replace the function-local `FOR_ITER_GATE_DIAG` usage
here and the duplicate static near the other diagnostic site with calls to this
accessor, and update
`pyre_jit_trace::jitcode_dispatch::census_record_for_iter_gate_decline` to use
the same shared accessor so all diagnostics read one flag.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs`:
- Around line 999-1045: Guard the Object-strategy branch in
FbwListEffect::PopEnd by requiring ll_list_obj_capacity(list_ref) >=
length_before before restoring the length and w_item. If capacity is
insufficient, use the existing rollback diagnostic and optional debug-abort path
instead of writing; keep the Integer-strategy restoration unchanged.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 581-588: Update the generated user-layout check around
is_generated_user_layout_family to call try_gc_owns_object(obj as *mut u8)
before header_of; return false when the object is not collector-owned, and only
then read the GC header and perform the existing type_id checks.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 2055-2056: Restore PYFRAME_VABLE_TOKEN_FIELD_DESCR in the gc_edges
list for the affected frame descriptor, preserving strict line-by-line
structural parity with the existing descriptor layout. Keep the positional field
list unchanged so the vable_token GC edge is retained for JIT-allocated frames
and satisfies the existing test.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 2681-2691: Remove the unsafe CodeObject dereference from
census_dump_foriter_inflight and change the TLS census entry data to retain
owned clones of qualname and source_path when the entry is created. Update the
recording and iteration logic to store and print those labels directly, while
preserving the unknown labels for entries without a code object.
🪄 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: 3e77ace8-1df1-441e-b9e9-d8c39d4ad7ce
📒 Files selected for processing (15)
majit/majit-metainterp/src/trace_ctx.rspyre/pyre-interpreter/src/_structseq.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/listobject.rs
Comment on lines
+2547
to
+2580
| /// `_opimpl_setfield_vable`'s `synchronize_virtualizable()` half | ||
| /// (`pyjitpl.py:1188-1199`, `virtualizable.py write_boxes`), narrowed to | ||
| /// the one static just written because the shadow's array half carries | ||
| /// NULL operand slots mid-opcode that a full `write_all_boxes` would stamp | ||
| /// into the live frame. | ||
| pub fn synchronize_virtualizable_static(&self, index: usize) { | ||
| let Some(heap_ptr) = self.virtualizable_heap_ptr else { | ||
| return; | ||
| }; | ||
| let Some(info) = self.virtualizable_info.as_ref() else { | ||
| return; | ||
| }; | ||
| let Some(values) = self.virtualizable_values.as_ref() else { | ||
| return; | ||
| }; | ||
| if index >= info.num_static_extra_boxes || index >= values.len() { | ||
| return; | ||
| } | ||
| if info.array_fields.iter().any(|a| { | ||
| matches!( | ||
| a.storage, | ||
| crate::virtualizable::VableArrayStorage::RustVec { .. } | ||
| ) | ||
| }) { | ||
| return; | ||
| } | ||
| let value = values[index]; | ||
| if value == Value::Ref(majit_ir::GcRef::NO_CONCRETE) { | ||
| return; | ||
| } | ||
| unsafe { | ||
| info.write_field(heap_ptr as *mut u8, index, value_to_raw_bits(value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Call the single-field synchronizer from vable_setfield.
Line 4159 still calls synchronize_virtualizable(). That full write-back can write array shadow slots during a static-field store. synchronize_virtualizable_static has no caller in this file, so this change does not fix the stated _opimpl_setfield_vable path.
Replace the standard static write-back call and add an integration test through vable_setfield.
Proposed fix
- self.synchronize_virtualizable();
+ self.synchronize_virtualizable_static(index);🤖 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 `@majit/majit-metainterp/src/trace_ctx.rs` around lines 2547 - 2580, Update the
static-field handling in vable_setfield to call synchronize_virtualizable_static
with the written field index instead of synchronize_virtualizable, ensuring the
_opimpl_setfield_vable path only writes that static field. Add an integration
test exercising vable_setfield that verifies array shadow slots are not written
during a static-field store.
Comment on lines
+525
to
+528
| /// shadow flush (`flush_to_frame`) publish into the boxes shadow and | ||
| /// synchronize the static back to the live frame, so future readers | ||
| /// (JUMP-arg dedup, `close_loop_args_at`) observe the same identity as | ||
| /// `s.vable_*`. Callers gate on |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Journal the live-frame value before synchronizing it.
Line 597 now writes the static field into the live PyFrame. The supplied fbw_publish_exit_last_instr caller invokes this helper before FBW_EXIT_LAST_INSTR_UNDO reads the old last_instr. The undo record therefore stores py_pc instead of the pre-walk value. If the walk aborts, rollback restores the new instruction position and resumes at the wrong opcode.
Move the undo capture before this helper, or add an API that journals the old value before live-frame synchronization.
Proposed caller ordering fix
- crate::trace_opcode::mirror_vable_static_to_boxes(...);
if recording_frame_ptr != 0 {
let slot = ...;
FBW_EXIT_LAST_INSTR_UNDO.with(|c| {
if c.get().is_none() {
c.set(Some((recording_frame_ptr, unsafe { *slot })));
}
});
unsafe {
*slot = py_pc as isize;
}
}
+ crate::trace_opcode::mirror_vable_static_to_boxes(...);Also applies to: 593-597
🤖 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/trace_opcode.rs` around lines 525 - 528, Ensure the
old live-frame instruction value is journaled before the helper that
synchronizes the static field into PyFrame. Update the
fbw_publish_exit_last_instr caller ordering around FBW_EXIT_LAST_INSTR_UNDO and
the helper at flush_to_frame so rollback records the pre-walk last_instr rather
than py_pc.
`build_list_storage` called `IntArray::from_vec` and `FloatArray::from_vec` unconditionally, and `try_alloc_typed_items_block` clamps `cap` to 1 into the old-gen `try_gc_alloc_stable_raw`, so every list allocated two blocks whose strategy never reads them. The trace emitters leave those fields null: `emit_empty_list_inline` and `emit_object_list_inline` set only `length` / `items` / `strategy`, and `emit_typed_list_inline` writes one typed pair. Add `IntArray::empty()` / `FloatArray::empty()` and use them where emptiness is statically known — `build_list_storage`'s non-matching arms, `switch_to_object_strategy`, `w_list_clear`. `switch_to_correct_strategy` keeps `from_vec`, since its twin `emit_promote_empty_list_inline` emits a capacity-1 block and seeds the capacity getfield cache with 1. `base()` takes `wrapping_add`, so the null block yields the items offset — a non-null, 8-aligned address `from_raw_parts` accepts at length zero. `list_object_custom_trace` skips the ownership query on a null typed block. Assisted-by: Claude
virtualize.py:184-190 optimize_GETFIELD_GC_* resolves a field the virtual has never been written to through optimizer.new_const(fielddescr). Pyre carried only the written-field arm, so such a read fell through to OptEarlyForce, which forces every argument of a non-exempt operation and materialised the struct along with everything its fields reach. The array counterpart was already in place: NEW_ARRAY_CLEAR seeds every slot with the typed zero at creation (virtualize.py:27-35, info.py:507-514). typeptr keeps its own arm. heaptracker.py:66 excludes it from the virtual field set and the block above answers it from the descr vtable, so a struct whose descr carries no vtable must not fold its class pointer to null. Assisted-by: Claude
Restore exact int and bool objects to 24 bytes, Unicode objects to 64 bytes, and tuple objects to 40 bytes. Add distinct user-subclass layouts carrying mapdict map and storage fields, with their own GC types and traces. Select the wider layouts from builtin subclass constructors and resolve mapdict field descriptors from each concrete carrier layout. Keep the specialized attribute load guarded by the subclass map and storage descriptors. Record the wasm guard-count changes caused by the restored exact-object heap trajectory. Assisted-by: Claude
Assisted-by: Claude
Guard the live Python class before native mapdict field access. Size map descriptors to the target word and exclude specialised tuple layouts. Mark private user layouts as GC objects without adding duplicate subclass-range peers. Re-root every mapdict carrier on class reassignment and allocate hasdict structseq values with tuple-user storage. Extend parity coverage for exact-value exits, descriptors, slots, GC inspection, and structseq extras. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Five of them (`exception_traceback_loop_forms`, `gc_bug_bridge_flavor_traceback_names`, `loops_comprehension`, `newslice_step_hot`, `unpack_ex_hot`) return to the values already committed on the base; the rebase conflict resolution had kept this branch's older measurements over them. Their only remaining difference from the base is added counter keys. `range_ctor_in_loop` compiles and enters its loop for the first time, so loops_compiled 1 -> 5, bridges_compiled 0 -> 3 and guard_failures 0 -> 1009: a fixture that never entered compiled code reported zero guard failures trivially. An in-place revert of the FOR_ITER admission reproduced the old values. `closure_per_call` guard_failures moves 420 -> 417. This one is not attributed by a control arm. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Admit replay-safe fresh tuple and list allocation helpers during nested callee tracing. Specialize len() for empty-list storage and add a cross-backend parity fixture for the admitted shape. Assisted-by: Claude
Assisted-by: Claude
The 603 guard failures comprise three 200-hit bridge thresholds and three one-off transition failures. The final bridge reconnects the traceback walk to its compiled inner-loop token, so no resume-semantics change is required. Assisted-by: Claude
`Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude
`loops_compiled` 66 -> 67, `loops_aborted` 14 -> 15 and `guard_failures` 339 -> 356 on the wasm leg of `synth/pickle_terminal_raise_resume`. The file already carried the 356 from an earlier recording; the two loop counters did not. `retraces_compiled=0` joins the recorded set. Bisected to `jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate` by in-place whole-tree control arms at four points of this branch: the base and the trees at the three commits below it read 66 / 14 / 339, the tree at that commit reads 67 / 15 / 356, and every counter moves there together. A base control arm reproduces main's committed 66 / 14 / 339 on this host, so the move is this branch's and not the host's. The dynasm and cranelift baselines for the same fixture are byte-identical to main's (30 compiled, 1 aborted, 338 guard failures) and are unchanged here: the loop the widened gate admits is one only the guest reaches, which compiles 66 loops in this fixture where the native backends compile 30. The extra abort is one more attempt at a loop the gate now allows, recorded beside the compile it gained. Not the collection schedule: `PYPY_GC_MIN` at 256MB, 384MB and 512MB gives identical counters, and three repeats agree exactly. Assisted-by: Claude
`mirror_vable_static_to_boxes` wrote `virtualizable_boxes` without the `synchronize_virtualizable()` half `_opimpl_setfield_vable` performs (pyjitpl.py:1188-1199). `walker_capture_snapshot_for_last_guard_impl` publishes `last_instr = py_pc - 1` through it, and the walk never runs the interpreter's own `frame.last_instr = pc` store, so the live frame stayed one opcode behind the shadow and `check_synchronized_virtualizable` (pyjitpl.py:3463-3468) failed under `debug_assertions` in `gc_stress::module_dict_move_to_end_reentrant_survives_python_callbacks`. Add `TraceCtx::synchronize_virtualizable_static`, a single-static `write_boxes` that keeps `synchronize_virtualizable`'s guards and its `VableArrayStorage::RustVec` carve-out. The full `write_all_boxes` is not usable here: the shadow's array half holds NULL for the operand slots a mid-opcode guard resumes before, and writing it back would stamp those NULLs into the live frame. Call it from `mirror_vable_static_to_boxes`. `try_execute_residual_call_via_executor` saves the `last_instr` shadow entry before publishing the executing pc and restores it after the residual returns, matching `LiveLastInstrGuard`'s save/restore of the heap half. The restore is skipped when the callee forced the virtualizable. Assisted-by: Claude
`PYRE_FOR_ITER_GATE_DIAG` (pyre-jit-trace/src/jitcode_dispatch/mod.rs, pyre-jit/src/eval.rs) and `PYRE_FORITER_INFLIGHT_CENSUS` (pyre-jit-trace/src/jitcode_dispatch/mod.rs) are read through `env::var_os(..).is_some()`, so both are default-OFF diagnostics and belong in §6c. `pyre/pyrex/tests/gate_triage_complete.rs ::every_live_pyre_gate_has_a_gate_triage_entry` failed on their absence. Assisted-by: Claude
Rebasing onto `a7eb493079f` moved these counters. Measured on the final tree
with all three backends rebuilt from a full `extract-llbc.py`.
synth/pypy_type_surface (all 3) bridges_compiled 102 -> 5,
guard_failures 20497 -> 1011
synth/mapdict_frozen_unboxing_fold (all 3) guard_failures 8 -> 11
synth/ca_bridge_multiframe_resume_double_call (wasm)
guard_failures 2581 -> 2592
synth/closure_per_call (wasm) guard_failures 418 -> 426
synth/wasm_ca_trampoline_decline (wasm) guard_failures 404 -> 601
synth/recursion_memo_branch (wasm) guard_failures 4724 -> 4704
`pypy_type_surface` returns to the values #999 committed. #1086 had rewritten
the file to 102 / 20497 — the numbers the `Cls.__name__` metaclass fold
produced while it guarded the raw `w_class` slot its oracle's `gettypefor`
fallback never read — and #1086 landed before #1106 declined that fold, so
the file has named a defect since. The fixed fold gives 5 / 1011 again.
`pypy_type_surface`, `ca_bridge_multiframe_resume_double_call` and
`wasm_ca_trampoline_decline` are also red on main's own CI at `b925c3e5ead`
(run 31283765874, ubuntu leg), so those three do not originate here.
An in-place control arm reverting only this branch's vable shadow write-back
reproduces every one of these numbers, so none of them is that change.
Assisted-by: Claude
`InflightForiterBody::Jit` carries `jitcode_index: i32` since #1111, which also made the identity negative when unresolvable. The census `code_ptr` resolution still destructured the former `outer_jitcode_index: u32` and cast it, so the crate stopped compiling once both sides met. `raw_code_for_jitcode_index` indexes with the value, so a negative index misses and the census keeps the live frame's code. Assisted-by: Claude
`has_mapdict_layout` answers the physical question — the allocation carries the `MapdictStorageMixin` slots — and no longer consults `w_type_get_hasdict` for the generated int/str/tuple user layouts. `has_mapdict_storage` is that test plus the owning class's `hasdict` flag, and `mapdict_carrier` now asserts the layout predicate, so a `__slots__`-only native subclass no longer trips the assertion. `is_generated_user_layout_family` carries the specialised-tuple exclusion for the layout test, the storage test, and the carrier's `W_TupleObjectUser` arm. Assisted-by: Claude
The array-backed tuple constructor can collect, so the class pointer read before it can be stale when it is stored into the new object's `w_class`. Re-read it from the shadow-stack slot after the allocation. Assisted-by: Claude
`raw_code_for_jitcode_index` runs `ensure_finish_setup` and borrows `METAINTERP_SD`; `fbw_foriter_inflight_take` called it on every take even though `census_record_foriter_inflight` returns immediately unless `PYRE_FORITER_INFLIGHT_CENSUS` or `PYRE_FBW_DEBUG_ABORT` is set. The enable check moves into `foriter_inflight_census_enabled`, which both sites share. Assisted-by: Claude
`w_context` is written by the raise lowering, not left zeroed by GC pointer clearing. Assisted-by: Claude
`loop_region_includes_out_of_line_handler_rejoining_mid_body` compared each backward target against `outer_header` while still lowering it, so a jump seen before the smallest target was missed. The scan now runs twice over a shared target closure, each pass with its own `OpArgState`. Assisted-by: Claude
Five cases: the single-field write-back, absent virtualizable state, an out-of-range index, a RustVec-backed array field, and a shadow slot holding no concrete. Assisted-by: Claude
`virtualizable_values`'s last slot holds the vable identity (`virtualizable_boxes[-1]`), not a field value. `synchronize_virtualizable_static` bounded `index` by the full vector length, so a shadow shorter than the declared static count would have written the identity ref into a static field. Bound by the data length. Assisted-by: Claude
The per-opcode decline and the whole-region decline each owned a function-local `OnceLock` for the same variable. Assisted-by: Claude
The seven mapdict residual wrappers tested their receiver with `is_instance`, which is true only for an ordinary `W_ObjectObject`. The generated int/str/tuple user layouts failed that test, and the wrappers answer a value rather than declining: the unboxed reads returned 0 and 0.0, the boxed read returned PY_NULL, and all three writes returned without storing. `Flag.__or__` reads `other._value_`, so `Perm.R & Perm.R` computed `4 & 0`; `test.test_enum`'s `OldTestIntFlag` test_and/test_or/test_xor/ test_type failed on that. Measured on the release dynasm build, an unboxed int attribute read on an int/str/tuple subclass was wrong 1756/ 2411/2498 times per run and correct under PYRE_NO_JIT=1. The receiver test is now `has_mapdict_layout`, which is `mapdict_carrier`'s own precondition, shared through `is_mapdict_carrier`. The parity fixture gains loops that validate the loaded and stored values for the unboxed int and float slots; the existing ones discard what they load and so never observed this. Assisted-by: Claude
This reverts commit 2a0f91f. The decline it removed is load-bearing. A comprehension whose body calls a user Python function drops an element: `[random.randrange(25) for i in range(size)]` returned 22 items for size=23, and PYRE_FORITER_INFLIGHT_CENSUS reported DELIVERED=0 REFUSED=1 for that body pc on the same run. `test.test_heapq`'s test_heapsort failed on the shortened list, raising IndexError from `heappop`. The reverted commit argued the append always sits past the resume coordinate; the census shows the refusal path is reachable, because the call commits body effects that `fbw_foriter_inflight_take` sees as a committed effect since the consume. The parity fixture records the shape. Assisted-by: Claude
`mirror_vable_static_to_boxes` now calls `synchronize_virtualizable()`, the shape `_opimpl_setfield_vable` uses (`pyjitpl.py:1188-1199`), so the narrowed single-field variant and its tests have no caller. Assisted-by: Claude
The call-bearing LIST_APPEND admission raised loops_compiled and bridges_compiled on exception_group_type, list_append_virtual_payload, minmax_key_rooting, range_ctor_in_loop and subscr_user_getitem_stack_index; reverting it returns them to what main records. mapdict_frozen_unboxing_fold's guard_failures returns to 2, the value main carries — the branch's 11 was recorded while the mapdict storage helpers answered zero. dynasm only; the cranelift and wasm baselines follow. Assisted-by: Claude
Same six benches as the dynasm pass, same direction and magnitude. Assisted-by: Claude
The same six benches as the dynasm and cranelift passes, plus global_store_plain_dict_globals and pickle_terminal_raise_resume, whose observed loops_compiled / loops_aborted / guard_failures all return to the values main records. closure_per_call keeps main's guard_failures: its loops_compiled and bridges_compiled are unchanged, so the count drifts without a shape change. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1d84385852b907d723f1398c9b356d154bb6dc5b/pyre-interpreter/src/objspace/std/mapdict.rs#L584
When no collector hook is installed, an exact non-specialized tuple is allocated with plain Box::new in tupleobject.rs:372-378, so it has no preceding GcHeader. Such tuples still satisfy is_generated_user_layout_family, and ordinary attribute/dict probes call this predicate, causing header_of to read outside the allocation. This is undefined behavior in unit tests, bootstrap, or embedded interpreter use before GC initialization; reject exact builtin tuples before inspecting the header or ensure their fallback allocation also carries a header.
ℹ️ 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".
youknowone
pushed a commit
that referenced
this pull request
Aug 14, 2026
The gate that keeps a whole frame out of the tracer has had a decline census since #1103 (`census_record_for_iter_gate_decline`, `PYRE_FOR_ITER_GATE_DIAG`), but nothing that runs it across the corpus — so which of its four predicates actually costs coverage, and on which frames, was never recorded. `rework.md` already names that failure mode once ("left the finding unmeasurable behind a census that was never built"); this is the driver so it does not repeat here. Counts frames distinct by (source, qualname) rather than events, because a declined helper on a hot path emits hundreds of events and a declined module body emits one — an event ranking sorts by call frequency, not by how much surface the gate withdraws. Both are reported; the frame count is what orders the work. Splits fixture code from `lib-python/` for the same reason: every run pays the stdlib declines during import, and pooling them buries the per-fixture signal. That split is also what shows the two frame-level predicates live only on the stdlib side (`re/_parser._parse`, `enum.EnumType.__new__`, `pickle._Pickler.save_tuple`, `enum._simple_enum.<locals>.convert_class`), where no synthetic fixture reaches them. Sits beside `vable-projection-census.py`, which serves the same tripwire role for virtualizable-field projections. Assisted-by: Claude Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TQY1q3Hv4JKcQpXdmuJC4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three witnessed wrong-code defects in the compiled path, the mapdict layout work
they surfaced from, and a FOR_ITER admission gate widened after the hazard it
was suppressing was measured to be unreachable.
Wrong code, reproduced and fixed
Numeric binary specializers never guarded
w_class.try_walker_specialize_binary_op_int/_floatmatched on the operand'slayout, so a user subclass of
intorfloatthat overrides__add__/__radd__took the builtin arm and its override was skipped. The fixture(
numeric_binary_subclass_specialization.py) feeds the subclass through thesame BINARY_OP pc that went hot — specialization is per-pc, and an earlier
version of this fixture used a tail expression at a different pc, so it passed
on the unfixed binary and asserted nothing.
walker_numeric_builtin_classreturns null for tagged ints and bools, where no class guard is emitted at all.
A compiled builtin raise dropped
__context__.walker_emit_recorded_builtin_raisebuilt the exception instance withoutwriting
w_context, so an exception raised from compiled code inside an activehandler lost its chained context. Fixtures around it had been written asserting
__context__ is None, which is what the bug produced.The FOR_ITER region scan treated a loop as a contiguous pc interval.
A body whose handler rejoins outside
[start, end)ended the region early, andthe trip that crossed the rejoin vanished from the compiled loop.
loop_region_endnow grows the region to a fixpoint instead of scanning one interval.
Inline exception allocation left scalar fields uninitialized.
__suppress_context__and its neighbours were read from whatever the freshallocation happened to contain.
Layout
Exact
int/boolare 24 bytes again,str64,tuple40. User subclasses ofthose builtins get distinct wider layouts carrying the mapdict map/storage pair,
each with its own GC type id and trace. The private user-layout ids are not
rclass vtable ids — class identity stays in
PyObject.w_class— so they are keptout of the GC's subclass-range census.
optimize_GETFIELD_GC_*now answers a field the virtual was never written towith the descr's typed zero (
virtualize.py:184-190) instead of falling throughto
OptEarlyForce, which forced the struct and everything its fields reach.typeptrkeeps its own arm (heaptracker.py:66excludes it from the virtualfield set).
An unused typed list strategy no longer allocates two empty old-gen blocks.
The FOR_ITER admission gate
for_iter_body_is_jit_safe_atis an allowlist with no upstream basis. PyPyfires
can_enter_jiton every backward jump with no inspection of the body(
interp_jit.py:117), supplies noconfirm_enter_jitso the defaultreturn Trueapplies (warmstate.py:785), and handles "cannot trace this" only byaborting mid-trace — none of the six abort reasons (
rlib/jit.py:1428) isopcode-shaped.
Upstream makes the append/abort hazard structurally impossible rather than
allowlisting around it:
flatten.py:259places-live-before a branch butafter a residual call, a post-call guard takes
resumepc=-1, andLIST_APPENDis a residual
space.call_method(v, 'append', w)(pyopcode.py:1492) — so theresume coordinate always sits past a committed effect. Nothing is rolled back:
blackhole.py:1712issetposition, which continues from the coordinate alreadyreached. The comment in this file previously cited that line as authority for the
append being rolled back and replayed once. It is not, and the comment now says
what pyre actually relies on.
pyre does not have that property yet — a non-committed walk exit resumes at the
caller's CALL, and the inline-subwalk arm stages
mirror_stack: Noneand keepsthe legacy entry replay, whose consumed-item delivery
fbw_foriter_inflight_takecan refuse. So
LIST_EXTENDand call-bearingLIST_APPENDwere admitted onlyafter instrumenting that refusal:
PYRE_FORITER_INFLIGHT_CENSUS=1keys every in-flight delivery by(code object, body_pc) and records both outcomes. Over the whole
check.pycorpus plus every
bench/synth/*.py: REFUSED = 0, one DELIVERED, in apre-existing
SET_ADDframe the widening does not newly admit. The census isgated on the env var and costs nothing when off.
range_ctor_in_loopgoes frommc_entered=0to 813 — a frame that neverentered compiled code at all.
The gate can be deleted once every walk-exit leg reachable from a FOR_ITER body
resumes at or after the last committed effect. Until then it is a
symptom-suppressor for a real gap, and the comment says so.
Diagnostics kept
Two censuses, both env-gated and inert when off: FOR_ITER gate opcode declines
(by replay class), and in-flight FOR_ITER delivery outcomes.
Baselines
Nineteen
.jitstatsbaselines move. Six fixtures enter compiled code where theydid not before, so their zero counters were zero trivially —
exception_group_type,list_append_virtual_payload,minmax_key_rooting,range_ctor_in_loop, and onwasm
global_store_plain_dict_globalsandpickle_terminal_raise_resume.mapdict_frozen_unboxing_foldtakesguard_failures2 → 8 withloops_compiledunchanged. An A/B across the call-bearing
LIST_APPENDadmission alone givesmc_entered2 → 8 on the same fixture, so the counter tracks compiled-codeentries one for one; its
[C(i) for i in range(n)]is a call-bearingLIST_APPENDbody.gc_bug_bridge_flavor_traceback_names(wasm) improvesguard_failures2027 → 1670.
exception_reused_object_tb_not_doubled(wasm) losesfbw_blackhole_adopted_single_frame3 → 0. A binary built from128590c675fwith no branch commit applied reports 0 on the same fixture, so the fall is the
base's — the baseline was last recorded at
779da08a355. Every other counter onthat fixture is unchanged and its traceback-shape oracle passes.
pyre/check.pywith no--backend: dynasm 403/403, cranelift 403/403,wasm 399/399.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
int,str, andtuplesubclasses, including instance attributes, slots, class reassignment, and garbage collection.Tests