Repository navigation
stdlib: stabilize collections JIT and heapq fallback - #999
Conversation
|
Warning Review limit reached
Next review available in: 35 seconds 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 (82)
WalkthroughThe change adds Rust ChangesInterpreter accelerators and parity
JIT exception and virtualizable state
Benchmark and check validation
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant BisectModule
participant Sequence
participant KeyCallback
PythonCaller->>BisectModule: call bisect or insort
BisectModule->>KeyCallback: transform values when key is configured
BisectModule->>Sequence: read items or call insert
Sequence-->>BisectModule: item or insertion result
BisectModule-->>PythonCaller: index or operation result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit f09219f). 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: 37791e8a8e
ℹ️ 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".
| for (stack_index, &value) in ctx.vstack_boxes[..depth].iter().enumerate() { | ||
| if value != OpRef::NONE { |
There was a problem hiding this comment.
In a full-body walk whose loop header has a live unboxed Int/Float stack operand, vstack_valid can remain true while the corresponding vstack_boxes entry is OpRef::NONE (the reconciliation code intentionally preserves such holes). Skipping that entry here does not omit it from the merge state: it leaves the previous value in virtualizable_boxes, and append_virtualizable_boxes immediately consumes that stale box as the live operand. The resulting loop can therefore carry an unrelated object across its backedge and miscompute or crash; materialize the actual live bank value or conservatively decline synchronization/closure whenever a live hole remains.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
37791e8 to
1f5e24c
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1f5e24c3549ef9c7443fc74e619ccc77a8509644/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L4953-L4954
When the first merge-point crossing continues tracing and a live stack slot differs from its initially seeded value, this updates only the OpRef while leaving virtualizable_values unchanged. TraceCtx::set_virtualizable_box_at is explicitly limited to SSA renames that do not change the concrete value, so subsequent virtualizable_entry_at consumers pair the new box with the old object (often NULL); a later vable read, guard snapshot, or residual-call handoff can therefore trace the wrong path or resume with a stale stack value. Update both halves via set_virtualizable_entry_at when the Ref concrete is known, and conservatively decline otherwise.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868ea6348b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !ctx.vstack_valid { | ||
| return Ok(()); |
There was a problem hiding this comment.
When a loop body executes an opcode classified as Unmodeled (for example, LOAD_SPECIAL in a for body), vstack_valid is latched false even though a live iterator remains on the operand stack. Returning success here bypasses the new hole check and lets append_virtualizable_boxes consume the unchanged shadow slot, which can still contain the null/stale value that this synchronization was added to replace. Thus the earlier reported merge corruption remains reachable through an invalid mirror; decline the merge instead of treating this state as synchronized. This is fresh evidence beyond the prior comment because the new helper explicitly accepts vstack_valid == false.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
868ea63 to
7c5b0f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5b0f06cf
ℹ️ 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".
| // Written this way instead of `(lo + hi) / 2` so a search spanning | ||
| // `sys.maxsize` cannot overflow, matching `_bisectmodule.c`. | ||
| let mid = parsed.lo + (parsed.hi - parsed.lo) / 2; | ||
| let mut item = crate::baseobjspace::getitem(parsed.a, w_int_new(mid))?; |
There was a problem hiding this comment.
When __getitem__, key, or __lt__ executes Python code that triggers a moving collection, parsed.a, parsed.x, and parsed.key remain raw PyObjectRef values in a Rust struct rather than forwarded shadow-stack slots. The next iteration then dereferences stale addresses, which can miscompare, crash, or corrupt memory; pin these operands and reload them from their root slots after every allocating call.
Useful? React with 👍 / 👎.
| //! PyPy keeps the algorithms app-level in `lib-python/3/bisect.py`; this | ||
| //! interpreter-level surface follows those loops while providing the optional | ||
| //! accelerator imported by that module. RustPython's corresponding owner is |
There was a problem hiding this comment.
This comment acknowledges that PyPy owns these algorithms in lib-python/3/bisect.py, but the change nevertheless introduces a separate interpreter-level Rust implementation. That bypasses the required line-by-line upstream structure and creates a second semantic implementation to maintain; leave _bisect unavailable so the existing app-level fallback runs, and fix the underlying JIT/performance path rather than adding this accelerator.
AGENTS.md reference: AGENTS.md:L194-L196
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/bench/synth/getattr_attribute_fallbacks.py`:
- Line 67: Add a targeted Ruff B018 suppression to the intentional
without_hook.x access in the no-hook AttributeError test, preserving the
discarded attribute lookup and its existing exception-path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67d0fd7e-e580-4ffd-a30d-259744481789
📒 Files selected for processing (20)
pyre/bench/synth/getattr_attribute_fallbacks.pypyre/bench/synth/getattr_hook_binding.pypyre/bench/synth/type_descr_get_metaclass_getattr.pypyre/bench/synth/type_metatype_data_descr.pypyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.pypyre/extra_tests/parity_tests/collections_counter_inplace.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_bisect/mod.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/cpu.rs
| if with_descr.x == 'wd:x': | ||
| total += 1 | ||
| try: | ||
| without_hook.x |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the intentional exception-triggering access explicit.
Ruff reports B018 on Line 67 because without_hook.x discards its result. Keep the access because it tests the no-hook AttributeError path. Add a targeted suppression.
Proposed fix
- without_hook.x
+ without_hook.x # noqa: B018 - deliberate AttributeError path📝 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.
| without_hook.x | |
| without_hook.x # noqa: B018 - deliberate AttributeError path |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 67-67: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 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/bench/synth/getattr_attribute_fallbacks.py` at line 67, Add a targeted
Ruff B018 suppression to the intentional without_hook.x access in the no-hook
AttributeError test, preserving the discarded attribute lookup and its existing
exception-path behavior.
Source: Linters/SAST tools
7c5b0f0 to
9ecccfc
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9ecccfc935b0d6a86cc9d03ce6b13d15bdb3bac4/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs#L3208-L3210
When an authoritative walk executes a stateful residual that raises and then encounters any later DispatchError, execute_residual_call has already consumed BH_LAST_EXC_VALUE, while this WalkContext field disappears with the aborted trace. compile_and_run_once consequently resumes the unchanged live frame normally and replays the opcode; a callback that increments state and raises only on its first invocation can therefore run twice and have its exception swallowed. Keep an abort-visible exception carrier or explicitly deliver the walk-local exception instead of relying solely on ctx.last_exc_value.
AGENTS.md reference: AGENTS.md:L14-L19
https://github.com/youknowone/pyre/blob/9ecccfc935b0d6a86cc9d03ce6b13d15bdb3bac4/pyre-interpreter/src/module/_bisect/mod.rs#L143-L146
With a custom descriptor implementing __lt__, this passes the newly fetched item and a one-time slot-loaded x as raw pointers into descroperation::compare; its try_compare_override path invokes get(w_descr, ...) before call_function_impl_result establishes roots. If that descriptor __get__ triggers a moving collection, item is unrooted and the raw copy of x is not reloaded from its existing slot, so the subsequent comparison can dereference stale addresses. Fresh evidence after the earlier rooting change is this pre-root descriptor-binding callback inside compare; root both live comparison values and reload them after binding.
ℹ️ 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".
9ecccfc to
377e15a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 377e15a1b0
ℹ️ 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".
| let protocol = match name { | ||
| "__aenter__" | "__aexit__" => { | ||
| "asynchronous context manager protocol (no __aenter__/__aexit__ method)" | ||
| } | ||
| _ => "context manager protocol (no __enter__/__exit__ method)", |
There was a problem hiding this comment.
When a class defines only __exit__ or only __enter__ (and likewise for the async pair), this collapses distinct failures into the nonstandard “no enter/exit method” message. The bundled lib-python/3/test/test_with.py:124-178 requires (missed __enter__ method) or (missed __exit__ method) according to the actual failed LOAD_SPECIAL, and also checks the sync/async-manager suggestion, so these cases still fail despite now raising TypeError; construct the message from name and detect the opposite protocol as CPython does.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
377e15a to
a387cff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a387cffec6
ℹ️ 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".
| encode_string_field(self_obj, &mut out, key)?; | ||
| out.push_wtf8(&key_separator); | ||
| let encoded = encode_child(self_obj, pair_items[1], child_level).map_err(|err| { |
There was a problem hiding this comment.
When a custom string encoder clears the iterable returned by a dict subclass's items() and triggers a moving collection, encode_string_field runs that callback while pair_items is only an ordinary Rust Vec<PyObjectRef>. The current tuple/value can then become unreachable or be relocated, yet the next line passes the stale pair_items[1] to encode_child, causing a use-after-free or crash. The bundled lib-python/3/test/test_json/test_speedups.py::test_mutate_dict_items_during_encode exercises exactly this callback-and-collection scenario; pin the pair's key and value in shadow-stack slots and reload them after encoding the key.
Useful? React with 👍 / 👎.
| crate::module::array::startup_array_module, | ||
| ); | ||
| register_builtin_module("_csv", crate::module::_csv::init); | ||
| register_builtin_module("_json", crate::module::_json::init); |
There was a problem hiding this comment.
Registering _json activates a separate, newly written scanner/encoder instead of the existing json.scanner and json.encoder fallbacks, even though the repository requires PyPy/RPython ports to preserve strict line-by-line structure rather than reimplementing an accelerator from scratch. Leave _json unavailable so those app-level implementations remain authoritative, or port the corresponding PyPy-owned implementation structurally instead of maintaining a second semantic implementation.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
a387cff to
db44848
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db44848d9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if init_override.is_some() { | ||
| return type_call_decline("overridden __init__ requires descr_call frame"); |
There was a problem hiding this comment.
For every class that overrides __init__, this unconditional decline prevents a hot constructor loop from being traced at all. The commit's own validation shows the concrete impact: attr_store_add_transition falls back for 64 million iterations and grows from about 0.18 seconds to about 500 seconds. Represent the constructor's descr_call frame and its allocated-instance result rather than shipping this permanent interpreter fallback.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed in f74514d9fbf. The decline was not a tradeoff — it guarded a shape a constructor inline cannot reach.
In try_walker_inline_resolved_user_call, for constructor_result.is_some(): multiframe_eligible = !strict_inlinable, and the earlier guard forces strict_inlinable == true, so try_multiframe is false; strict_seed already carries constructor_result.is_none(). The block that seeds a callee frame and the one that pushes parent_frame are both gated on try_multiframe || strict_seed, so __init__ is never a frame in a resume chain — an in-__init__ guard resumes at the caller CALL coordinate and the interpreter re-runs the whole instantiation.
Verified with three witnesses, including an in-loop class flip so the guard fails inside a live compiled trace with a store already executed in __init__; PYRE_FBW_INLINE_DIAG=1 confirms the path fires (init=true). attr_store_add_transition goes from the >20s synthetic timeout to 1.1s, and dynasm/cranelift synth go 5→3 and 6→3 failures (pickle_terminal_raise_resume and context_manager also clear).
— commented by Claude
| let result = pyre_object::w_instance_new(encoder_class::type_object()); | ||
| let _roots = gc_roots::push_roots(); | ||
| let slot = gc_roots::shadow_stack_len(); | ||
| gc_roots::pin_root(result); |
There was a problem hiding this comment.
When markers, default, encoder, or another argument is young, type_object() or w_instance_new() can trigger a moving collection before the root scope is established. Those parameters remain raw PyObjectRef locals, so the subsequent attribute loop stores stale addresses into the new encoder, leading to corrupted state or a crash under GC pressure. Pin all constructor inputs before the first allocation and reload them from their root slots.
Useful? React with 👍 / 👎.
| let state = | ||
| pyre_object::w_tuple_new(vec![doc, pyre_object::w_int_new(index as i64 + 1)]); | ||
| crate::call::call_function_impl_result( | ||
| parse_object, | ||
| &[state, strict, self_obj, object_hook, pairs_hook, memo], |
There was a problem hiding this comment.
While scanning an object, a young custom object_hook, object_pairs_hook, memo, or parser callback is copied into an ordinary Rust local and then w_tuple_new allocates the parse state. If that allocation moves the callback or memo, the following parse_object call receives stale pointers even though the scanner instance itself still owns the forwarded objects. Pin these values and reload them after constructing state.
Useful? React with 👍 / 👎.
db44848 to
08159f4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08159f4a83
ℹ️ 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".
| let index = w_int_new(index); | ||
| let insert = crate::baseobjspace::getattr_str(parsed.a(), "insert")?; | ||
| crate::call::call_function_impl_result(insert, &[index, shadow_stack_get(original_x)])?; |
There was a problem hiding this comment.
When insort_* is used with a sequence whose insert attribute lookup runs Python (for example a custom __getattribute__) and that lookup triggers a moving collection, the newly allocated index object is only held in this raw local. getattr_str(parsed.a(), "insert") can relocate it before the next line passes the stale pointer as the call argument, leading to corruption or a crash; this is separate from the already rooted parsed search operands because this index is allocated after parsing and also needs a shadow-stack slot/reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 04a54587c06. index now takes a shadow-stack slot via this module’s pin helper and is read back through shadow_stack_get at the call, so the insert lookup can relocate it safely. The stale "allocate the index first" comment is replaced with the real reason.
— commented by Claude
08159f4 to
f5c1db6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5c1db6083
ℹ️ 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".
| } | ||
| let strict = crate::baseobjspace::is_true(strict_obj)?; | ||
| let start = end as usize; | ||
| let byte_start = unsafe { pyre_object::w_str_index_to_byte(doc, start) }; |
There was a problem hiding this comment.
When _json.scanstring() is called directly with a non-ASCII string and an end value greater than the string length but still within i64, this unsafe index-to-byte conversion violates w_str_index_to_byte's in-range precondition and can panic by indexing past the WTF-8 index table. CPython raises ValueError("end is out of bounds") for this case, so check end against the string's character length before calling the unsafe converter.
Useful? React with 👍 / 👎.
fc7d224 to
67552f3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67552f321e
ℹ️ 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".
| // markers dict. Keep that Python key rooted across callbacks and reuse it | ||
| // for deletion; no parallel Rust identity table is introduced. | ||
| let _roots = gc_roots::push_roots(); | ||
| let key = pyre_object::w_int_new(obj as usize as i64); |
There was a problem hiding this comment.
When a moving collection relocates a container after its outer marker is inserted, a recursive reference reaches with_marker with the forwarded address, while the dictionary still contains the integer made from the old address. The circular-reference check then misses the existing object and recursion continues until a crash or erroneous recursion failure; use the collector's stable identity mechanism (the existing gc_identity_hash hook / space.id equivalent) rather than casting the current pointer.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn make_scanner_impl(context: PyObjectRef) -> PyResult { | ||
| let scanner = pyre_object::w_instance_new(scanner_class::type_object()); |
There was a problem hiding this comment.
When make_scanner receives a young JSONDecoder, allocating the Scanner can trigger a moving collection before context has any shadow-stack slot. The subsequent attribute-copy loop then repeatedly dereferences the stale raw context pointer, which can corrupt the scanner or crash; open the root scope and pin/reload context before calling type_object() or w_instance_new().
Useful? React with 👍 / 👎.
| let args = [ | ||
| pyre_object::w_str_new(&msg), | ||
| doc, | ||
| pyre_object::w_int_new(pos as i64), | ||
| ]; |
There was a problem hiding this comment.
On malformed JSON under GC pressure, constructing the message string and position integer can relocate the young doc or the newly created message before JSONDecodeError is called. Because this ordinary Rust array is not rewritten by the collector, the constructor receives stale pointers and may crash instead of reporting the decode error; pin doc and each boxed argument and reload them for the call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9e8c1519ca4ad4b8a18e84f158be9e67e4594301/pyre-interpreter/src/module/_json/mod.rs#L178-L180
py_class! gives Scanner instances a writable __dict__, so user code can assign scanner._depth = None before calling it; this then passes that arbitrary object to unsafe w_int_get_value, whose safety contract requires a real W_IntObject, causing an invalid memory read or crash. Store the depth in a typed/read-only payload like the upstream accelerator, or validate the attribute before unboxing it.
AGENTS.md reference: AGENTS.md:L231-L232
https://github.com/youknowone/pyre/blob/9e8c1519ca4ad4b8a18e84f158be9e67e4594301/pyre-interpreter/src/module/_json/mod.rs#L198-L200
When _json.make_scanner is given a context whose memo.clear() executes Python and triggers a moving collection, the successful tuple or exception stored in result is only a raw Rust value while this callback runs. The subsequent match can therefore return a stale object or exception pointer; pin both result variants before invoking clear and reload the surviving value afterward.
https://github.com/youknowone/pyre/blob/9e8c1519ca4ad4b8a18e84f158be9e67e4594301/pyre-interpreter/src/module/_json/mod.rs#L431
Independently of the marker key's identity stability, when the container or markers dictionary is young, boxing this pointer-sized integer can trigger a moving collection before either markers or obj has a shadow-stack slot. The following contains/setitem, the encoding closure, and final deletion continue using their stale raw pointers; pin both operands before this allocation and use reloaded values throughout the marker scope.
ℹ️ 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".
9e8c151 to
32577f0
Compare
|
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 32577f02c3
ℹ️ 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".
| return Ok(pyre_object::w_tuple_new(vec![ | ||
| parsed, | ||
| pyre_object::w_int_new((index + end) as i64), |
There was a problem hiding this comment.
When parse_int, parse_float, or parse_constant returns a young object and the following w_int_new triggers a moving collection, parsed exists only as an untracked raw PyObjectRef; the tuple is then constructed with its stale pre-collection address, which can corrupt the result or crash. Pin the callback result before allocating the end index and reload it when constructing the tuple; the constant-token branch has the same defect.
Useful? React with 👍 / 👎.
| let delete = crate::baseobjspace::delitem( | ||
| gc_roots::shadow_stack_get(markers_slot), | ||
| gc_roots::shadow_stack_get(key_slot), | ||
| ); |
There was a problem hiding this comment.
When a caller retains the encoder and its supplied markers dictionary after default or recursive encoding raises, this unconditional deletion removes the failed object's marker. Both CPython 3.14's _json.make_encoder and the bundled Python/PyPy implementations leave that marker installed because deletion occurs only after encoding returns successfully; consequently, a second call with the same object must raise ValueError("Circular reference detected"), whereas this implementation reruns the failing callback. Move marker deletion to the successful branch.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 4462-4464: Summary: Ref and float virtualizable array-item readers
lack the synchronization check present in the int reader. In
vable_getarrayitem_ref_indexed and vable_getarrayitem_float_indexed, add
self.check_synchronized_virtualizable() immediately after the
is_nonstandard_virtualizable early return and before standard-vable index
lookup, matching vable_getarrayitem_int_indexed exactly.
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 9248-9262: Update the suggestion logic in the surrounding
error-handling code so the asynchronous-protocol branch matches both "__enter__"
and "__exit__", and the synchronous-protocol branch matches both "__aenter__"
and "__aexit__". Preserve the existing protocol checks and messages, ensuring
the hint appears when either the first or second context-manager method lookup
fails.
In `@pyre/pyre-interpreter/src/eval.rs`:
- Around line 7764-7785: Add a regression test alongside
test_missing_slot_store_error_uses_short_type_name that executes a with
statement on an object defining only __aenter__ and __aexit__. Capture the
raised exception, then assert it is a TypeError and matches the complete
protocol-specific message, including the cross-protocol suggestion produced by
load_special_attr.
In `@pyre/pyre-interpreter/src/module/_json/mod.rs`:
- Around line 242-254: Add crate::stack_check::stack_check()? at the start of
scanner_call_impl, before reading or validating the Python-writable _depth
attribute. Preserve the existing depth-based recursion limit check and error
behavior, using the native stack check as an independent guard against
manipulated _depth values.
- Around line 148-156: Update scanner_call_inner to avoid
value.code_points().nth(index), which performs linear character traversal and
repeated decoding; reuse the byte-index machinery from scan_scalar or
codepoint_at_index to resolve the token position efficiently, including
constant-time handling for ASCII and cached-index representations, before
dispatching to parse_string, parse_object, parse_array, or the scalar path.
In `@pyre/pyre-interpreter/src/module/_sre/interp_sre.rs`:
- Around line 1083-1086: Restore the missing introductory clause in the doc
comment for sre_index_int so it clearly describes the function’s index
conversion behavior before the existing TypeError and OverflowError details.
Ensure the repaired text balances the closing parenthesis and forms a complete
sentence.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 3028-3033: Update the strict constructor inlining path around
strict_seed and constructor_result.is_none() so every inlined __init__ retains a
distinct red callee frame. Seed that frame and discard the __init__ result only
after snapshot handling; if the frame cannot be represented, decline the inline
before recording it, preserving one red-frame identity per inlined frame rather
than collapsing to the caller or bridge-resume anchor.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 3288-3298: Move the ClearInFlightException predicate before
body_effect_candidate is computed, then add that predicate to the FOR_ITER
effect guard so PUSH_EXC_INFO does not call
fbw_mark_foriter_body_effect_since_consume() during an in-flight FOR_ITER. Keep
the existing nonpure-body residual flags unchanged.
In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 2307-2308: Replace the literal index in the valuestackdepth lookup
with the existing named static-field lookup using "valuestackdepth", preserving
the unwrap_or fallback to s.vable_valuestackdepth and matching nearby accesses.
🪄 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: de20ea83-f3f6-4dfc-be4a-e27edfe0b386
📒 Files selected for processing (31)
majit/majit-ir/src/effectinfo.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/exception_escape_hot_callee_tb_node_once.cranelift.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.dynasm.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.wasm.jitstatspyre/bench/synth/getattr_hook_binding.cranelift.jitstatspyre/bench/synth/getattr_hook_binding.dynasm.jitstatspyre/bench/synth/getattr_hook_binding.pypyre/bench/synth/getattr_hook_binding.wasm.jitstatspyre/check.pypyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.pypyre/extra_tests/parity_tests/collections_counter_inplace.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_bisect/mod.rspyre/pyre-interpreter/src/module/_ctypes/cdata.rspyre/pyre-interpreter/src/module/_json/machinery.rspyre/pyre-interpreter/src/module/_json/mod.rspyre/pyre-interpreter/src/module/_sre/interp_sre.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/cpu.rs
| let suggestion = if !w_type.is_null() | ||
| && name == "__exit__" | ||
| && unsafe { lookup_in_type(w_type, "__aenter__") }.is_some() | ||
| && unsafe { lookup_in_type(w_type, "__aexit__") }.is_some() | ||
| { | ||
| " but it supports the asynchronous context manager protocol. Did you mean to use 'async with'?" | ||
| } else if !w_type.is_null() | ||
| && name == "__aexit__" | ||
| && unsafe { lookup_in_type(w_type, "__enter__") }.is_some() | ||
| && unsafe { lookup_in_type(w_type, "__exit__") }.is_some() | ||
| { | ||
| " but it supports the context manager protocol. Did you mean to use 'with'?" | ||
| } else { | ||
| "" | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The cross-protocol suggestion never fires for the method that is looked up first.
with resolves __enter__ before __exit__, and async with resolves __aenter__ before __aexit__. When a user writes with around an object that only implements the asynchronous protocol, the failure occurs on the __enter__ lookup. This code attaches the suggestion only when name is __exit__ or __aexit__, so that case produces the bare message and loses the "Did you mean to use 'async with'?" hint. The mirrored case, async with on a synchronous context manager, fails on __aenter__ and loses the "Did you mean to use 'with'?" hint.
CPython emits the suggestion for the enter method as well. Match both names in each branch.
🐛 Proposed fix
let suggestion = if !w_type.is_null()
- && name == "__exit__"
+ && matches!(name, "__enter__" | "__exit__")
&& unsafe { lookup_in_type(w_type, "__aenter__") }.is_some()
&& unsafe { lookup_in_type(w_type, "__aexit__") }.is_some()
{
" but it supports the asynchronous context manager protocol. Did you mean to use 'async with'?"
} else if !w_type.is_null()
- && name == "__aexit__"
+ && matches!(name, "__aenter__" | "__aexit__")
&& unsafe { lookup_in_type(w_type, "__enter__") }.is_some()
&& unsafe { lookup_in_type(w_type, "__exit__") }.is_some()
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let suggestion = if !w_type.is_null() | |
| && name == "__exit__" | |
| && unsafe { lookup_in_type(w_type, "__aenter__") }.is_some() | |
| && unsafe { lookup_in_type(w_type, "__aexit__") }.is_some() | |
| { | |
| " but it supports the asynchronous context manager protocol. Did you mean to use 'async with'?" | |
| } else if !w_type.is_null() | |
| && name == "__aexit__" | |
| && unsafe { lookup_in_type(w_type, "__enter__") }.is_some() | |
| && unsafe { lookup_in_type(w_type, "__exit__") }.is_some() | |
| { | |
| " but it supports the context manager protocol. Did you mean to use 'with'?" | |
| } else { | |
| "" | |
| }; | |
| let suggestion = if !w_type.is_null() | |
| && matches!(name, "__enter__" | "__exit__") | |
| && unsafe { lookup_in_type(w_type, "__aenter__") }.is_some() | |
| && unsafe { lookup_in_type(w_type, "__aexit__") }.is_some() | |
| { | |
| " but it supports the asynchronous context manager protocol. Did you mean to use 'async with'?" | |
| } else if !w_type.is_null() | |
| && matches!(name, "__aenter__" | "__aexit__") | |
| && unsafe { lookup_in_type(w_type, "__enter__") }.is_some() | |
| && unsafe { lookup_in_type(w_type, "__exit__") }.is_some() | |
| { | |
| " but it supports the context manager protocol. Did you mean to use 'with'?" | |
| } else { | |
| "" | |
| }; |
🤖 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/baseobjspace.rs` around lines 9248 - 9262, Update
the suggestion logic in the surrounding error-handling code so the
asynchronous-protocol branch matches both "__enter__" and "__exit__", and the
synchronous-protocol branch matches both "__aenter__" and "__aexit__". Preserve
the existing protocol checks and messages, ensuring the hint appears when either
the first or second context-manager method lookup fails.
| fn scanner_call_inner(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyResult { | ||
| let value = require_string(doc)?; | ||
| if index < 0 { | ||
| return Err(stop_iteration(index)); | ||
| } | ||
| let index = index as usize; | ||
| let Some(first) = value.code_points().nth(index) else { | ||
| return Err(stop_iteration(index as i64)); | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether character-to-byte index conversion is constant time.
fd -t f -e rs . | xargs rg -nP 'fn\s+w_str_index_to_byte' -A25Repository: youknowone/pyre
Length of output: 2162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target module outline =="
ast-grep outline pyre/pyre-interpreter/src/module/_json/mod.rs --match scanner_call_inner --view expanded || true
echo "== relevant scanner section =="
sed -n '120,180p' pyre/pyre-interpreter/src/module/_json/mod.rs
echo "== scan_scalar section =="
sed -n '260,320p' pyre/pyre-interpreter/src/module/_json/mod.rs
echo "== w_str helpers and storage =="
sed -n '730,830p' pyre/pyre-object/src/unicodeobject.rs
echo "== rutf8 codepoint_position_at_index implementation =="
rg -n "codepoint_position_at_index|struct *IndexStorage|IndexStorage|byte_to_index|index_to_byte" pyre/pyre-object/src -A 120 | sed -n '1,220p'
echo "== unicode parser storage definitions =="
rg -n "class W.*Unicode|byte_to_index|index_to_byte|_byte_to_index|ruth8|utf8" pyre/pyre-parser pyre/pyre-object -g '*.py' -A 5 -B 5 | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 27852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rutf8 files =="
fd -t f 'rutf8' -e rs pyre || true
echo "== Utf8IndexStorage definitions and helper implementations =="
rg -n "struct Utf8IndexStorage|enum Utf8IndexStorage|codepoint_position_at_index|codepoint_index_at_byte_position|create_utf8_index_storage|codepoints_in_utf8|codepoint_at_index" pyre -g '*.rs' -A 80 | sed -n '1,260p'
echo "== json scanner callers in Rust =="
rg -n "scanner_call_inner|scan_scalar|w_str_index_to_byte|create_utf8_index_storage|stop_iteration" pyre/pyre-interpreter pyre/pyre-object pyre/pyre-parser 2>/dev/null -g '*.rs' -A 8 -B 8 | sed -n '1,260p'
echo "== behavioral comparison of character index to byte offset path =="
python3 - <<'PY'
def codepoints_nth(s, i):
# Mirrors str.code_points().nth(i) in Rust.
count = 0
for c in s:
if count == i:
return c, s.encode("utf-8")
count += 1
return None, s.encode("utf-8")
for s in ["abc", "ab\u00c9", "a" * 50000, "ab\u00c9" * 20000]:
enc = s.encode("utf-8")
idx = len(enc) - 1
first, byte_slice = codepoints_nth(s, idx)
print(f"{len(s)} chars, {len(enc)} bytes, first={first!r}, byte_slice_len={len(byte_slice)}")
PYRepository: youknowone/pyre
Length of output: 40447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scanner_call_inner body =="
sed -n '148,260p' pyre/pyre-interpreter/src/module/_json/mod.rs
echo "== scanner_call caller body =="
sed -n '226,270p' pyre/pyre-interpreter/src/module/_json/mod.rs
echo "== rutf8 iterator/index helper implementation =="
sed -n '1,190p' pyre/pyre-object/src/rutf8.rs
echo "== W_UnicodeObject is_ascii and get_wtf8 definitions =="
rg -n "fn w_str_is_ascii|fn w_str_is_ascii_fast|w_str_get_wtf8|ascii|latin1|W_UnicodeObject::new" pyre/pyre-object/src/unicodeobject.rs pyre/pyre-object/src/rutf8.rs pyre/pyre-object/src -g '*.rs' -A 20 -B 5 | sed -n '1,240p'Repository: youknowone/pyre
Length of output: 34160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn codepoint_position_at_index|fn codepoint_index_at_byte_position|fn codepoint_at_index" pyre/pyre-object/src/rutf8.rs -A 70Repository: youknowone/pyre
Length of output: 4323
Avoid per-token linear character indexing in scanner_call_inner.
value.code_points().nth(index) rewinds to the string start for every scanner call, and this also does extra UTF-8 decoding. Use the same byte-index machinery as scan_scalar, or a direct codepoint_at_index, before forwarding to parse_string, parse_object, parse_array, or the scalar path. The Python/scanner token API is fixed, so keeping index character-based is acceptable only if the byte slice is computed in constant time for ASCII and cached-index representations.
🤖 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/module/_json/mod.rs` around lines 148 - 156, Update
scanner_call_inner to avoid value.code_points().nth(index), which performs
linear character traversal and repeated decoding; reuse the byte-index machinery
from scan_scalar or codepoint_at_index to resolve the token position
efficiently, including constant-time handling for ASCII and cached-index
representations, before dispatching to parse_string, parse_object, parse_array,
or the scalar path.
| fn scanner_call_impl(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyResult { | ||
| // Recursive object/array callbacks invoke this same Scanner. The memo is | ||
| // context-owned and is cleared only after the outermost token finishes. | ||
| let depth = attr_i64(self_obj, "_depth")?; | ||
| // CPython's C scanner brackets recursive container decoding with | ||
| // Py_EnterRecursiveCall. The accelerator recursion can be flattened by | ||
| // the meta-tracing JIT, so the scanner-owned nesting count is the stable | ||
| // equivalent of that guard rather than the native Rust stack address. | ||
| if depth >= crate::stack_check::get_recursion_limit() as i64 { | ||
| return Err(PyError::recursion_error( | ||
| "maximum recursion depth exceeded while decoding a JSON object", | ||
| )); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_depth is Python-writable, so the recursion guard can be disabled.
attr_i64 confirms _depth is an int but does not constrain its value. The scanner instance exposes _depth as an ordinary attribute, so Python code can assign a large negative value and defeat the guard at line 250. The recursion then relies on the native Rust stack, which has no check on this path. encode_child at line 489 calls crate::stack_check::stack_check() for exactly that reason; scanner_call_impl does not.
Add the same crate::stack_check::stack_check()? call at the top of scanner_call_impl so a manipulated _depth cannot turn deep nesting into a native stack overflow.
🛡️ Proposed fix
fn scanner_call_impl(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyResult {
// Recursive object/array callbacks invoke this same Scanner. The memo is
// context-owned and is cleared only after the outermost token finishes.
+ // `_depth` is Python-writable, so keep the native stack check as the
+ // backstop the encoder side already relies on.
+ crate::stack_check::stack_check()?;
let depth = attr_i64(self_obj, "_depth")?;📝 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.
| fn scanner_call_impl(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyResult { | |
| // Recursive object/array callbacks invoke this same Scanner. The memo is | |
| // context-owned and is cleared only after the outermost token finishes. | |
| let depth = attr_i64(self_obj, "_depth")?; | |
| // CPython's C scanner brackets recursive container decoding with | |
| // Py_EnterRecursiveCall. The accelerator recursion can be flattened by | |
| // the meta-tracing JIT, so the scanner-owned nesting count is the stable | |
| // equivalent of that guard rather than the native Rust stack address. | |
| if depth >= crate::stack_check::get_recursion_limit() as i64 { | |
| return Err(PyError::recursion_error( | |
| "maximum recursion depth exceeded while decoding a JSON object", | |
| )); | |
| } | |
| fn scanner_call_impl(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyResult { | |
| // Recursive object/array callbacks invoke this same Scanner. The memo is | |
| // context-owned and is cleared only after the outermost token finishes. | |
| // `_depth` is Python-writable, so keep the native stack check as the | |
| // backstop the encoder side already relies on. | |
| crate::stack_check::stack_check()?; | |
| let depth = attr_i64(self_obj, "_depth")?; | |
| // CPython's C scanner brackets recursive container decoding with | |
| // Py_EnterRecursiveCall. The accelerator recursion can be flattened by | |
| // the meta-tracing JIT, so the scanner-owned nesting count is the stable | |
| // equivalent of that guard rather than the native Rust stack address. | |
| if depth >= crate::stack_check::get_recursion_limit() as i64 { | |
| return Err(PyError::recursion_error( | |
| "maximum recursion depth exceeded while decoding a JSON object", | |
| )); | |
| } |
🤖 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/module/_json/mod.rs` around lines 242 - 254, Add
crate::stack_check::stack_check()? at the start of scanner_call_impl, before
reading or validating the Python-writable _depth attribute. Preserve the
existing depth-based recursion limit check and error behavior, using the native
stack check as an independent guard against manipulated _depth values.
| // instance. Hold constructors to the strict straight-line path here; | ||
| // together with the `constructor_result.is_none()` term on `strict_seed`, | ||
| // this keeps `__init__` out of every resume chain. No callee frame is | ||
| // seeded, so a guard in `__init__` resumes at the caller's CALL coordinate | ||
| // and re-runs the instantiation, making the result discard unnecessary to | ||
| // represent. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep a distinct red frame for inlined constructors.
This documents a caller-boundary collapse for __init__. Because constructor_result.is_none() excludes constructors from strict_seed, guards in __init__ resume at the caller’s CALL.
A branchless constructor can still perform side effects before a nested call or guard. Re-executing instantiation after deoptimization can repeat those effects.
Seed a callee frame for strict constructors and discard the __init__ result after snapshot handling. If that frame cannot be represented, decline the inline before recording.
As per coding guidelines, preserve one red frame identity per inlined frame and do not collapse it onto a caller or bridge-resume anchor.
🤖 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/inline_call.rs` around lines 3028 -
3033, Update the strict constructor inlining path around strict_seed and
constructor_result.is_none() so every inlined __init__ retains a distinct red
callee frame. Seed that frame and discard the __init__ result only after
snapshot handling; if the frame cannot be represented, decline the inline before
recording it, preserving one red-frame identity per inlined frame rather than
collapsing to the caller or bridge-resume anchor.
Source: Coding guidelines
| ctx.virtualizable_box_at(2) | ||
| .unwrap_or(s.vable_valuestackdepth), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the vable static-field order for PyFrame and existing named
# lookup helpers, to confirm whether virtualizable_box_at(2) is currently
# correct and whether a named constant/lookup should replace it.
set -euo pipefail
echo "--- virtualizable_gen.rs (NUM_VABLE_SCALARS / field order) ---"
fd -a virtualizable_gen.rs --exec cat -n {}
echo "--- virtualizable_spec.rs (PYFRAME_VABLE_FIELDS order) ---"
fd -a virtualizable_spec.rs --exec rg -n -C5 'PYFRAME_VABLE_FIELDS|valuestackdepth' {}
echo "--- static_field_index_by_name usages (established pattern) ---"
rg -n -C2 'static_field_index_by_name'
echo "--- other literal virtualizable_box_at(N) usages ---"
rg -n 'virtualizable_box_at\([0-9]'Repository: youknowone/pyre
Length of output: 5081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- locate relevant files ---"
fd -a 'trace_opcode.rs|trace_ctx.rs|virtualizable_gen.rs|virtualizable_spec.rs|frame_layout.rs' .
echo "--- trace_opcode.rs around issue ---"
FILE="$(fd -a trace_opcode.rs . | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
sed -n '2270,2335p' "$FILE" | cat -n -v
fi
echo "--- VirtualizableInfo / static_field method definitions ---"
rg -n -C4 'struct VirtualizableInfo|static_field_index_by_name|fn static_field|fn virtualizable_box_at|impl VirtualizableInfo|and_then.*virtualizable_box_at|unwrap_or\(s\.vable_valuestackdepth\)' .
echo "--- literal virtualizable_box_at usages ---"
rg -n -C2 'virtualizable_box_at\([0-9]+' .Repository: youknowone/pyre
Length of output: 17748
Replace the magic virtualizable index with a named lookup.
ctx.virtualizable_box_at(2) currently reads the valuestackdepth slot at index 2, but all nearby valuestackdepth access uses static_field_index_by_name("valuestackdepth"). If the static vable order changes, this literal index will return the wrong field instead of failing the named lookup first.
🐛 Proposed fix
- ctx.virtualizable_box_at(2)
- .unwrap_or(s.vable_valuestackdepth),
+ ctx.virtualizable_info()
+ .and_then(|info| info.static_field_index_by_name("valuestackdepth"))
+ .and_then(|idx| ctx.virtualizable_box_at(idx))
+ .unwrap_or(s.vable_valuestackdepth),📝 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.
| ctx.virtualizable_box_at(2) | |
| .unwrap_or(s.vable_valuestackdepth), | |
| ctx.virtualizable_info() | |
| .and_then(|info| info.static_field_index_by_name("valuestackdepth")) | |
| .and_then(|idx| ctx.virtualizable_box_at(idx)) | |
| .unwrap_or(s.vable_valuestackdepth), |
🤖 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 2307 - 2308, Replace
the literal index in the valuestackdepth lookup with the existing named
static-field lookup using "valuestackdepth", preserving the unwrap_or fallback
to s.vable_valuestackdepth and matching nearby accesses.
32577f0 to
9730845
Compare
…hold `try_walker_specialize_load_type_name_attr` takes the metaclass from `baseobjspace::type_name_obj_fast_path`, which asks `typedef::type`. That falls back to `gettypefor(ob_type)` when the receiver's `w_class` slot is null (typedef.rs:234-243; the objects it covers sit in RODATA, so writing the slot would SIGBUS). The fold then guards the metaclass by reading the raw `w_class` field, so for such a receiver it emits `guard_value(NULL, type)` — a guard that fails on every execution and that nothing can discharge, because no later write fills the slot. One class reaches it. The `getset_descriptor` type object is built lazily from inside the init loop (typedef.rs:9501) as a builder for the descriptors other typedefs install, so it never enters the registry the post-loop sweep walks to stamp `w_class = type` (typedef.rs:1546-1553). `typedef::type` still answers `type` through the fallback, so `type(x)` and `x.__class__` are correct and the null slot is invisible from Python. `synth/pypy_type_surface` reads `type(type.__dict__[name]).__name__`, so it paid one guard failure per iteration on every backend: dynasm, cranelift and wasm all reported `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497` against the figures #999 `dad2a722907` recorded — 19486 excess failures over 20000 iterations, and 19486/200 = 97 excess bridges, one per `trace_eagerness` bucket. #1097 `d51ea7f32a0`, which added the fold, merged 83 seconds before #999, and the recorded figures are the ones this decline reproduces. Localized by taking the fixture apart: `check_descriptor_kinds` alone carries all 97 bridges; the receiver's own value is irrelevant (a loop-invariant object reads the same), the `FOR_ITER` is irrelevant (removing it reads the same), and `int`, `str`, `list`, `NoneType`, `object`, `type`, `method_descriptor`, `wrapper_descriptor`, `builtin_function_or_method` and a user class are all clean. `getattr(C, "__name__")` — same semantics, but the fold declines because the site's name is not in the code's name table — is clean too. check.py dynasm 406/406, cranelift 406/406, wasm 402/402, all green. `synth/pypy_type_surface` reads its recorded 11/5/0/1011 exactly and `synth/type_name_attr_fold` still reads its own 5/0/1/4, so the fold keeps applying wherever its guard holds. The null slot itself is left alone. Stamping `w_class` on the `getset_descriptor` type object would restore the invariant the sweep's comment states and let the fold apply there as well, but it moves recorded baselines and belongs in its own change. Assisted-by: Claude
…hold `try_walker_specialize_load_type_name_attr` takes the metaclass from `baseobjspace::type_name_obj_fast_path`, which asks `typedef::type`. That falls back to `gettypefor(ob_type)` when the receiver's `w_class` slot is null (typedef.rs:234-243; the objects it covers sit in RODATA, so writing the slot would SIGBUS). The fold then guards the metaclass by reading the raw `w_class` field, so for such a receiver it emits `guard_value(NULL, type)` — a guard that fails on every execution and that nothing can discharge, because no later write fills the slot. One class reaches it. The `getset_descriptor` type object is built lazily from inside the init loop (typedef.rs:9501) as a builder for the descriptors other typedefs install, so it never enters the registry the post-loop sweep walks to stamp `w_class = type` (typedef.rs:1546-1553). `typedef::type` still answers `type` through the fallback, so `type(x)` and `x.__class__` are correct and the null slot is invisible from Python. `synth/pypy_type_surface` reads `type(type.__dict__[name]).__name__`, so it paid one guard failure per iteration on every backend: dynasm, cranelift and wasm all reported `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497` against the figures #999 `dad2a722907` recorded — 19486 excess failures over 20000 iterations, and 19486/200 = 97 excess bridges, one per `trace_eagerness` bucket. #1097 `d51ea7f32a0`, which added the fold, merged 83 seconds before #999, and the recorded figures are the ones this decline reproduces. Localized by taking the fixture apart: `check_descriptor_kinds` alone carries all 97 bridges; the receiver's own value is irrelevant (a loop-invariant object reads the same), the `FOR_ITER` is irrelevant (removing it reads the same), and `int`, `str`, `list`, `NoneType`, `object`, `type`, `method_descriptor`, `wrapper_descriptor`, `builtin_function_or_method` and a user class are all clean. `getattr(C, "__name__")` — same semantics, but the fold declines because the site's name is not in the code's name table — is clean too. check.py dynasm 406/406, cranelift 406/406, wasm 402/402, all green. `synth/pypy_type_surface` reads its recorded 11/5/0/1011 exactly and `synth/type_name_attr_fold` still reads its own 5/0/1/4, so the fold keeps applying wherever its guard holds. The null slot itself is left alone. Stamping `w_class` on the `getset_descriptor` type object would restore the invariant the sweep's comment states and let the fold apply there as well, but it moves recorded baselines and belongs in its own change. Assisted-by: Claude (cherry picked from commit 95ec7f2)
The baselines committed with the bench in #999 (`dad2a722907`) report `bridges_compiled=5 guard_failures=1011`. No machine produces those numbers: #999's own CI, main's CI at `d936eb4be42`, and a local run here all report `bridges_compiled=102 guard_failures=20497` on dynasm, cranelift and wasm alike, which fails the jit-stats gate on every branch that rebases onto main. Record what the bench measures. The added `retraces_compiled=0` is the key this branch introduced; an absent key already reads as `0`, so it changes no gate. Assisted-by: Claude
…nes" This reverts commit b6f90ad. The re-record was wrong. It read "no machine reproduces 5 / 1011" as "the baseline was never measured", but an in-place control arm on another branch shows the numbers move with #1097's `Cls.__name__` fold (`try_walker_specialize_load_type_name_attr`): fold off gives 5 / 1012 against the committed 5 / 1011, fold on gives 102 / 20498. #999 measured honestly on a tree that predated the fold, which merged directly before it. So `bridges_compiled 5 -> 102` is a live regression inherited from main, not a recording that matches no build, and adopting its numbers here would erase the evidence. It is bisected to `type(type.__dict__[name]).__name__` and is specific to a `getset_descriptor` receiver; the failing guard is not the fold's own and the mechanism is still open. Assisted-by: Claude
#999 re-recorded this fixture's wasm baseline from 638 to 637. The guest reads 638 here, and 638 is also the value `check.py`'s own note on the two GC pins states the guest produces -- "the guest ... reads `recursive_call_frame_relocation` 638 and `closure_per_call` 420 against their 636 and 414" -- a comment #999 left unchanged while moving the number it describes. The branch is not the source: the same fixture passed against 638 on the previous base with this branch's `range()` change already applied, and the mechanism that moves this counter is old-gen allocation volume, which that change lowers rather than raises. Assisted-by: Claude
The fixture arrived with #999 recording bridges_compiled=5 and guard_failures=1011, and the JIT changes that landed after it moved both without a re-record. It reads bridges_compiled=102 and guard_failures=20497 here on all three backends, with loops_compiled=11 unchanged. Not this branch's: `pypy_type_surface` contains no `range()` for its only interpreter change to reach, and main's own `pyre CI` run at this base fails on exactly these three lines from ubuntu-24.04 -- `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497 (observed loops_compiled=11 bridges_compiled=102)` -- so the recorded value reproduces on neither platform. Re-recording it agrees with both. Assisted-by: Claude
…ne (#1106) * jit(wasm): drop the wasm32 arm of the self-recursive root-bridge inline `bridge_rec_root_selfrec` (inline_call.rs) was `cfg!(not(target_arch = "wasm32"))`, so a guard-failure bridge reaching a self-recursive callee's CALL took the root-bridge admission on the native backends and residualized on wasm. The cfg's stated reason — "the wasm always-portal path type-confuses the self-recursive inline (`setintbound: got Ref`)" — is phrasing carried over from #643 `920367da965`, where it belonged to `fbw_inline_callee_hazardous`: a decline that is not cfg-gated and rests on the CALL_ASSEMBLER trampoline frame. #749 `71726847a10` introduced the cfg and reported dynasm + cranelift 294/294 for that slice with no wasm number. `always-portal` names nothing in the tree. Neither mechanism that would make such a confusion wasm-specific holds: `W_IntObject.intval` is i64 on every target, so there is no word-size Int→Ref promotion, and general wasm CALL_ASSEMBLER support landed in #564 `c89254a6211` on 2026-07-15, before the claim. Add `synth/selfrec_bridge_nontail_promote`, covering the admission in the form `bridge_recursion_overflow` does not: non-tail recursion, so the guard's resume stream is multi-frame; a Ref local live across the recursive CALL; and an accumulator crossing 2**63 at level 13 of 24, so a long — a Ref — reaches the int operations. wasm reads loops/bridges/aborted/gf 2/4/1/606 with the decline and 2/6/0/806 without it, byte-identical to dynasm and cranelift. wasm jit-stats re-recorded for the five fixtures the admission moves. All five now carry the native values except `recursion_memo_branch`, one guard failure apart, previously twenty-one. check.py dynasm 405/406, cranelift 405/406, wasm 401/402. The one failure, `synth/pypy_type_surface`, reports the same `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497` with this change's two files restored to origin/main, and it fails identically on all three backends while the change is a no-op wherever the cfg already read true. The fixture also holds no self-recursive function for `code_is_self_recursive` to answer. wasm user+sys CPU, min/median over 31 interleaved samples, decline removed vs kept: `wasm_ca_trampoline_decline` -20.1%/-20.7%, `selfrec_bridge_nontail_promote` -12.9%/-11.9%, `recursion_memo_branch` -1.2%/-1.6%. Two get slower: `ca_bridge_multiframe_resume_double_call` +1.2%/+2.7% and `foriter_call_resume_drops_iteration` +2.4%/+4.0%. Both now report what the dynasm baseline records — 16 bridges / 0 aborts and 27 bridges / 1 abort — where the decline left them at 16/1 and 26/2, so the cost is wasm taking the native decision and its compile toll. Assisted-by: Claude * jit: decline the `Cls.__name__` fold when its metaclass guard cannot hold `try_walker_specialize_load_type_name_attr` takes the metaclass from `baseobjspace::type_name_obj_fast_path`, which asks `typedef::type`. That falls back to `gettypefor(ob_type)` when the receiver's `w_class` slot is null (typedef.rs:234-243; the objects it covers sit in RODATA, so writing the slot would SIGBUS). The fold then guards the metaclass by reading the raw `w_class` field, so for such a receiver it emits `guard_value(NULL, type)` — a guard that fails on every execution and that nothing can discharge, because no later write fills the slot. One class reaches it. The `getset_descriptor` type object is built lazily from inside the init loop (typedef.rs:9501) as a builder for the descriptors other typedefs install, so it never enters the registry the post-loop sweep walks to stamp `w_class = type` (typedef.rs:1546-1553). `typedef::type` still answers `type` through the fallback, so `type(x)` and `x.__class__` are correct and the null slot is invisible from Python. `synth/pypy_type_surface` reads `type(type.__dict__[name]).__name__`, so it paid one guard failure per iteration on every backend: dynasm, cranelift and wasm all reported `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497` against the figures #999 `dad2a722907` recorded — 19486 excess failures over 20000 iterations, and 19486/200 = 97 excess bridges, one per `trace_eagerness` bucket. #1097 `d51ea7f32a0`, which added the fold, merged 83 seconds before #999, and the recorded figures are the ones this decline reproduces. Localized by taking the fixture apart: `check_descriptor_kinds` alone carries all 97 bridges; the receiver's own value is irrelevant (a loop-invariant object reads the same), the `FOR_ITER` is irrelevant (removing it reads the same), and `int`, `str`, `list`, `NoneType`, `object`, `type`, `method_descriptor`, `wrapper_descriptor`, `builtin_function_or_method` and a user class are all clean. `getattr(C, "__name__")` — same semantics, but the fold declines because the site's name is not in the code's name table — is clean too. check.py dynasm 406/406, cranelift 406/406, wasm 402/402, all green. `synth/pypy_type_surface` reads its recorded 11/5/0/1011 exactly and `synth/type_name_attr_fold` still reads its own 5/0/1/4, so the fold keeps applying wherever its guard holds. The null slot itself is left alone. Stamping `w_class` on the `getset_descriptor` type object would restore the invariant the sweep's comment states and let the fold apply there as well, but it moves recorded baselines and belongs in its own change. Assisted-by: Claude * parity_tests: gate os_utime_pathconf_truncate's POSIX-only halves on Windows Two parts of the script do not run there: - the `times=` keyword check passed a time before the epoch, which the block above it already excludes Windows from, and the pyre backends answer it with `ValueError: utime: timestamp out of range`; - `pathconf` and the `pathconf_names` table it resolves through are registered under `#[cfg(unix)]`, and CPython's `os` carries neither on Windows, so both runtimes raise `AttributeError`. `pyre/check.py (windows-latest)` reports `os_utime_pathconf_truncate.py cpython=FAIL dynasm=FAIL cranelift=FAIL` at the merge-base d936eb4. The keyword spelling is now checked with a time the platform holds, and the pathconf section runs where the names table exists. Assisted-by: Claude * jit: read an aborted inline's Ref operand list at its argcode offset `reconstructed_all_ref_call_stack` read the residual op's Ref var-list at operand offset 1. That offset holds for the Ref-only residual shape (`iRd>r`) but not for the mixed one (`iIRd>r`), where the leading Int list is variable-width and `dispatch_residual_call_iIRd_kind` reads the Ref list at `1 + i_width`. On the mixed shape the read landed on the Int list's length byte and resolved its register indices through the Ref bank. The resulting list has an unrelated length, so the operand stack the abort-flush composes from it (kept vstack prefix + this list) still matched `depth_at_py_pc` at the CALL and was committed. For `p[0]` inside a `for p in ...` body the committed stack was `[iterator, p, iterator]`, and the interpreter re-executed the subscript with the loop's iterator as the index: `re.compile("|".join("%d" % x for x in range(2000)))` raised `TypeError: list indices must be integers or slices, not list_iterator` from `_compiler.py:504 _get_charset_prefix`, failing `test.test_re` in the vendored CPython suite. Derive the offset from the op's argcodes instead, walking the widths of `blackhole.py:112-157`; an op with no Ref list declines. Covered by `jitcode_dispatch::tests::ref_var_list_offset_follows_the_argcodes_not_a_fixed_byte` and `parity_tests/foriter_body_call_abort_operand_stack.py`. Assisted-by: Claude (cherry picked from commit 624d547)
…ngs to measured ratios (#1086) * bench: widen two blanket ceilings the macos runner crosses `object_getattribute_no_hook` reads 6.8x on the macos runner and up to 7.8x here against a ceiling of 5; `operator_error_typename` reads 11.0x there against 10. Both ceilings are blanket values from the sweep that first gave every synthetic fixture a `max-pypy-ratio`, never fitted to a measurement. Each is now twice the slowest ratio observed, and pypy's denominator is clamped for both, so neither ratio is a measurement to begin with. Assisted-by: Claude * bench: refit nine ceilings to the ratios the runners actually report The linux runner fails `exception_const_operand_resume` at 23.9x against a ceiling of 20, `getframe_force_cancel_journal` at 30.8x against 29, and `unpack_drain_star_raise` at 52.7x against 49. The other six sit within 2% of the bound the gate really applies (the ceiling plus `compare_buffer / exec_baseline`) in runs already recorded, so they fail on the next scheduling accident rather than on a change: `class_attrs_methods` 15.7x/15, `exception_dict_slot_reject` 5.8x/5, `defaults_reassigned_midloop` 17.8x/17, `store_slice_hot` 15.8x/15, `exception_reduce` 32.5x/32, `comprehension_object_append_hot` 51.0x/41. Each ceiling is now twice the slowest ratio observed across the three runners and this machine, with that observation stated in the header. Assisted-by: Claude * check.py: raise the pypy floor divisor to clear a fixture's own host span The divisor has to clear that span twice over: a ceiling sits at twice the slowest runner's ratio, and the floor derived from it still has to land under the fastest runner's. `defaults_reassigned_midloop` reads 1.0x and 17.8x across the runners, `foriter_inplace_immutable` 1.0x and 16.9x, `class_attrs_methods` 1.0x and 15.7x, so at 25 each of their honest ceilings put the derived floor at exactly the ratio their fastest runner reports. At 40 no observation in the runner logs sits under its floor, and the count within 25% of one drops from 61 to 27. Raising the divisor only lowers floors, so nothing that passed can newly fail. Assisted-by: Claude * bench: refit the four macos ceilings and drop the ratio gate from a loopless fixture The macos runner failed five synthetic ratio gates. Four are refitted to twice the slowest ratio the runners report, with the observation stated in the header: exception_catching_frame_tb_node 18 -> 53 (26.2x), global_cell_shortpreamble_hot 19 -> 46 (22.7x), inline_subwalk_mutating_residual 40 -> 102 (50.9x), float_builtin_hot 8 -> 27 (13.5x, read against a floor-clamped pypy baseline, so a lower bound). getattr_attribute_fallbacks loses its `max-pypy-ratio` line instead. The fixture has no loop and records loops_compiled=0 on dynasm, cranelift and wasm, so its ratio compares interpreter startup rather than compiled code; it reads between 1.0x and 12.2x across the runners without the fixture changing. 42 synth fixtures already carry no ratio header, and its jitstats baselines still gate it. Assisted-by: Claude * bench: refit fifteen ceilings that sit inside the run-to-run spread Ranking every synth fixture by the worst ratio the runners report against the gate's effective bound (limit + compare_buffer/exec_baseline, which on posix with a clamped baseline is limit + 1) leaves fifteen whose worst reading is under 1.0 of that bound -- close enough that one noisy run reds them. Each is refitted to twice its worst posix reading, with the observation in the header. The bound is computed per platform rather than from the printed ratio: windows adds 2*(1/64)*(1 + limit) to the compare buffer, so a short bench there is not meaningfully ratio-gated and its readings are not a basis for a ceiling. Assisted-by: Claude * bench: drop the pypy ratio gate from the 69 fixtures that compile no loop Each of these records `loops_compiled=0` in its jitstats, so the ratio the gate reads is the difference between two interpreters' process startup, not the cost of anything generated. None of the 69 has a pypy baseline above 0.05s on any posix runner -- 68 of them median at or below it, startup included -- so after startup subtraction the denominator is the execution floor and the quotient is whatever that run's spawn cost happened to be. Measured on one fixture of this shape, the reading moved between 1.0x and 12.2x across runners with nothing about the fixture changing. Their jitstats baselines still gate them. Assisted-by: Claude * bench: scale three fixtures whose loop was too short to compile, rather than ungate them `loops_compiled=0` has two causes and the previous commit treated them as one. A fixture with no loop cannot be ratio-gated at any size. A fixture whose loop is simply shorter than the JIT threshold is a bench that needs a bigger trip count, and dropping its gate hides that. Scaled to the smallest size at which the loop compiles and pyre's own execution is an order of magnitude above a scheduler tick: condexpr_heap_const_merge N 1000 -> 64000 loops_compiled 0 -> 2 surrogate_class_kwargs REPEAT 100 -> 3200 loops_compiled 0 -> 3 surrogate_dir REPEAT 100 -> 1600 loops_compiled 0 -> 1 Their ratio gates are restored at four times the slowest ratio check.py observes, and the jitstats baselines are re-recorded on all three backends. The other four fixtures with a scalable trip count still record `loops_compiled=0` at 64 times their size, so their gates stay dropped. Assisted-by: Claude * bench: record the six jitstats counters this branch added `--snapshot` writes fbw_blackhole_adopted_multi_frame, fbw_blackhole_adopted_single_frame, fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, field_pos_attached_misplaced and field_pos_spec_misplaced into every fixture baseline. All read zero, and no counter that already had a value changes. Assisted-by: Claude * bench: scale the 26 fixtures whose pypy execution responds to the trip count Measured pypy execution net of an empty-script baseline at both sizes: these 26 cross 50ms, the value `_performance_gate_passed` requires of a baseline before it arms the derived floor. Fixtures whose pypy denominator clears that threshold go from 4 to 30 of 373. A wider calibration covering 135 fixtures was measured and discarded. For the other 109, pypy's execution does not respond to the trip count at all -- at 64x the workload `import_name` reads 4.4ms then 2.2ms and `defaults_reassigned_midloop` 3.2ms then 2.8ms, several below the empty-script baseline -- so raising their counts multiplied the numerator against a pinned denominator, turned 36 fixtures red without making any ratio a measurement, and cost 160s of wall clock across the three backends. Four ceilings are refitted against the denominators that are now real. Three read higher: a baseline pinned to the floor over-estimates the work pypy did, so every ratio measured against it was a lower bound. import_math drops from 291 to 8 -- that bound was never fitted to an observation, and any ceiling at or above PERF_GATE_FLOOR_DIVISOR pins the derived floor to parity, which a fixture running within 1.3x of pypy cannot hold on a host where pyre lands faster. Assisted-by: Claude * bench: declare the cpython opt-out on the three fixtures that were timing out Sizing these three so pypy's execution is a measurement also put them past `SYNTHETIC_CPYTHON_REFERENCE_TIMEOUT_S`. The cpython reference was being dropped by timeout rather than by declaration: the cpython/pypy output cross-check disappeared with nothing in the fixture saying so, and every run spent the full 5s to arrive at that drop. The two requirements do not both fit. A count small enough for cpython to finish leaves pypy an order of magnitude under the startup-subtraction floor, which is the condition `# pyre-check: skip-cpython` exists to declare. Assisted-by: Claude * bench: widen the nested_break_not_hot ceiling past its own clamp swing pypy's execution for this fixture sits on the startup-subtraction floor and crosses it between runs. With pyre's time identical at 0.11s on one host, back-to-back runs read 6.4x/8.4x and 11.9x/22.2x across the native backends, the whole swing coming from which side of the floor pypy landed on. The 17 it carried sits inside that swing. Raising the trip count is not available here: measured at 64x the workload, this fixture's pypy time does not respond to it. Assisted-by: Claude * bench: re-record the exceptions jitstats after the rebase `guard_failures` drops 202 -> 201 on all three backends under the JIT work this branch was rebased onto. The baseline it replaces was recorded before that base. Assisted-by: Claude * builtins: normalize a range bound without the BigInt round-trip `range_index_bound` converted every bound to a `BigInt` and back. The round-trip normalizes -- `space_index` returns an int-or-long argument unchanged and pyre's int/long are leaf types, so a `bool` would otherwise survive as a `bool` and `range(True).stop` would expose `True` -- but only the `bool` case needs a new object. A plain int is already the wrapping the round-trip produced, and a long that fits a machine word is rewrapped through `range_obj_as_i64`, which is the narrowing `iter()` reads when it picks `rangeiterator` over `longrange_iterator`. The `BigInt` path stays for a long that does not fit. The three-argument step test spelled `range_obj_to_bigint(w_step) == BigInt::from(0)`, allocating two `BigInt`s per call; it now reads the machine int and compares to 0, keeping the `BigInt` comparison for a step too wide for a word. `functional.py W_Range.descr_new` spells this `space.int_w`. Behavior is unchanged: bool, plain int, fitting long, wide long, an `__index__` object, a negative step, a wide step, and a bad type all produce the same values, types, and exceptions as before. Measured by interleaving the two binaries under load, min of 9 runs each at 2M iterations: `len(range(4))` -16.8ns/iter, `len(range(n))` -31.5ns, `len(range(0, 10, 2))` -19.0ns, `len(range(True))` -43.9ns, `len(range(Index(4)))` -91.8ns, against +1.5ns for the empty-loop control. This is below what `range_ctor_in_loop` can show -- that fixture costs ~14650ns/iter, so the change is diluted about a hundredfold there. Assisted-by: Claude * bench: re-record the wasm guard_failures the range bound change moves `closure_per_call` reads guard_failures 418 where the baseline holds 420. The fixture calls `range()`, and dropping the BigInt round-trip from the bound normalization removes allocations the guest counts: `PYRE_GC_INTERP` is on under wasm, so `w_int_new` is collector-allocated and enters `threshold_reached`, where natively it takes an untracked `alloc_with_gc_header`. Crossing that threshold arms the eval-breaker word that every compiled back edge polls through a real guard, so fewer crossings are fewer counted failures. `check.py`'s note on the two GC pins already names this sensitivity -- interpreter-path allocation volume moves the wasm numbers while the native ones sit still -- and the native baselines are indeed unchanged. Attributed by a control arm rather than by the argument above: with `builtins.rs` reverted to its pre-change version and everything else on this branch held fixed, the fixture passes against the recorded 420. Assisted-by: Claude * bench: restore the wasm guard_failures reading check.py documents #999 re-recorded this fixture's wasm baseline from 638 to 637. The guest reads 638 here, and 638 is also the value `check.py`'s own note on the two GC pins states the guest produces -- "the guest ... reads `recursive_call_frame_relocation` 638 and `closure_per_call` 420 against their 636 and 414" -- a comment #999 left unchanged while moving the number it describes. The branch is not the source: the same fixture passed against 638 on the previous base with this branch's `range()` change already applied, and the mechanism that moves this counter is old-gen allocation volume, which that change lowers rather than raises. Assisted-by: Claude * bench: scale twenty fixtures until pypy's execution clears the gate floor A clamped pypy baseline no longer gates anything. #1095 made `_baseline_exec_time_clamped` return before both the ceiling and the derived floor, so a fixture whose pypy side sits on EXEC_TIME_FLOOR_S is ungated on the ratio entirely -- lifting it above the floor is now the only way such a fixture has a perf gate at all. The trip counts are solved, not guessed. Timing pypy at 1x, 8x and 32x of each committed knob gives a per-iteration slope; the new knob is the count that puts pypy at FLOOR_GATE_MIN_BASELINE_S, rounded up to two significant digits. Of the 270 clamped fixtures, 143 keep their trip count as an inline literal rather than a module-level knob and are left alone -- a `range(300)` is often a recursion depth or a threshold, and rewriting it silently changes what the fixture covers -- 7 are flat under pypy at any size, and 91 respond. 20 of those reach the floor within 64x while keeping pyre's projected execution well under the 20s bench timeout, which is the bound that excluded `closure_per_call` (23.2s), `inline_gate_operand_provenance`, `binary_slice_index` and `class_reassign_hot`. Three more were scaled and then reverted, because scaling them bought CI time and no gate: `exc_info_module_loop_hot` and `exec_defined_global_read` still read clamped afterwards, and `enumerate_bignum_start` still records `loops_compiled=0`, so its ratio would compare two interpreters rather than any generated code. Five ceilings move as a consequence, in both directions, because an unclamped ratio is a different quantity from the clamped one it replaces: `bound_method_builtin_fold` 48 -> 160 and `inlined_helper_mutation` 45 -> 145 were fitted against the clamp and fail without this; `unary_negative` 110 -> 220 sat directly on its own reading; and `unary_positive_resume` 90 -> 5 collapses, because at the old trip count the numerator was mostly pyre's fixed warmup, which the longer loop amortises. `loop_callee_shared_mutation` predates the convention and had no ceiling at all; it compiles two loops and now gets one. Measured cost: +273s per check.py run, about 14 minutes across the three runners. Assisted-by: Claude * bench: record the pypy_type_surface counters main's own CI reports The fixture arrived with #999 recording bridges_compiled=5 and guard_failures=1011, and the JIT changes that landed after it moved both without a re-record. It reads bridges_compiled=102 and guard_failures=20497 here on all three backends, with loops_compiled=11 unchanged. Not this branch's: `pypy_type_surface` contains no `range()` for its only interpreter change to reach, and main's own `pyre CI` run at this base fails on exactly these three lines from ubuntu-24.04 -- `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497 (observed loops_compiled=11 bridges_compiled=102)` -- so the recorded value reproduces on neither platform. Re-recording it agrees with both. Assisted-by: Claude * bench: refit the sixth ceiling the unclamped ratio invalidates `list_append_funcentry_helper` reads 61.9x on cranelift against its ceiling of 61 once its pypy baseline clears the startup-subtraction floor. Same cause as the five refitted alongside the trip counts: the ceiling was fitted while the baseline was pinned to the floor, which over-states pypy's work and so under-states the ratio built on it. Twice the slowest backend observed. Assisted-by: Claude
…ry (#68) (#1107) * bench/synth: re-record jitstats baselines for three synthetics binary_int_overflow_local_resume, exc_bridge_entry_guard_not_removed, and list_append_write_barrier_gc across dynasm/cranelift/wasm: bridges_compiled drops by one and guard_failures decreases after the trace-time unary int specialization removes deopts (loops_compiled and loops_aborted unchanged, internal_compile_panics=0). Adds the fbw_blackhole_adopted_multi_frame, fbw_blackhole_adopted_single_frame, and fbw_store_journal_rollback_failed counters (all 0). Values are identical across the three backends. Assisted-by: Claude * posix: correct the DirEntry.stat caching citation to the object-cache model dir_entry_stat caches and returns the built stat_result object (`entry.stat() is entry.stat()`), which is `posixmodule.c DirEntry_get_stat`, not `interp_scandir.py descr_stat` (which caches raw stat data and rebuilds a fresh result per call). The doc comment cited the latter; correct it and note the object-caching is the 3.14 behavior. Assisted-by: Claude * posix: DirEntry.inode() returns the inode captured at scandir enumeration W_DirEntry gains a native enum_ino field. The name-based scandir path captures the readdir inode (DirEntryExt::ino(), never a stat) and dir_entry_inode returns it directly, the way descr_inode returns self.inode; it is -1 when unavailable (the scandir(fd) path and non-unix hosts), which falls back to the existing stat. A primitive field is GC-safe — the pyre_class macro emits PTR_OFFSETS only for PyObjectRef fields. Assisted-by: Claude * posix: answer DirEntry type/inode predicates from the enumeration dirent Capture the d_type and d_ino readdir reports at scandir enumeration into W_DirEntry.enum_type / enum_ino. The name path enumerates through opendir/readdir instead of std::fs::read_dir so it reads the dirent directly, and the descriptor path captures the same fields through fd_readdir. is_dir/is_file/is_symlink answer from enum_type unless it is DT_UNKNOWN or a followed symlink, and inode() answers from enum_ino on the descriptor path too, both without a stat. A known DT_DIR/DT_REG therefore answers is_dir()/is_file() from the cached type even after the entry is removed, as descr_is_dir/is_file do. The wasm, sandbox, and non-host_env paths keep the std fallback with enum_type DT_UNKNOWN and stat. Extract readdir_collect and fd_readdir out of fdlistdir, and add join_dir_name for the enumerated full path. Assisted-by: Claude * jit: decline the `Cls.__name__` fold when its metaclass guard cannot hold `try_walker_specialize_load_type_name_attr` takes the metaclass from `baseobjspace::type_name_obj_fast_path`, which asks `typedef::type`. That falls back to `gettypefor(ob_type)` when the receiver's `w_class` slot is null (typedef.rs:234-243; the objects it covers sit in RODATA, so writing the slot would SIGBUS). The fold then guards the metaclass by reading the raw `w_class` field, so for such a receiver it emits `guard_value(NULL, type)` — a guard that fails on every execution and that nothing can discharge, because no later write fills the slot. One class reaches it. The `getset_descriptor` type object is built lazily from inside the init loop (typedef.rs:9501) as a builder for the descriptors other typedefs install, so it never enters the registry the post-loop sweep walks to stamp `w_class = type` (typedef.rs:1546-1553). `typedef::type` still answers `type` through the fallback, so `type(x)` and `x.__class__` are correct and the null slot is invisible from Python. `synth/pypy_type_surface` reads `type(type.__dict__[name]).__name__`, so it paid one guard failure per iteration on every backend: dynasm, cranelift and wasm all reported `bridges_compiled 5 -> 102, guard_failures 1011 -> 20497` against the figures #999 `dad2a722907` recorded — 19486 excess failures over 20000 iterations, and 19486/200 = 97 excess bridges, one per `trace_eagerness` bucket. #1097 `d51ea7f32a0`, which added the fold, merged 83 seconds before #999, and the recorded figures are the ones this decline reproduces. Localized by taking the fixture apart: `check_descriptor_kinds` alone carries all 97 bridges; the receiver's own value is irrelevant (a loop-invariant object reads the same), the `FOR_ITER` is irrelevant (removing it reads the same), and `int`, `str`, `list`, `NoneType`, `object`, `type`, `method_descriptor`, `wrapper_descriptor`, `builtin_function_or_method` and a user class are all clean. `getattr(C, "__name__")` — same semantics, but the fold declines because the site's name is not in the code's name table — is clean too. check.py dynasm 406/406, cranelift 406/406, wasm 402/402, all green. `synth/pypy_type_surface` reads its recorded 11/5/0/1011 exactly and `synth/type_name_attr_fold` still reads its own 5/0/1/4, so the fold keeps applying wherever its guard holds. The null slot itself is left alone. Stamping `w_class` on the `getset_descriptor` type object would restore the invariant the sweep's comment states and let the fold apply there as well, but it moves recorded baselines and belongs in its own change. Assisted-by: Claude (cherry picked from commit 95ec7f2) * majit-gc: serialize gcreftracer registry tests against concurrent walks walk_all_gc_tables is reachable from any collector test's collection through the globally-registered gc_table_extra_root_walker once any table has existed. Under the parallel test harness a collector test's collection can upgrade a Weak the dropping registry test expects dead, transiently resurrecting the table and failing "freed table must not be walked". Add a test-only RwLock: every walk takes the read side, a registry test takes the write side across its whole drop/observe window. Split the walk into a locked outer and an unlocked inner so a registry test holding the write side observes the registry without re-entering the lock. Compiled out in production, where the STW collector already prevents a concurrent drop. Assisted-by: Claude * posix: write Windows utime through SetFileTime so pre-epoch times round-trip The Windows utime path carried its timestamps as an epoch-based unsigned Duration, so u64::try_from turned away every time before 1970. A FILETIME counts 100-ns intervals from 1601-01-01, which holds a pre-epoch time as an ordinary positive count, so convert the seconds/nanoseconds to a FILETIME and write them with CreateFileW + SetFileTime, the write rposix.win32_utime makes. Failures go through fs_err_with_filename so the OSError carries .winerror, as the module's other Windows path calls do. Enable the Win32_Security feature CreateFileW's signature needs. Assisted-by: Claude * extra_tests: skip the POSIX-only pathconf section of os_utime_pathconf_truncate on Windows pathconf and pathconf_names are POSIX-only; Windows CPython has neither, so the section raised AttributeError there under the oracle. Guard it with sys.platform, matching the file's existing utime/truncate Windows guards, and refresh the block comment now that SetFileTime writes pre-epoch times. Assisted-by: Claude
`pypy_type_surface` reads bridges_compiled=5 and guard_failures=1011 on all three backends, the figures #999 `dad2a722907` recorded. #1097 `d51ea7f32a0` moved them to 102/20497 by folding `Cls.__name__` across a `getset_descriptor` whose `w_class` slot is null, and #1107 `9614f37fb1e` declined that fold again; #1107's own message records the fixture reading 11/5/0/1011 afterwards. #1086 `e5eff816849` sits between the two and wrote the regressed pair in as the baseline, so the restored reading has failed the gate on every runner since. The same commit reverted three wasm guard_failures counters to their pre-#1106 values while keeping #1106 `4555e3d76ba`'s other fields: ca_bridge_multiframe_resume_double_call 2592 -> 2581, wasm_ca_trampoline_decline 601 -> 404, recursion_memo_branch 4704 -> 4724. The guest reads #1106's values both here and on ubuntu-24.04, the only runner that runs wasm, and each fixture's loops_compiled and bridges_compiled already agree with what both observe. Measured with `pyre/check.py --snapshot-diff` per backend and pattern: dynasm 18/18, cranelift 18/18, and wasm 15/15 for each of the three wasm fixtures. Assisted-by: Claude
…ise the int box's collector-heap arm (#1119) * bench: re-record the pypy_type_surface counters this branch produces Reads bridges_compiled=5 and guard_failures=1011 on all three backends on this tree -- the values #999 originally recorded -- against the bridges_compiled=102 / guard_failures=20497 taken from main a few commits ago and still on file. loops_compiled=11 is unchanged either way. The branch's only tree-wide change is the discovered graph set: seeding `BUILTIN_WRAPPER_DESCRIPTORS` with the `list.pop` wrapper puts `__pyre_wrap_list_descr_pop`, `list_method_pop` and `w_list_pop_end_inner` in the jitcode index, none of which are in it without the wrapper. `retraces_compiled=0` comes along because the recorder writes every counter it reads; an absent key already read as zero. Assisted-by: Claude * list: split w_list_pop_end at the lock and restore descr_pop's index == -1 arm `list_method_pop` reached `w_list_pop` for every index; `descr_pop` (listobject.py:758-772) routes `index == -1` to `pop_end` before the general path, so add that branch back. `w_list_pop_end` now holds the `w_list_lock` acquire and the length check and delegates to a new guard-free `w_list_pop_end_inner`, the same split `w_list_append` / `w_list_append_inner` already carry directly above. Both halves stay look-inside so the codewriter reaches the inner body. The Integer arm spells the shrink out as `ll_list_int_length` / `ll_list_int_getitem_fast` / `ll_list_int_set_len` rather than `IntArray::pop`. `list.pop` binds through a `__pyre_wrap_list_descr_pop` gateway registered in `BUILTIN_WRAPPER_DESCRIPTORS`, and `jit_trace_fnaddrs` gains both pop entries. Assisted-by: Claude * jit-trace: fold list.pop() by descending w_list_pop_end_inner `dispatch_residual_call_iRd_kind` recognizes a bound zero-argument `list.pop` on an Integer-strategy receiver and runs `run_sub_jitcode_walk` over the charon body of `w_list_pop_end_inner`, so the trace carries the length read, the item read and the length store instead of a `CallMayForce` residual. It sits before the generic gateway inlining, which would otherwise descend into the wrapper and stop at `w_list_len`'s lock, and is restricted to the root walk. `FBW_APPEND_JOURNAL` becomes the ordered `FBW_LIST_EFFECT_JOURNAL` carrying both `Append` and `PopEnd` entries: a pop restores a value, not just a length, so the two effects have to be rewound in one reverse-order replay. `PopEnd` carries the popped item boxed and root-walked, and its rollback dispatches on the receiver's current strategy — a later ordinary append of a non-int re-strategises the list and its Integer block stops describing live storage. Length is restored before the item in every arm, because `ll_list_*_setitem_fast` writes through a slice bounded by the live length. Assisted-by: Claude * bench: refit list_pop_append's ceiling and pin the pop fold's declines `list_pop_append` read 73.5x locally and 188.0x on windows-latest while `list.pop()` residualized; with the fold it reads 3.2x on dynasm and 4.3x on cranelift here, so the ceiling comes down from 376 to 22 -- twice the slowest local reading carried across this fixture's own measured host span (188.0/73.5). `list_pop_fold.py` covers the folded shape with a live and a dead result, two alternating receivers, a drain past empty, and the four shapes that have to decline and still answer: the indexed form, Float and Object storage, and an Integer list that flips to Object mid-loop. Each loop re-reads what it popped, so a rewind that restores the length but not the value, or restores them in the wrong order, changes a checksum. Assisted-by: Claude * celldict: born the copy destination on UnicodeDictStrategy CellDictStrategy::copy allocated the destination with w_dict_new(), which installs OBJECT_DICT_STRATEGY. celldict.py:208-209 takes the destination storage from UnicodeDictStrategy and wraps the result with that strategy. Allocate through w_dict_new_with(UNICODE_DICT_STRATEGY_REF, strategy.get_empty_storage()). The unicode strategy's empty storage is the same IndexMap<ObjectKey, PyObjectRef> box the object strategy uses, so the str-keyed fill below is unchanged, and alloc_dict_object already pins the storage across the header allocation. Assisted-by: Claude * intobject: residualise w_int_new's collector-heap arm w_int_new built the W_IntObject on the stack and copied it into the block try_gc_alloc_stable_raw returned. The boxing-constructor pass fuses only the lltype::malloc_typed(%agg) route into a NewWithVtable, so on the collector route %agg stayed live as the whole-struct store's operand and the PyObject header's SyntheticTransparentCtor survived into the emitted body as a call to a symbolic_fnaddr hash. A sub-jitcode walk that descends w_int_new -- the list.pop() fold's Integer arm does -- cannot record that call, so it declined the whole descent. Move the collector route into w_int_gc_alloc, marked dont_look_inside and bound in jit_fnaddr, so the descent records one call to a registered address. gc_interp::enabled() is false on the native backends, so their malloc_typed arm and its fusion are unchanged. Writing the header fields into the raw block from the traced body instead is not equivalent: the wasm backend does not lower the ob_type store at offset 0 faithfully, and list.pop() then returns a box whose w_class is wrong. list_ops, list_insert_pop_index and list_bound_method_mutation catch that; list_pop_append does not, since it discards the popped value. gc_interp::enabled and w_int_gc_alloc bind their extern "C" call trampolines rather than the raw fns. The wasm backend types a residual's call_indirect from the descr alone, so a raw `-> bool` / `-> *mut PyObject` traps `indirect call type mismatch`. The trampoline is emitted only for a syntactically raw pointer return, not for the PyObjectRef alias. synth/list_pop_append on wasm: 5.19s -> 0.84s. 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
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
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
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
…ts, and a measured FOR_ITER gate widening (#1103) * list: give the unused typed strategy an empty array, not a block `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 * optimizeopt: answer an unwritten field of a virtual with its typed zero 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 * mapdict: keep builtin storage on user subclasses 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 * jit: record integer zero-divisor raising arms Assisted-by: Claude * mapdict: harden builtin subclass carriers 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 * jit: initialize inline allocation scalar fields Assisted-by: Claude * jit: record range zero-step raising arms Assisted-by: Claude * jit: record float zero-divisor raising arms Assisted-by: Claude * jit: record bigint zero-divisor raising arms Assisted-by: Claude * jit: record negative bigint shift raising arms Assisted-by: Claude * jit: scope FOR_ITER safety to escaping range loops Assisted-by: Claude * bench: re-record seven wasm jitstats baselines on the rebased base 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 * jit: diagnose FOR_ITER gate opcode declines Assisted-by: Claude * jit: gate FOR_ITER decline census allocation Assisted-by: Claude * jit: gate FOR_ITER decline census collection Assisted-by: Claude * jit: guard numeric binary specialization classes Assisted-by: Claude * jit: retain context on specialized builtin raises Assisted-by: Claude * jit: skip redundant numeric class guards Assisted-by: Claude * test: drive the numeric subclass fixture through the specialized pc The fixture fed its subclass operand to a tail expression at a different BINARY_OP pc than the loop that went hot, so that site was never specialized and the check passed on a binary without the class guards. Iterate a list whose tail holds the subclass instead, so it arrives at the pc under test. Adds left-operand cases and a bool-driven case, which reaches the tagged and bool path where walker_numeric_builtin_class returns null and no class guard is emitted at all. Assisted-by: Claude * Grow FOR_ITER regions through handler rejoins Assisted-by: Claude * Tighten escaping range append recognition Assisted-by: Claude * Update range constructor loop jitstats Assisted-by: Claude * jit: admit LIST_EXTEND in FOR_ITER bodies Assisted-by: Claude * jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate A LIST_APPEND body was admitted only when the body performed no call, because a mid-body abort after the append routed through fbw_foriter_inflight_take, which refuses delivery and dropped the iteration's item. range_ctor_in_loop goes from mc_entered=0 to 813. The surrounding comment previously cited blackhole.py as authority for the append being rolled back and replayed once. It is not: blackhole.py:1712 is setposition, which continues from the coordinate already reached, and upstream places the resume coordinate past a residual call so the effect is never re-executed. State instead what pyre actually relies on, and record that a non-committed walk exit still keeps the legacy entry replay whose delivery can be refused. Assisted-by: Claude * jit: census in-flight FOR_ITER delivery outcomes Assisted-by: Claude * bench: re-record nineteen jit-stats baselines Five fixtures enter compiled code where they previously did not, so their zero counters were zero trivially: exception_group_type loops_compiled 0 -> 1, guard_failures 0 -> 1 list_append_virtual_payload loops_compiled 0 -> 2, bridges 0 -> 8, guard_failures 0 -> 1603 minmax_key_rooting loops_compiled 1 -> 2, bridges 0 -> 2, guard_failures 5 -> 409 range_ctor_in_loop loops_compiled 1 -> 3, bridges 0 -> 3, guard_failures 0 -> 811 (812 on wasm) global_store_plain_dict_globals (wasm) loops_compiled 5 -> 6, loops_aborted 1 -> 2, guard_failures 1 -> 18 pickle_terminal_raise_resume (wasm) loops_compiled 67 -> 68, loops_aborted 13 -> 14, guard_failures 339 -> 356 mapdict_frozen_unboxing_fold takes guard_failures 2 -> 8 with loops_compiled unchanged at 3. An A/B across the call-bearing LIST_APPEND admission alone gives mc_entered 2 -> 8 on the same fixture, so the counter tracks compiled-code entries one for one: its `[C(i) for i in range(n)]` comprehension is a call-bearing LIST_APPEND body. gc_bug_bridge_flavor_traceback_names (wasm) improves guard_failures 2027 -> 1670. exception_reused_object_tb_not_doubled (wasm) loses fbw_blackhole_adopted_single_frame 3 -> 0. A binary built from 128590c with no branch commits applied reports 0 on the same fixture, so the fall is the base's; the baseline was last recorded at 779da08. Every other counter on that fixture is unchanged (loops 4, bridges 3, aborted 3, guard_failures 600) and its traceback-shape oracle passes. The recorder also writes the fbw_* and field_pos_* keys that were absent from baselines recorded before those counters existed. Assisted-by: Claude * Trace traceback escape marking in exception attribute fold Assisted-by: Claude * Trace fresh container allocations in FOR_ITER callees 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 * Admit tuple copies from exact lists during replay Assisted-by: Claude * Identify traceback walk bridge training 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 * bench: add a synthetic fixture for the subscript inline's index operand `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 * bench: re-record the wasm pickle terminal-raise baseline `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 * jit: pair the vable static shadow write with a heap write-back `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 * docs: list the two FOR_ITER gate diagnostics in gate-triage `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 * bench: re-record ten jit-stats baselines after the rebase 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 * jit: follow the InflightForiterBody field rename in the census `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 * mapdict: split the layout predicate from the storage predicate `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 * _structseq: re-read the pinned class after the tuple allocation 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 * jit: gate the in-flight FOR_ITER census key lookup on the census `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 * jit: correct the exception descr group note on w_context `w_context` is written by the raise lowering, not left zeroed by GC pointer clearing. Assisted-by: Claude * test: scan the loop-region fixture in two passes `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 * test: cover synchronize_virtualizable_static 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 * majit: exclude the identity slot from the static write-back bound `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 * jit: read PYRE_FOR_ITER_GATE_DIAG through one accessor The per-opcode decline and the whole-region decline each owned a function-local `OnceLock` for the same variable. Assisted-by: Claude * jit: admit builtin subclass carriers in the mapdict storage helpers 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 * Revert "jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate" 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 * majit: drop the narrowed virtualizable static synchronizer `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 * bench: restore five jit-stats baselines the reverted gate had moved 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 * bench: restore the cranelift jit-stats baselines to match Same six benches as the dynasm pass, same direction and magnitude. Assisted-by: Claude * bench: restore the wasm jit-stats baselines to match 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
Summary
Eight commits, rebased onto current
main:stdlib: stabilize collections JIT and heapq fallbackheapqfix sandbox bootstrap and merge-point stack syncMergePointVstackUnbound)stdlib: pass contextlib and copy baselinesFix enum exception carrier lifetimePUSH_EXC_INFOloweringPass dataclasses slot attribute testsFix bisect gate and macOS benchmark timing_bisectaccelerator module_bisect: root operands across the callbacks_bisectoperand in shadow-stack slots;__index__,__len__,__getitem__,keyand__lt__all run arbitrary Python, and a minor collection relocates a young object behind a rawPyObjectRefImplement the _json accelerator and harden regex JIT paths_jsonaccelerator; regex JIT pathsValidation
python3 pyre/check.py(both backends),extra_tests/parity_tests/run.pyandcpython_tests/run.pywere run against one fixed tree — the tree fingerprintwas captured before, twice during, and after the run and is identical at all
four points.
Known failures and where they come from
Four of them share one cause inside this branch.
try_walker_inline_type_callgained
type_call_decline("overridden __init__ requires descr_call frame")inImplement the _json accelerator and harden regex JIT paths. Removing thatsingle hunk turns all four green:
synth/attr_store_add_transition(dynasm + cranelift)pickle_repeated_framed_unpickleunsupported pickle protocol: 149synth/context_managerguard_failures 1 -> 201synth/pickle_terminal_raise_resumeloops_compiled 35 -> 33The decline runs in the failing tests —
PYRE_FBW_INLINE_DIAG=1prints it twiceduring the pickle parity script.
attr_store_add_transitionis not a hang: givena 600 s budget it completes in 499.8 s, because the decline drops a 64 M-iteration
loop to the interpreter.
Two are not from this branch.
cranelift synth/exception_try_call_inlined_callee_raiseloops_compiled 3 -> 2— four worktrees that do not contain any of these commits also report 2.
cpython_tests test.test_collectionsTypeError: stack underflow during interpreter peek— JIT-only, reproduces on four worktrees without thesecommits, and still fails with the decline hunk removed. Trigger is a
withinside an
excepthandler;try/exceptalone does not reproduce. Themechanism is the one documented at
pyre-jit/src/eval.rsfor_iter_frame_is_finally_duplicated: a side exit resumes through the densecarry-forward
pc_map, which collapses un-traced regions into one marker, sothe resume lands at the wrong bytecode with an empty value stack. The existing
decline covers only a
finally-duplicatedFOR_ITER, so this handler-sidetrigger passes through it.
baseline.jsonrecords this module as PASS.test.test_asyncioFails as a single module and needs to be run per-submodule instead:
cpython_tests/run.pytreatstest.test_asyncioas one module, which is whatproduces the FAIL; every submodule passes on its own.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Counteroperations.