majit, pyre: twenty census slices — residual boundaries, unsigned-bank folds, dyn-indirect on by default - #873
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (30)
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 |
![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: 8daf008c6c
ℹ️ 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".
| /// | ||
| /// # Safety | ||
| /// `w_dict` must be a valid `W_DictObject` on `INT_DICT_STRATEGY`. | ||
| #[majit_macros::dont_look_inside] |
There was a problem hiding this comment.
When compiled code reaches an int-dict strategy promotion, this marker turns the entire mutation into an opaque residual solely because IndexMap lacks a lowering, even though the comment acknowledges that upstream traces the body. That is a translator-generation gap rather than an acceptable boundary: add the corresponding dict construction/insertion lowering and keep the promotion traced. The same defect is repeated for the bytes, kwargs, identity, and module-dict promotion helpers introduced in this commit.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2d87f58). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`annrpython.py:437-438` sets `e.source = source_lines(graph, block, None, long=True)` on the UnionError before either recording it under `keepgoing` or re-raising; the port dropped that step, so the error carried only the two offending annotations. `source_lines` is already ported (`tool/error.rs:400`) and the sibling `KeyError` panic in the same function already prints slot/block context — only this arm was missing it. Without it the message does not identify the merge. The merging block is routinely an inlined callee, so the graph named by the census subject is not the graph that failed: `pyre_interpreter::pyopcode::execute_copy` reports `r_uint ∪ int` but the merge is in `opcode_copy_value`, and the five `*pop_jump*` subjects all resolve to one block in `BranchOpcodeHandler::guard_truth_value`. The failing input slot is threaded out of the `unionof` fold so it can be named alongside the graph. Census is unchanged at phaseA 314 / phaseB 24 with an identical histogram — this alters an error string, not a disposition. Assisted-by: Claude
`source_lines1` answers `no source!` for every graph lowered from LLBC rather than from Python, because `graph.source` is absent — which is all of them here. So the `source_lines` context added alongside it names the failing graph but never shows the block, losing the half of upstream's message that localises the merge (`error.py:34-48` quotes the block's operation line range). Quote the block's operations instead, rendered the way `gather_error` already renders an operation (`tool/error.rs:498`). Blocks with no operations — which is what a graph's startblock looks like — say so explicitly, and that distinction is itself the diagnosis: `pyopcode::u32_as_usize` reports `int ∪ r_uint` at slot 0 of a block with no operations, i.e. its own entry, so the two annotations are the caller-supplied ones and nothing in the body is involved. Census is unchanged at phaseA 314 / phaseB 24 with a byte-identical histogram. Assisted-by: Claude
`SYS_MODULES` (`LazyLock<Mutex<HashMap<String, usize>>>`) is the process-owned name→module registry, distinct from the `SYS_MODULES_DICT` pointer already residualized beside it. Its entries are stamped at runtime by `set_sys_module` / `remove_sys_module`, so it is not a build-time constant and the front end must not trace into the read. The two pure readers — `check_sys_modules`'s fallback after the `sys.modules` dict misses, and `get_interpreter_sys_module` — now share one `#[majit_macros::dont_look_inside] sys_modules_registry_get(name)` seam with a `jit_fnaddr` `push_alias_pair` row, the `lookup_exc_class` shape (same `fn(&str) -> Option<PyObjectRef>` signature). The mutators and the GC walkers keep the raw static. The merged seam is poison-tolerant, which is `get_interpreter_sys_module`'s existing spelling rather than `check_sys_modules`'s `.unwrap()`: a poisoned mutex means another thread panicked while holding it, and a `HashMap<String, usize>` carries no invariant that a panic mid-`insert` or mid-`remove` could leave broken. Census, freshly re-extracted corpus. Default (gate off) is unchanged at phaseA 314 / phaseB 24 — `SYS_MODULES` contributes no record there. Under `PYRE_DYN_INDIRECT=1`, where the dynamic-dispatch bodies reach it, the root `pyre_interpreter::importing::SYS_MODULES` goes 12 → 0 and distinct roots 52 → 51. Records stay at 214: ten of the freed graphs re-root one wall deeper at `std::io::stdio::stderr` and two at `vec::Vec::index`, so this is again a roots-cleared rather than records-cleared slice. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 328/328, cranelift 328/328, wasm 325/325. Assisted-by: Claude
Both are host boundaries whose value is produced fresh per call, never a build-time constant, and each is the single in-tree seam its traced callers reach the host through: - `host_seam::emit_stdout` / `emit_stderr` — the fd 1 / fd 2 writers that tracebacks, the warning machinery (`warn` -> `warn_category` -> `emit_stderr`) and the displayhook all funnel through. Marked in both spellings: the unix bodies in `host_seam.rs` and the non-unix mirror declared inline in `lib.rs`, since a corpus extracted on a non-unix host sees only the latter. - `module::thread::current_ident` — reads `rustpython_host_env::thread::current_thread_id`. Each takes a `jit_fnaddr` `push_alias_pair` row; `&[u8]` arguments and the `()` / `i64` returns need no result shell. Census, freshly re-extracted corpus: | root | gate off | gate on | | --- | --- | --- | | `std::io::stdio::stderr` | 2 -> 0 | 12 -> 0 | | `rustpython_host_env::thread::current_thread_id` | 4 -> 0 | 12 -> 0 | Default (gate off) phaseA 314 -> 313, distinct roots 48 -> 46; three of the freed graphs re-root at `__dyn_call` and one at `saturating_sub`. Under `PYRE_DYN_INDIRECT=1` records go 214 -> 199 and roots 51 -> 49, with eleven re-rooting at `vec::Vec::index` — now the largest single item left on the ledger that gates the gate flip. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 328/328, cranelift 328/328, wasm 325/325. `wasm32-unknown-unknown` target check clean, which is the configuration that compiles the non-unix mirror. Assisted-by: Claude
…ger banks
`vec_index_regular_leaf` spelled its index-type test inline. Extract it as
`vec_index_type_is_scalar` and record why both banks are accepted: `usize` —
what essentially every real callsite indexes with — serializes as
`{"UInt": "Usize"}` and types as `Unsigned`, not `Int`, so an `Int`-only test
leaves the fold dead. A `Range` index types as `Ref` and stays rejected,
which is what the gate exists for.
The acceptance set itself is unchanged: #814 widened it in the same place.
What was missing is a test that runs — the trap was caught only by the
`#[ignore]`d real-LLBC anchor `vec_index_mut_fill_user_function_args_real`,
which needs the 440MB corpus and does not run in CI. The new unit test pins
both banks and the `Range` rejection off a JSON fixture.
Assisted-by: Claude
`try_eq_w` was the only `try_*` hook wrapper in `dict_eq_hook` that read its thread-local inline. Its two siblings, `try_hash_w` and `try_hash_str`, both decompose into a marked `has_*_hook` predicate plus a marked `*_hooked` trampoline returning a sentinel, leaving the `Option` construction visible to the front end. `try_eq_w` now does the same: `has_eq_w_hook` / `eq_w_hooked` carry the `dont_look_inside`, the wrapper is a plain `if`, and the two leaves get `jit_fnaddr` rows beside `has_hash_w_hook` / `hash_w_hooked`. Same value on every input: `eq_w_hooked` returns `false` when no hook is installed and the wrapper only calls it under `has_eq_w_hook`, so `None` still means "no hook" and `Some(false)` still means "not equal". Census under `PYRE_DYN_INDIRECT=1`: `pyre_object::dict_eq_hook::EQ_W_HOOK` 19 -> 0. Distinct roots and records do not move (49 / 198 before and after) because the same nineteen graph heads — set-identical, not merely equal in count — re-root at `map::IndexMap::get`, which sat directly behind it. Their cascade depth collapses 3 -> 1 (`dict_keys_equal -> try_eq_w -> EQ_W_HOOK` becomes a direct failure), so depth-1 goes 66 -> 85 and depth-3 52 -> 33. Gate off is unchanged at 45 / 214. This changes generated code — one blocked graph becomes two residual calls — so it was validated end to end rather than by census alone. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 328/328, cranelift 328/328, wasm 325/325. Assisted-by: Claude
Of the seven `DictStrategy::getitem` impls, six delegate their storage probe to a named `w_dict_lookup_*_strategy` / `w_module_dict_lookup_inner` helper. `IdentityDictStrategy::getitem` was the exception: it called `identity_storage(w_dict).get(&IdentityKey(w_key))` inline in the trait method, so the raw `IndexMap::get` sat in the traced body with nowhere to attach a marker. Extract it as `w_dict_lookup_identity_strategy`, marked `dont_look_inside` with the same citation the int twin already carries (`rordereddict.ll_dict_getitem`, traced only for a virtual dict), and give it a `jit_fnaddr` row beside `w_dict_lookup_int_strategy`. `IdentityKey` hashes and compares by address, so the residual boundary encloses no user Python code at all — the discriminator that separates a residualizable per-key probe from a container walk that has to stay traced. Census under `PYRE_DYN_INDIRECT=1`: `map::IndexMap::get` 19 -> 15 and the `getitem` graph clears entirely; the four freed graphs re-root at `IndexMap::with_capacity` in `switch_to_object_strategy`, and the other fifteen turn out to be a different graph (`w_module_dict_lookup_inner`). Roots and records do not move yet (49 / 198). Gate off unchanged at 45 / 214. Assisted-by: Claude
`w_module_dict_lookup_inner` is `celldict.py:131-141`: a str fast path, a never-equal-to-string shortcut, a strategy promotion, and — in two of its arms — an `IndexMap::get` on the promoted object storage. Only the probe is opaque to the tracer; `celldict.py` carries no residual marker on the router itself, so marking the whole function would swallow branches PyPy traces. Both arms now call `w_module_dict_lookup_object_entries`, which is the probe alone under `dont_look_inside` with the `rordereddict.ll_dict_getitem` citation the other dict lookup leaves carry. It returns `None` when the dict is not on object storage, which is what the second call site's `?` spelled. Also drops the `dont_look_inside` added to `w_dict_lookup_object_strategy` while attributing this cluster: it measured bit-identical with and without, and reverting it here does not bring the `getitem` graph back, so it was carrying nothing. Census under `PYRE_DYN_INDIRECT=1`: `map::IndexMap::get` 15 -> 0, distinct roots 49 -> 48. Records stay at 198 — the fifteen graphs re-root at `core::slice::<Impl>::as_ptr`, which goes 11 -> 26. Gate off unchanged at 45 / 214. This and the preceding identity-leaf commit change generated code — a traced map probe becomes a residual call — so they were validated end to end together at this tree state rather than by census alone. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 328/328, cranelift 328/328, wasm 325/325. Assisted-by: Claude
…_sub Nine sites across `type_methods` and `baseobjspace` computed "how many arguments did the caller pass, not counting the receiver" as `args.len().saturating_sub(1)` — always inside a TypeError message, always on a gateway slice whose slot 0 is the bound receiver. They now call a shared `args_given`, which spells the same value as an explicit branch. `core::num::<Impl>::saturating_sub` is an unregistered foreign leaf, so it stopped every graph that reached one of these lines; a plain subtraction lowers to `Sub` plus an overflow `Assert` the front end already strips (`front/mir.rs:3326`). Same value on every input, including the malformed empty slice: `saturating_sub(1)` and the new branch both answer 0. `resolve_dict_backing`'s `nslots.saturating_sub(newslotnames.len())` is a different quantity, so it gets its own explicit branch rather than the helper. The remaining `saturating_sub` callers (`call_valuestack`'s drop count, the string-search cursors, the formatter width) are untouched — no census record reaches them. Census: `core::num::<Impl>::saturating_sub` 27 -> 0 under `PYRE_DYN_INDIRECT=1` and 28 -> 0 with the gate off. Records drop 198 -> 196 and 214 -> 212 — the first graphs to lift outright rather than re-root. The other twenty-two surface the next leaf in the same bodies (`core::slice::<Impl>::get`, i.e. `args.get(2)` / `args.get(3)`), which is why the distinct-root count reads 48 -> 49: one large root split into two smaller ones. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 328/328, cranelift 328/328, wasm 325/325. Assisted-by: Claude
`generator_throw_impl`, its async twin, and the `send`/`asend` pair read their optional positional arguments as `args.get(n).copied().unwrap_or_else(w_none)`. They now call a shared `arg_or_none`, which spells the same value as an explicit bounds test. `core::slice::<Impl>::get` is an unregistered foreign leaf, and unlike the integer helpers it cannot be folded either: it yields an `Option<&T>` whose payload is valid on only one arm, and the front end's single `Option`-synthesizing path writes the payload unconditionally in the call's block, before the consumer's discriminant switch runs (`emit_tagged_pair_aggregate`, documented at `front/mir.rs:9936`). An out-of-bounds read would therefore execute on the absent arm. No lowering in `front/mir.rs` splits a block, so there is no in-tree way to emit the guarded read a fold would need. A plain index has no such problem — it lowers to a native read plus an overflow `Assert` the front end already strips. Census: `core::slice::<Impl>::get` 27 -> 0 under `PYRE_DYN_INDIRECT=1` and 27 -> 0 with the gate off. Roots and records are unchanged in both (62 / 245 and 59 / 249): all twenty-seven re-root at `iter::adapters::skip::Skip::next`, which is a different function (`dict_view_iter_reduce_method`), so the generator-throw family is now clear. The same `args.get(n)` idiom appears at roughly 165 further sites across `pyre-interpreter`. They are left alone: no census record reaches them, and a sweep of that size is worth doing on evidence rather than on symmetry. `python3 pyre/check.py --backend dynasm,cranelift,wasm` — dynasm 331/331, cranelift 331/331, wasm 327/327. Assisted-by: Claude
`try_hash_str` gates `hash_str_hooked(ptr, len)` on `has_hash_str_hook`, both already `dont_look_inside`, but spelled `bytes.as_ptr()` in its own traced body. `core::slice::<Impl>::as_ptr` is an unregistered foreign leaf — unlike `slice::len`, which `is_container_len` folds (`front/mir.rs:8583`) — so the call stopped `try_hash_str` and every caller above it. `hash_str_hooked_bytes(&[u8]) -> i64` carries the marker and a `push_alias_pair` row, so the pointer/length extraction runs inside a boundary that is already opaque. A `&[u8]` argument crosses a residual call unchanged (`host_seam::emit_stdout` is the same shape) and the `i64` return is one word. Census, re-extracted corpus, both gate states: | root | OFF before | OFF after | ON before | ON after | |------------------------------------|-----------:|----------:|----------:|---------:| | `core::slice::<Impl>::as_ptr` | 8 | **0** | 23 | **0** | | `iter::adapters::skip::Skip::next` | 27 | **0** | 27 | **0** | | records | 249 | 247 | 245 | 243 | | distinct roots | 59 | 69 | 62 | 71 | `try_hash_str` leaves the blocked-function list in both states. The rising root count is the analyzer descending further rather than a regression: with the str-hash leaf lifted, the str-keyed dict path is walked for the first time and reports the walls behind it — `dict_entries_get_str -> IndexMap::get` 18 and `str_utf8_w -> Wtf8::as_str` 11 (gate ON), `w_dict_getitem_str`, `strategy_is` and `w_dict_delitem` all at `__dyn_call` (gate OFF, 36 records the gate removes). `Skip::next` reaching 0 is the same effect from the other side: the graphs that used to reach `dict_view_iter_reduce_method` — `getattr_str_impl` and its callers — now stop earlier, and `getattr_str_impl` reports as a root itself. check.py green on all three backends. Assisted-by: Claude
…espace `dict_entries_get_str` spells `IndexMap::get` twice — once behind the borrowed `StrLookupKey` when the str hash hook is installed, once behind `object_key_for(w_str_new(key))` when it is not. `dict_entries_probe_str` and `dict_entries_probe_object` carry the marker and a `push_alias_pair` row each, so both arms cross the same boundary `w_dict_lookup_int_strategy` and `w_module_dict_lookup_object_entries` already sit behind — the oopspec'd residual arm of `rordereddict.ll_dict_getitem`. The hook gate, the stale-eq reset and the arm selection stay traced. Marking only one arm is inert: a body that reaches either `get` stops there, so the enclosing graph reports the same wall. Measured — with the borrowed probe alone, `IndexMap::get` moved 19 -> 17; with both, 19 -> 4. `_warnings::state_ns` reads a namespace pointer `setup_after_space_initialization` stamps at runtime, so it is not a build-time constant; it takes the `importing::sys_modules_dict` treatment (marked, bound by qualified path, single-word `PyObjectRef` return) rather than an address row that would fold whatever the build process saw. Census, re-extracted corpus, both gate states: | root | ON before | ON after | OFF before | OFF after | |---------------------------------|----------:|---------:|-----------:|----------:| | `map::IndexMap::get` | 19 | **4** | 4 | **3** | | `_warnings::STATE_NS` | 6 | **0** | 3 | **0** | | records | 243 | 236 | 247 | 244 | | distinct roots | 71 | 70 | 69 | 68 | `dict_entries_get_str` leaves the blocked-function list in both states. Behind it the next walls surface: `IndexMap::with_capacity` 5 -> 14 and `Wtf8::as_str` 19 -> 24 (gate ON). The `with_capacity` records are almost all depth-1 — the census enqueues the foreign method itself as a graph head under the dyn-indirect widening — so they are out of reach of a pyre-side change. check.py green on all three backends. Assisted-by: Claude
Every `switch_to_object_strategy` that rebuilds storage is `IndexMap` construction and refill end to end — `with_capacity`, a loop of `insert`, a GC box, then the strategy relabel. The int, bytes, kwargs and identity strategies each carried that body inline in their trait impl; the module dict carried it in a free function. Extracted the four impl bodies into free functions and marked all five, with a `push_alias_pair` row each. `mapdict_switch_to_object_strategy` already had the marker and is unchanged. Upstream carries no marker on `dictmultiobject.py:1223 AbstractTypedStrategy.switch_to_object_strategy`, and that is not an argument for tracing it here: upstream can trace the body because every step is an RPython dict primitive the JIT models (`ll_newdict`, `ll_dict_setitem`'s `look_inside_iff(isvirtual)`), while `IndexMap` has no lowering at all, so the last modellable point is the call itself. The `()` return crosses the residual boundary the way `w_set_copy_storage_from` does. Census, re-extracted corpus, both gate states: | root | ON before | ON after | OFF before | OFF after | |---------------------------------|----------:|---------:|-----------:|----------:| | `map::IndexMap::with_capacity` | 14 | **0** | 1 | **0** | | records | 236 | 229 | 244 | 244 | Seven of the fourteen advance to `Wtf8::as_str` (19 -> 30, gate ON) rather than lifting outright; that wall is the `&str` fat return, which no marker can cross. Marking only the two `dictmultiobject.rs` bodies first measured 14 -> 13: the census names an inner graph by its LEAF name, and `switch_to_object_strategy` has nine definitions across five files. The remaining three live in `kwargsdict.rs`, `identitydict.rs` and `mapdict.rs`. check.py green on all three backends. Assisted-by: Claude
`IdentityDictStrategy::delitem` spelled `IndexMap::shift_remove` in its own
body; `w_dict_delete_identity_strategy` takes it, leaving the keys-version bump
traced. Upstream's `_ll_dict_del` (`rordereddict.py:884`) carries
`@jit.look_inside_iff(jit.isvirtual(d) and jit.isconstant(i))`, and an
`IndexMap` can never be virtual to this front end, so the predicate is
permanently false and the residual arm is the only reachable one.
`setitem_list_slice` and the bytearray extended-slice arm walked their index
list with `.iter().enumerate()`, and the bytearray arm wrote through
`slice::get_mut`. Both are unregistered foreign leaves. Rewritten as index
loops with an explicit bounds test — the shape
`listobject.py setitem__List_ANY_ANY` and
`bytearrayobject.py setitem__Bytearray_ANY_ANY` use for the same walk.
Census, re-extracted corpus, both gate states:
| root | ON before | ON after | OFF before | OFF after |
|------------------------------------------|----------:|---------:|-----------:|----------:|
| `iter::adapters::enumerate::Enumerate::next` | 7 | **0** | 5 | **0** |
| `map::IndexMap::shift_remove` | 5 | **0** | 0 | 0 |
| records | 229 | 229 | 244 | 244 |
The record totals do not move: every cleared graph re-blocks one step later.
Gate ON the successors are `wrapping_add` 1 -> 6, `from_raw_parts_mut` 1 -> 4,
`ctypes::borrow_memory` 3 -> 5 and `Map::collect` 9 -> 11; gate OFF,
`from_raw_parts_mut` 1 -> 4 and `__dyn_call` 44 -> 46.
The two new successors are named:
- `core::num::<Impl>::wrapping_add`, inner graphs `delitem` x5 and
`w_dict_bump_keys_version` — the keys-version bump the delete leaf leaves
traced. `try_lower_wrapping_binop` (`front/mir.rs:9773`) already folds this
call, but its width gate reads `tyref_literal_int_atom` and admits only
`I64`/`Isize`, while `keys_version` is `usize` and serialises as
`{"UInt": "Usize"}`, so the fold declines.
- `core::slice::raw::from_raw_parts_mut`, inner graph `object_items_slice_mut`
— the `&mut [PyObjectRef]` view `w_list_setitem` goes through, now reached
because the index walk no longer stops at `Enumerate::next`.
check.py green on all three backends.
Assisted-by: Claude
`try_lower_wrapping_binop`'s width gate read `tyref_literal_int_atom` and
admitted only `I64`/`Isize`. `usize` serialises as `{"UInt": "Usize"}`, which
that accessor does not see at all, so every unsigned wrapping op kept the
generic `Call` form and stopped its graph — the same `Int`-only-test defect
`vec_index_type_is_scalar`'s doc already records. Gate on both banks the way
`try_lower_num_from`'s destination test (`front/mir.rs:10285-10291`) does.
Unsigned is the same machine op under a different rtyper dispatch:
`rint.py:217 rtype_add` -> `_rtype_template` prefixes `uint_`, and
`jtransform.py:1608-1610` renames `uint_{add,sub,mul}` straight back to
`int_{add,sub,mul}` for the JIT. Still word-sized only — a narrower
`wrapping_add` wraps at its own width and keeps the `Call` form.
The emitted `BinOp` now carries the receiver's signedness instead of a flat
`ValueType::Int`. `union_type` widens `Int u Unsigned` to `Unknown`
(`binaryop.py:191` UnionError), so annotating a `usize` sum as `Int` would
poison the merge where it meets the unsigned field read it came from.
Census, unchanged corpus (no re-extract — this is front-end only):
| root | ON before | ON after | OFF before | OFF after |
|----------------------------------|----------:|---------:|-----------:|----------:|
| `core::num::<Impl>::wrapping_add`| 6 | **0** | 1 | **0** |
| records | 229 | 228 | 244 | 243 |
The six were `w_dict_bump_keys_version` plus five `delitem` subjects reaching
it; `keys_version` and `clear_gen` are the `usize` counters involved. Five of
them re-block on `IndexMap::shift_remove` in `w_module_dict_delitem_inner`, one
wall further down the same delete path.
check.py green on all three backends.
Assisted-by: Claude
The five `delitem` subjects walk one chain, and clearing a link only exposes the next, so all three are taken together. `w_module_dict_delitem_inner` spelled `IndexMap::shift_remove` in both of its arms (the object-strategy arm and the post-promotion arm). `dict_entries_remove_object` takes the remove, the delete twin of `dict_entries_probe_object` over the same `IndexMap<ObjectKey, PyObjectRef>` table. Upstream's `_ll_dict_del` (`rordereddict.py:884`) carries `@jit.look_inside_iff(jit.isvirtual(d) and jit.isconstant(i))`; an `IndexMap` can never be virtual to this front end, so the predicate is permanently false and the residual arm is the only reachable one. Both arms go through the leaf — leaving one traced keeps the graph blocked whatever the other does. `VersionTag::fresh` bumped a fn-local `static NEXT: AtomicU64` inline. `next_version_tag_serial` is the read seam, marked because the counter is a runtime-mutable global: the value the build process observes is not a constant and folding it would hand every dict the same tag. Upstream allocates a fresh `VersionTag()` object here, which rtypes to a `malloc` the JIT models; a static atomic has no llop counterpart, so the call is the last modellable point. `notify_version_watchers` ran the `Vec::retain` sweep over its weak watcher flags inline. `sweep_version_watchers` takes the non-empty half; the `is_empty` early-out stays traced, so a mutation with no watchers still makes no call. This is the `version?` quasi-immutable invalidation walk, and upstream runs all of it outside traced code — `QuasiImmut.invalidate` (`metainterp/quasiimmut.py`) iterates `looptokens_wrefs` from the residual `jit_force_quasi_immutable` path, never from a trace. Census, re-extracted at each step, gate ON (gate OFF is unmoved at 243 throughout — every one of these is a dyn-indirect-only wall): | step | root cleared | before | after | records | |------|---------------------------------|-------:|------:|--------:| | 1 | `map::IndexMap::shift_remove` | 5 | **0** | 228 | | 2 | `celldict::<Impl>::fresh::NEXT` | 5 | **0** | 227 | | 3 | `vec::Vec::retain` | 4 | **0** | 227 | Each step's five (then four) graphs re-blocked one link further down the same delete path, which is why the record totals barely move. After step 3 the four remaining ones land on `Wtf8::as_str` (30 -> 34) — the fat `&str` return no residual ABI can carry, and the terminus of this chain. check.py green on all three backends. Assisted-by: Claude
`w_module_dict_lookup_inner_checked` and `w_module_dict_store_inner_checked`
spelled `IndexMap::{get,insert,pop}` inline. Three leaves take them:
`dict_entries_probe_hashed`, `dict_entries_insert_hashed` and
`dict_entries_pop_last`. `IndexMap` has no lowering, so the call is the last
modellable point — the same argument the sibling `dict_entries_*` leaves carry,
and upstream's `_ll_dict_setitem_lookup_done` (`rordereddict.py:674`) is
`@jit.look_inside_iff(jit.isvirtual(d) and jit.isconstant(key))`, neither
conjunct of which can hold for an `IndexMap` here.
The key crosses the boundary DECOMPOSED into `hash: i64` + `obj: PyObjectRef`
rather than as an `ObjectKey`: a struct by value is not a residual-ABI argument
shape, and the caller already holds both halves. `object_key_for_checked`
stays traced, which is required, not incidental — it clears the hash-error flag
on its `Err` path (`take_hash_error`, `dictmultiobject.rs:321`), so a design
that let the leaf swallow the raise and had the caller re-read the flag
afterwards would silently lose the error. The post-probe
`take_dict_key_error()` test and the `insert`/`pop` undo sequencing likewise
stay traced.
Census, re-extracted corpus:
| root | ON before | ON after | OFF before | OFF after |
|--------------------------|----------:|---------:|-----------:|----------:|
| `map::IndexMap::insert` | 4 | **0** | 2 | **0** |
| `map::IndexMap::get` | 4 | **0** | 3 | **1** |
| records | 227 | 224 | 243 | 243 |
This finishes the `IndexMap` axis of the `PYRE_DYN_INDIRECT` flip cost: the
flip now adds **no** `IndexMap` root at all, down from +14 when the axis was
first enumerated. The remaining `get_index_of` 3 is equal under both gates
(its `Option<usize>` return is two words with no niche — a separate wall).
Flip ledger is now OFF 243 -> ON 224, net -19.
Three of the freed graphs re-block on `FnOnce::call_once` (5 -> 7) inside
`callback_free_dict_op`, and the rest on `Wtf8::as_str` (34 -> 37).
check.py green on all three backends.
Assisted-by: Claude
`dyn_indirect_enabled()` flips from `PYRE_DYN_INDIRECT=1` opt-in to on by default with `PYRE_DYN_INDIRECT=0` as the kill switch, the shape `tuple_per_shape_enabled` and `option_residual_narrow_enabled` already use. `__dyn_call` is not a lowering, it is a placeholder: an unregistered synthetic path that stops whatever graph reaches it. Routing an inline-Field `dyn Trait` call through `CallTarget::Indirect` is what `jtransform.py`'s `indirect_call` does, so the gated side was the faithful one. What kept it gated was the cost of the widening it forces — routing through `Indirect` makes the annotator annotate every family member's body, and `DictStrategy` has ten impls whose `getitem`/`getitem_str`/`delitem` bodies read `IndexMap`. Census at the flip point, same corpus, no re-extract (front-end only): | state | records | biggest wall | |--------------------------|--------:|---------------------| | `PYRE_DYN_INDIRECT=0` | 243 | `__dyn_call` 48 | | default (this commit) | **224** | `Wtf8::as_str` 37 | The flip removes 48 `__dyn_call` records and adds 30 across 7 roots: `Wtf8::as_str` +16, `ctypes::borrow_memory` +5, `Map::collect` +3, `Wtf8Buf::push` +2, `FnOnce::call_once` +2, `AtomicBool::store` +1, `set_async` +1. When that added set was first enumerated it was 44 records across 14 roots, `IndexMap` supplying 14 of them; the preceding slices took `IndexMap` to zero and the rest to the list above. `Wtf8::as_str` is now over half of what is left and is a distinct epic — a fat `&str` return crosses no residual ABI. Verified as exactly the gate: the default census is wall-for-wall identical to the old `PYRE_DYN_INDIRECT=1` run, and the `=0` census is wall-for-wall identical to the old default. The auto-population of `>=2`-impl trait families (`lib.rs`) shares the same predicate and flips with it. That is required, not incidental: minting base/impl subclass classdefs perturbs `pyre_struct_root_names` -> `ensure_session` inheritance-id numbering even off-path, so the kill switch only restores byte-identity if it restores both halves together. check.py green on all three backends. Assisted-by: Claude
…thout closures `dispatch_callable` took its two arms as `impl FnOnce`, and `callback_free_dict_op` took the probe body the same way. Each puts an `FnOnce::call_once` in front of the work it wraps, and a closure has no counterpart in the lifted model, so every graph that reached one stopped there. `dispatch_callable` becomes `classify_callable -> Result<CallableKind, PyError>`: a fieldless two-arm answer that the one production caller, `call_callable_with_mode`, matches on with both arms written out. `callback_free_dict_op` becomes a macro, so the seven probe bodies expand where they are read; each one's early `return` becomes the `if`/`else` value it was standing for. Upstream spells both out at the call site — `ll_dict_lookup`'s fast pass before the paranoia restart (`rordereddict.py:1057`) is not a callable it hands off. Census: `FnOnce::call_once` 7 -> 0. Assisted-by: Claude
… view, and six leaves
Each of these stopped whole graphs at a call the rtyper has no lowering
for. Measured on the same corpus, `PYRE_RTYPER_VERBOSE=1` phaseA:
| wall | recs |
|-------------------------------|-----:|
| `bytecode::CodeUnits::deref` | 88 -> 0 |
| `Wtf8::as_str` | 33 -> 0 |
| `iter::…::Map::collect` | 8 -> 0 |
| `sync::atomic::AtomicBool::store` | 6 -> 0 |
| `Wtf8Buf::push` | 4 -> 0 |
| `rustpython_host_env::ctypes::borrow_memory` | 4 -> 0 |
| `Wtf8Buf::push_wtf8` | 3 -> 0 |
| `majit_ir::eval_breaker_word::set_async` | 2 -> 0 |
**`report_stack_underflow`** was reached from `PyFrame::pop` on the branch
it never takes, so its instruction-window formatting — a `Vec<String>`
built by walking the `CodeUnits` deref — was lowered into all 88 graphs
that pop a value. It moves to module scope as a `#[dont_look_inside]`
`&PyFrame` reporter; the `()` result is what crosses the boundary and the
panic still diverges.
**`Wtf8::as_str`** returns `Result<&str, Utf8Error>`, and the two-word `Ok`
payload crosses no residual boundary, so neither marking nor the existing
`is_string_to_str_identity` fold (which needs a destination that strips to
`str`) could take it. Split the answer instead: `w_str_first_surrogate`
scans once behind `#[dont_look_inside]` and returns the offending code
point index — or `-1` — in one machine word, `w_str_is_utf8` reads the
cached `is_ascii` counts first (`unicodeobject.py:1245`; one byte per code
point is UTF-8 by construction) and only then scans, and
`as_str_unchecked` names the view the caller has already earned. That
last one is the same string-value identity `String::as_str` is, so the
fold's leaf set gains it. `str_utf8_w` now reports its
`UnicodeEncodeError` offset from the same scan rather than a second
`code_points().position()` walk.
**The string builders** get named leaves, matching how upstream reaches
them: `w_str_concat` (`ll_strconcat`, `rstr.py:425-428`, already
`@jit.elidable @jit.oopspec('stroruni.concat')` upstream — not traced
through there either), `w_str_from_codepoint`
(`rutf8.unichr_as_utf8`, `rutf8.py:40`), and `w_str_slice_codepoints`
(`_unicode_sliced`, `unicodeobject.py:1373-1379`). `_warnings::show_warning`
assembles its line the same way and then writes it to `sys.stderr`, so the
whole printer residualises.
**`(0..n).map(|i| shadow_stack_get(base + i)).collect()`** appears five
times across the reroot-after-`next` paths plus twice in
`temporarily_as_objects`; each becomes the index loop it already was.
**`w_type_set_uses_object_getattribute`** joins its already-residual
`_setattr` twin — the read side folds to the field it aliases, a store has
no such fold — through the same `bool`-normalising C-ABI bridge. The
eval-breaker's async bit is armed and cleared through
`arm_async_eval_breaker` / `disarm_async_eval_breaker`, since an OS signal
handler writes the same word. `cdata_bytes_object` boxes the ctypes view
inside the boundary, so a one-word `Option<PyObjectRef>` crosses instead of
the `&[u8]` its caller was going to wrap anyway.
phaseA 288 -> 196. That is 92, not the 148 the cleared walls held: a graph
that no longer stops at one of these walks on to the next one, so part of
the count relocates rather than disappears (`core::mem::drop` 2 -> 14,
`from_raw_parts` 2 -> 9, `saturating_sub` 0 -> 6).
check.py green on dynasm, cranelift and wasm.
Assisted-by: Claude
… map Routing `dyn Trait` calls through `CallTarget::Indirect` became the default earlier in this branch, so the codewriter now emits `vtable_method_ptr/rd>i` into real jitcodes. `build_inline_call_only_bh_builder` hand-curates its registered set, and that byte was not in it, so the two guards over the production blackhole failed: - `production_bh_builder_covers_every_build_emitted_opname` — the byte is in `build_emitted_insns()` (pyre's `asm.insns`) with no dispatch entry. - `production_bh_builder_overlay_only_gap_snapshot` — the opname left the never-emitted list, which is the drift that snapshot exists to report. Register the key and drop it from the snapshot. `setup_insns(asm.insns)` (`blackhole.py:58-59`) resolves every opname the assembler emitted, so a byte outside the curated set reaches the unwired-opcode placeholder rather than a handler. The handler it binds stays `handler_vtable_method_ptr_unimplemented`. No layer consumes this op: there is no backend lowering (`assembler.rs`, "backend lowering of the actual vtable slot read is not yet implemented"), the walker answers `DispatchError::UnsupportedOpname`, and `PyreVtableMethodDescr` holds `(trait_root, method_name)` as strings with nothing that resolves them to an address. The walker's abort is what keeps the handler unreachable — a trace meeting the op never compiles, so no resume lands past it. What the entry buys is that if one ever does, the panic names the missing op instead of reporting an unwired byte. Its doc comment said pyre's hot path does not emit this pattern; that is no longer true and is rewritten. Assisted-by: Claude
youknowone
force-pushed
the
rewrite-tracer
branch
from
July 29, 2026 07:21
8daf008 to
2d87f58
Compare
youknowone
added a commit
that referenced
this pull request
Jul 30, 2026
`dict_keys_equal` answered `std::ptr::eq` before it consulted
`eq_error_pending()`, so the "no further comparison after a raise" rule the
function documents did not cover the identity shortcut.
That is reachable. A bucket can hold both a key whose `__eq__` raises and the
incoming key's own object; probed in that order, the raise poisons the probe
and the identity hit then still reports a match. `IndexMap::insert` therefore
REPLACES instead of appending, and `w_module_dict_store_inner_checked`'s undo —
`dict_entries_pop_last`, which assumes the failed probe appended — drops the
dict's unrelated last entry while leaving the replaced value overwritten.
Against CPython 3.14 on a module dict, storing under such a key:
q 'quiet-new' (CPython: 'quiet-old' — the store must not land)
lost ['run'] (a module-level function deleted from globals())
gained -1
`ll_dict_lookup` propagates at the first raising comparison
(`rordereddict.py:1055`), so no later key can be found however it would have
compared. Move the pending-error test above the identity shortcut, which
restores that: the probe matches nothing, `insert` appends, and the existing
`pop_last` undo is correct again.
`module_dict_invariants.py` gains the case; it fails on both backends before
this change and passes on CPython, dynasm and cranelift after.
Reported by the Codex parity review on #873 as a pre-existing mismatch. Its
stated mechanism — a later user comparison returning true — is not the live
one; `eq_error_pending()` already blocks that. The identity shortcut sitting
above the guard is what was left.
Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Twenty slices against the rtyper census, plus the two front-end changes they
depended on. Each slice moves a call the rtyper has no lowering for behind a
named boundary, so the graph that reached it keeps going.
A twenty-first commit follows from the dyn-indirect default below: the
codewriter now emits
vtable_method_ptr/rd>iinto real jitcodes, so theproduction blackhole's hand-curated insns map has to span that byte. Its
handler stays
unimplemented!()— no layer consumes the op (no backendlowering, the walker answers
UnsupportedOpname, andPyreVtableMethodDescrholds
(trait_root, method_name)as strings with nothing resolving them to anaddress). The walker's abort is what keeps it unreachable; the registration
only means a resume landing there names the missing op instead of reporting an
unwired byte.
What is in here
Residual-boundary slices (pyre) — most of the branch. The recurring shapes:
try_eq_wsplits intohas_eq_w_hook+eq_w_hooked;IdentityDictStrategy::getitemgets its ownlookup leaf; the module-dict entries probe residualizes instead of its router;
the checked module-dict probe/store and the three leaves on the delete path
follow.
SYS_MODULESregistry read,both entry-table probes, the
_warningsstate namespace, and the fivetyped-storage promotions.
hash_str's pointer/length movesinside the boundary; the optional generator-throw arguments are indexed
rather than
slice::get'd; the receiver-discounted argument count is spelledwithout
saturating_sub; the extended-slice walks are indexed.Front-end (majit).
wrapping_{add,sub,mul}folds on the unsigned word bank too.usizeserializes as
{"UInt": "Usize"}, whichtyref_literal_int_atomdoes not seeat all, so an
Int-only test silently declined every unsigned counter. It isthe same machine op under a different rtyper dispatch —
rint.py'sopprefixmakes it
uint_add, andjtransform.py:1608-1610renamesuint_{add,sub,mul}straight back toint_{add,sub,mul}for the JIT.dyn-indirectrouting is now the default.__dyn_callis not a lowering,it is a placeholder: an unregistered synthetic path that stops whatever graph
reaches it. Routing through
CallTarget::Indirectis whatjtransform.py'sindirect_calldoes.PYRE_DYN_INDIRECT=0is the kill switch.mergeinputargsUnionErrornow carriessource_linescontext and quotes the block's operations.
The last commit clears eight walls at once. The largest is
report_stack_underflow— a cold panic reporter reached from a branchPyFrame::popnever takes, whose instruction-window formatting was thereforelowered into every graph that pops a value.
Wtf8::as_strcloses bydecomposition rather than by the cached-flag epic it looked like it needed:
w_str_is_ascii(len == byte_len) already implies valid UTF-8, so a one-wordw_str_first_surrogatescan plus anas_str_uncheckedview that folds to itsreceiver is enough. That commit message carries the per-wall table.
Measurement
Warning
These figures were taken before this branch was rebased onto
#814. Theabsolute count moves with the traced-source tree, so they are reported against
their exact base rather than restated against the new one. The post-rebase
tree has not been re-censused.
66dada91c8→ phaseA 2882e27e62601) → phaseA 196Nine walls went to zero in the final commit alone:
CodeUnits::deref88,Wtf8::as_str33,Map::collect8,FnOnce::call_once7,AtomicBool::store6,
Wtf8Buf::push4,ctypes::borrow_memory4,push_wtf83,set_async2.That is −92, not the −148 those walls held. A graph that no longer stops at a
cleared wall does not become clean — it walks on to the next wall down its own
chain, so part of the count relocates rather than disappears
(
core::mem::drop2→14,from_raw_parts2→9,saturating_sub0→6). Budget awall slice at roughly 60% of the records it holds.
Rebase note
#814independently widened the sameVecindex gate this branch widened(
ValueType::Int→Int | Unsignedinmir.rs) — the one conflict. Itresolved to the helper form so the predicate is not spelled twice, and that
commit is reworded to state what it now does: extract
vec_index_type_is_scalarand pin both banks with a unit test that actually runs. The original trap was
caught only by the
#[ignore]d real-LLBC anchorvec_index_mut_fill_user_function_args_real, which needs the 440MB corpus anddoes not run in CI.
The branch was then replayed again onto
78ebd2d842(#865), picking up #869,#838 and #871 as well. That is the base these numbers and checks describe.
Verification
On
78ebd2d842, after a full LLBC re-extract (the newer base touches 17traced-crate files, so the corpus had to be rebuilt for the census-facing
tests to mean anything):
check.py— dynasm 332/332, cranelift 332/332, wasm 329/329.cargo test -p pyre-jit-trace -p majit-metainterp -p majit-translate --features dynasm— 4952 passed, 0 failed.Two macOS-only failures seen on an earlier push (
synth/sre_pattern_methodsand
test.test_strtod, both a silentexit -10under dynasm) were confirmedflaky: re-running those jobs on the identical commit, with no code change,
passed both. They do not reproduce locally (18 runs) and
maindoes not showthem.
🤖 Generated with Claude Code