majit: split state-field resume frames by liveness and give up on multi-frame bridges - #773
Merged
Conversation
|
Warning Review limit reached
Next review available in: 54 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 (11)
WalkthroughBridge resume reconstruction is centralized in ChangesJIT bridge pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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: 18e772a46b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5328
to
+5331
| if self.dispatch_jitcode.is_some() | ||
| && resume_data_result | ||
| .as_ref() | ||
| .is_some_and(|r| r.frames.len() > 1) |
There was a problem hiding this comment.
For a state-field #[jit_interp] driver whose guard fails while a #[jit_inline] callee is active, this condition aborts every bridge attempt solely because the resume record contains multiple frames. The guard therefore continues deoptimizing through the blackhole and can repeatedly retrigger tracing without ever compiling the hot failure path. Restore and seed each frame—including the innermost resume coordinate—rather than permanently disabling bridges for this common inlining case.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 9884ea5). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03db0628ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2389
to
+2391
| let __fvc = majit_ir::resumedata::get_frame_value_count_fn(); | ||
| let __fvc_ref: ::std::option::Option<&dyn Fn(i32, i32) -> usize> = | ||
| __fvc.as_ref().map(|f| f as &dyn Fn(i32, i32) -> usize); |
There was a problem hiding this comment.
When two independent #[jit_interp] drivers are installed on the same thread, register_dispatch_jitcode replaces the single STATE_FIELD_FVC TLS payload with the most recently installed driver's registry and liveness. This newly fetched global callback therefore decodes an earlier driver's rd_numb using the later driver's JitCodes, producing incorrect frame lengths that can turn values into frame headers and either reject or mis-seed the bridge. Pass the owning driver's registry/liveness into this decode instead of consulting shared TLS state.
AGENTS.md reference: AGENTS.md:L148-L162
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: 5
🤖 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/jitdriver.rs`:
- Around line 2035-2047: Prune loop-header green entries whenever their compiled
loop is removed or invalidated: update the cleanup paths remove_compiled_loop,
invalidate_loop, and mark_all_loops_for_release to remove the corresponding keys
from the loop_header_greens map, mirroring loop_header_pcs management. Also
replace the unnecessary loop_close_greens.clone() in the close-loop recording
block with a move, since the binding is not used afterward.
- Around line 392-412: Remove the temporary bridge_only_allows diagnostic and
its gating from should_bridge, restoring bridge formation to the existing
behavior. If the helper is intentionally retained instead, update its parsing so
an all-invalid MAJIT_BRIDGE_ONLY value fails loudly or falls back to allowing
all bridges rather than producing an empty allowlist; remove the temporary
diagnostic labeling before merge.
In `@majit/majit-metainterp/src/optimizeopt/optimizer.rs`:
- Around line 3173-3188: Add regression tests covering the optimizer branch
around `building_bridge`: assert bridge optimization succeeds with
`exported_loop_state == None`, while normal-loop optimization still returns
`InvalidLoop`. In both successful and failing paths, also assert that
`building_bridge` is restored to its original value after optimization.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 8206-8227: Evict stale loop metadata when compiled loops are
retired: update remove_compiled_loop and the memmgr eviction path in
try_to_free_some_loops to remove the owning key from both loop_header_greens and
the existing loop_header_pcs map, alongside the other compiled-loop cleanup.
Preserve compiled_key_for_greens behavior while ensuring each retired loop’s
entries are removed from both maps.
In `@majit/majit-metainterp/src/resume_box_reader.rs`:
- Around line 747-753: Replace the debug-only arity checks with unconditional
assert_eq! checks in both VStr/VUniConcatInfo at
majit/majit-metainterp/src/resume_box_reader.rs#L747-L753 and VStr/VUniSliceInfo
at majit/majit-metainterp/src/resume_box_reader.rs#L795-L802, preserving the
expected lengths of 2 and 3 respectively before indexing fieldnums.
🪄 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: 4caa61cb-a31d-4a21-aa80-11698f2079fe
📒 Files selected for processing (11)
majit/majit-ir/src/descr.rsmajit/majit-macros/src/jit_interp/codegen_state.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume_box_reader.rsmajit/majit-metainterp/src/trace_ctx.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/state.rs
Comment on lines
+392
to
+412
| /// TEMP DIAGNOSTIC (remove before commit): `MAJIT_BRIDGE_ONLY=a,b,c` restricts | ||
| /// bridge formation to the listed guard `fail_index` values, so a miscompiling | ||
| /// bridge can be bisected out of a run. Unset = allow all. | ||
| fn bridge_only_allows(fail_index: u32) -> bool { | ||
| static LIST: std::sync::OnceLock<Option<Vec<u32>>> = std::sync::OnceLock::new(); | ||
| let list = LIST.get_or_init(|| { | ||
| std::env::var("MAJIT_BRIDGE_ONLY").ok().map(|v| { | ||
| v.split(',') | ||
| .filter_map(|t| t.trim().parse::<u32>().ok()) | ||
| .collect() | ||
| }) | ||
| }); | ||
| let ok = match list { | ||
| None => true, | ||
| Some(allowed) => allowed.contains(&fail_index), | ||
| }; | ||
| if crate::bridge_debug_enabled() { | ||
| eprintln!("[bridgeONLY] fail_index={fail_index} allowed={ok}"); | ||
| } | ||
| ok | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Self-declared "remove before commit" diagnostic is still in the diff.
Two concerns:
- The doc comment marks this as temporary, but
bridge_only_allowsis wired intoshould_bridgeat Lines 3481-3482 and 5815-5816, so it ships as a live gate on bridge formation. filter_map(... .ok())silently drops unparsable tokens, soMAJIT_BRIDGE_ONLY=oopsyieldsSome(vec![])and suppresses every bridge — the inverse of the documented "empty/unset = allow all". A typo therefore silently disables bridge formation for the whole run.
Either drop the helper before merge, or keep it and make an all-invalid list fail loud / fall back to allow-all. Want me to open an issue to track removal?
🛡️ Fail loud instead of silently suppressing all bridges
let list = LIST.get_or_init(|| {
std::env::var("MAJIT_BRIDGE_ONLY").ok().map(|v| {
- v.split(',')
- .filter_map(|t| t.trim().parse::<u32>().ok())
- .collect()
+ v.split(',')
+ .map(|t| t.trim())
+ .filter(|t| !t.is_empty())
+ .map(|t| {
+ t.parse::<u32>().unwrap_or_else(|_| {
+ panic!("MAJIT_BRIDGE_ONLY: {t:?} is not a valid fail_index")
+ })
+ })
+ .collect()
})
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// TEMP DIAGNOSTIC (remove before commit): `MAJIT_BRIDGE_ONLY=a,b,c` restricts | |
| /// bridge formation to the listed guard `fail_index` values, so a miscompiling | |
| /// bridge can be bisected out of a run. Unset = allow all. | |
| fn bridge_only_allows(fail_index: u32) -> bool { | |
| static LIST: std::sync::OnceLock<Option<Vec<u32>>> = std::sync::OnceLock::new(); | |
| let list = LIST.get_or_init(|| { | |
| std::env::var("MAJIT_BRIDGE_ONLY").ok().map(|v| { | |
| v.split(',') | |
| .filter_map(|t| t.trim().parse::<u32>().ok()) | |
| .collect() | |
| }) | |
| }); | |
| let ok = match list { | |
| None => true, | |
| Some(allowed) => allowed.contains(&fail_index), | |
| }; | |
| if crate::bridge_debug_enabled() { | |
| eprintln!("[bridgeONLY] fail_index={fail_index} allowed={ok}"); | |
| } | |
| ok | |
| } | |
| /// TEMP DIAGNOSTIC (remove before commit): `MAJIT_BRIDGE_ONLY=a,b,c` restricts | |
| /// bridge formation to the listed guard `fail_index` values, so a miscompiling | |
| /// bridge can be bisected out of a run. Unset = allow all. | |
| fn bridge_only_allows(fail_index: u32) -> bool { | |
| static LIST: std::sync::OnceLock<Option<Vec<u32>>> = std::sync::OnceLock::new(); | |
| let list = LIST.get_or_init(|| { | |
| std::env::var("MAJIT_BRIDGE_ONLY").ok().map(|v| { | |
| v.split(',') | |
| .map(|t| t.trim()) | |
| .filter(|t| !t.is_empty()) | |
| .map(|t| { | |
| t.parse::<u32>().unwrap_or_else(|_| { | |
| panic!("MAJIT_BRIDGE_ONLY: {t:?} is not a valid fail_index") | |
| }) | |
| }) | |
| .collect() | |
| }) | |
| }); | |
| let ok = match list { | |
| None => true, | |
| Some(allowed) => allowed.contains(&fail_index), | |
| }; | |
| if crate::bridge_debug_enabled() { | |
| eprintln!("[bridgeONLY] fail_index={fail_index} allowed={ok}"); | |
| } | |
| ok | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/jitdriver.rs` around lines 392 - 412, Remove the
temporary bridge_only_allows diagnostic and its gating from should_bridge,
restoring bridge formation to the existing behavior. If the helper is
intentionally retained instead, update its parsing so an all-invalid
MAJIT_BRIDGE_ONLY value fails loudly or falls back to allowing all bridges
rather than producing an empty allowlist; remove the temporary diagnostic
labeling before merge.
Comment on lines
+2035
to
+2047
| // Key the loop by the greens it closed on, so a bridge reaching | ||
| // this merge point resolves THIS loop's procedure token. Only | ||
| // `last_compiled_key` is registered: on a cross-loop cut that | ||
| // is the inner loop the close point belongs to, whereas | ||
| // `loop_green_key` is the outer trace-start key. | ||
| if let Some(greens) = loop_close_greens.clone() { | ||
| if crate::closedbg_enabled() { | ||
| eprintln!("@@@CLOSE LOOP-GREENS key={} greens={greens:?}", k as i64); | ||
| } | ||
| self.meta.record_loop_header_greens(k, greens); | ||
| } else if crate::closedbg_enabled() { | ||
| eprintln!("@@@CLOSE LOOP-GREENS key={} greens=NONE", k as i64); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
loop_header_greens is never pruned, and the reverse lookup scans it linearly.
record_loop_header_greens inserts one (Vec, Vec, Vec) per compiled key, but none of remove_compiled_loop, invalidate_loop, or mark_all_loops_for_release clear this map — they only touch compiled_loops. Entries therefore accumulate for the process lifetime, and compiled_key_for_greens (pyjitpl.rs:8221-8226) walks every one of them on each bridge close, filtering dead keys via has_compiled_targets at read time. Lookups stay correct; the cost is steady growth plus an O(loops) scan on the bridge close path.
Consider evicting alongside the compiled_loops removal paths, mirroring how loop_header_pcs is managed.
Minor: loop_close_greens.clone() at Line 2040 can move instead — the binding is not read afterwards.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/jitdriver.rs` around lines 2035 - 2047, Prune
loop-header green entries whenever their compiled loop is removed or
invalidated: update the cleanup paths remove_compiled_loop, invalidate_loop, and
mark_all_loops_for_release to remove the corresponding keys from the
loop_header_greens map, mirroring loop_header_pcs management. Also replace the
unnecessary loop_close_greens.clone() in the close-loop recording block with a
move, since the binding is not used afterward.
Comment on lines
+8206
to
+8227
| /// Record the merge-point green constants a compiled loop was traced | ||
| /// under, so `compiled_key_for_greens` can invert them back to its key. | ||
| pub fn record_loop_header_greens( | ||
| &mut self, | ||
| green_key: u64, | ||
| greens: (Vec<i64>, Vec<i64>, Vec<i64>), | ||
| ) { | ||
| self.loop_header_greens.insert(green_key, greens); | ||
| } | ||
|
|
||
| /// pyjitpl.py:3005-3006 `ptoken = self.get_procedure_token(greenboxes)` / | ||
| /// `has_compiled_targets(ptoken)`: the green key of the compiled loop | ||
| /// whose header greens equal `greens`, or `None` when no loop lives at | ||
| /// those greens. Compared element-wise like pyjitpl.py:3912 | ||
| /// `same_greenkey`, over every green rather than the pc alone. | ||
| pub fn compiled_key_for_greens(&self, greens: &(Vec<i64>, Vec<i64>, Vec<i64>)) -> Option<u64> { | ||
| self.loop_header_greens | ||
| .iter() | ||
| .find(|(key, header)| *header == greens && self.has_compiled_targets(**key)) | ||
| .map(|(key, _)| *key) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
loop_header_greens entries are never evicted.
record_loop_header_greens only ever inserts; nothing removes an entry when its owning loop is retired. remove_compiled_loop clears compiled_loops/pending_preamble_tokens but not loop_header_greens (or the pre-existing loop_header_pcs), and try_to_free_some_loops's memmgr eviction path likewise never prunes either map. compiled_key_for_greens masks the functional symptom via the has_compiled_targets filter, but the map itself grows without bound across the process lifetime as loops recompile/evict — a slow leak in long-running JIT sessions.
Consider clearing the corresponding entry in remove_compiled_loop and in the memmgr eviction path in try_to_free_some_loops (same fix applies to the pre-existing loop_header_pcs).
♻️ Proposed fix
pub fn remove_compiled_loop(&mut self, green_key: u64) {
self.compiled_loops.swap_remove(&green_key);
self.pending_preamble_tokens.swap_remove(&green_key);
+ self.loop_header_pcs.swap_remove(&green_key);
+ self.loop_header_greens.swap_remove(&green_key);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 8206 - 8227, Evict stale
loop metadata when compiled loops are retired: update remove_compiled_loop and
the memmgr eviction path in try_to_free_some_loops to remove the owning key from
both loop_header_greens and the existing loop_header_pcs map, alongside the
other compiled-loop cleanup. Preserve compiled_key_for_greens behavior while
ensuring each retired loop’s entries are removed from both maps.
Comment on lines
+747
to
+753
| debug_assert_eq!( | ||
| fieldnums.len(), | ||
| 2, | ||
| "VStr/VUniConcatInfo must have exactly 2 fieldnums (left, right)" | ||
| ); | ||
| let left = decode_fieldnum(ctx, fieldnums[0], rd_virtuals, resume_data, cache); | ||
| let right = decode_fieldnum(ctx, fieldnums[1], rd_virtuals, resume_data, cache); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Arity preconditions on fieldnums use debug_assert_eq!, so release builds index out of bounds instead of reporting the violation. Both sites assert the expected fieldnums length and then index it directly; with debug_assert_eq! compiled out, a malformed resume stream panics with a bare slice-index message. Every other arity check in this module fails loud unconditionally — VRawSliceInfo at Line 640 uses assert!, matching upstream's active assert len(self.fieldnums) == 1 (resume.py:724).
majit/majit-metainterp/src/resume_box_reader.rs#L747-L753: change theVStr/VUniConcatInfocheck toassert_eq!so the 2-fieldnum precondition holds beforefieldnums[0]/fieldnums[1].majit/majit-metainterp/src/resume_box_reader.rs#L795-L802: change theVStr/VUniSliceInfocheck toassert_eq!so the 3-fieldnum precondition holds beforefieldnums[0..2].
📍 Affects 1 file
majit/majit-metainterp/src/resume_box_reader.rs#L747-L753(this comment)majit/majit-metainterp/src/resume_box_reader.rs#L795-L802
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/resume_box_reader.rs` around lines 747 - 753,
Replace the debug-only arity checks with unconditional assert_eq! checks in both
VStr/VUniConcatInfo at majit/majit-metainterp/src/resume_box_reader.rs#L747-L753
and VStr/VUniSliceInfo at
majit/majit-metainterp/src/resume_box_reader.rs#L795-L802, preserving the
expected lengths of 2 and 3 respectively before indexing fieldnums.
Move the trace-emit (box-reader) flavour of the resume-data reader out of pyre-jit-trace/state.rs into a new majit-metainterp/resume_box_reader.rs so every #[jit_interp] consumer can materialize a guard's virtuals and replay its deferred heap writes at bridge entry, not only the hand-written PyreJitState. - Add majit-metainterp/src/resume_box_reader.rs (BridgeVirtualCache, materialize_bridge_virtual, replay_pending_fields, emit_pending_field_op, decode_fieldnum, rebuilt_value_to_opref, default_bridge_array_descr) and re-export it from majit-metainterp/src/lib.rs. - pyre-jit-trace/src/state.rs: drop the local BridgeVirtualCache / materialize_virtual_from_rd / emit_stroruni_oopspec_call definitions and use the shared ones. - jit_interp macro setup_bridge_sym (codegen_state.rs): call replay_pending_fields once, and materialize_bridge_virtual for RebuiltValue::Virtual int/ref scalars that previously fell through unseeded (bridge read size>chain and dereferenced a NULL node head). Add env-gated (AHEUI_BRIDGE_DIAG) bridge-seed diagnostics. - Hoist field_descr_from_parent_by_offset into majit-ir/src/descr.rs; pyre-jit-trace make_field_descr_with_parent delegates to it. Assisted-by: Claude
optimize_bridge (unroll.py:193) runs propagate_all_forward without building the short-preamble export preview; virtual-state matching is deferred to jump_to_existing_trace (unroll.py:207), which catches VirtualStatesCantMatch and falls back to jump_to_preamble (unroll.py:209-210, 238-242). pyre folds the preview export into the shared optimize_with_constants_and_inputs_at, so a preview mismatch escaped as InvalidLoop and discarded the bridge. Add an Optimizer.building_bridge flag, set around the bridge call in optimize_bridge. When it is set, a preview make_inputargs_and_virtuals mismatch leaves exported_loop_state = None instead of returning InvalidLoop; the loop and peeled-loop paths keep the fatal behavior. Assisted-by: Claude
…ti-frame bridges `rebuild_from_resumedata` in the `#[jit_interp]` codegen passed `frame_value_count = None` to `rebuild_from_numbering`, so frame 0 consumed every remaining item in `rd_numb`. The writer side (`build_state_field_snapshot`) emits one section per MIFrame, outermost to innermost, each stamping its own absolute jitcode index and sized by its own liveness, so a guard failing inside a `#[jit_inline]` callee publishes a multi-frame stream whose second section's [jitcode_index, pc, py_pc] header and values were folded into frame 0. `setup_bridge_sym` then saw `reg_indices.total_len() != frame.values.len()` and returned without seeding, while the bridge was still compiled with `create_sym`'s loop-namespace InputArgs. On aheui pi.jinseo the guard at pc=613 decoded 14 values against 6 live root registers, and the resulting bridge reached the optimizer with a Ref operand on an int operation (`getintbound_handle: expected 'i'-typed operand, got Ref` in OptIntBounds::propagate_forward via close_bridge). Decode through the liveness splitter that `register_dispatch_jitcode` already installs (`install_state_field_fvc`), matching the other compile-time decoders of the same `rd_numb`, and give up on bridge formation when the decoded stream carries more than one frame: only the root frame's identity registers are seeded and the bridge re-enters at the root dispatch coordinate, which would re-run an inlined helper that already committed part of its effect. The giveup is scoped to state-field drivers by the `dispatch_jitcode` probe. aheui pi.jinseo: length-mismatch declines 2 -> 0, two multi-frame giveups, `getintbound_handle` panic no longer reached. logo/99bottles/99dan/quine stay byte-identical between --no-jit and --jit. Assisted-by: Claude
Restricts bridge formation to a comma-separated list of guard `fail_index`
values, so a run that miscompiles only when some bridge is built can be
bisected down to the individual guard. Unset keeps every bridge, so the
knob is inert by default; it sits next to the existing MAJIT_NO_BRIDGE
diagnostic and gates the same `should_bridge` predicate at both of its
sites. Logs each decision under MAJIT_BRIDGE_DEBUG.
On aheui pi.jinseo with GC suppressed this isolates two independent
single-bridge miscompiles: fail_index=50 alone diverges from the
interpreter at output byte 806, and fail_index=17 alone at byte 2195,
while {2,3,6,7} and {9,12} stay byte-identical.
Assisted-by: Claude
… greens A bridge closing on a merge point took its JUMP target from `current_trace_green_key().unwrap_or(bridge_key)`, i.e. the green key of the loop the failing guard belonged to. When the trace closed on an inner merge point (the cross-loop cut) that key names a different loop than the one reached. `pc` is a green and is baked into each compiled loop as a constant, so the JUMP entered a loop compiled for another pc while carrying the reached pc's reds. pyjitpl.py:3005-3007 reads `ptoken = self.get_procedure_token(greenboxes)` off the greens of the merge point just reached, and pyjitpl.py:3012-3060 keeps tracing (or compiles a loop) when that greenkey has no procedure token — it never substitutes another loop's token. The greens of the closing merge point are now recorded on the trace ctx by both close paths, each compiled loop registers the greens it closed on, and the bridge resolves its target by inverting those greens back to a key. When no compiled loop holds them, `has_targets` is false and the existing `compile_loop` fall-through (pyjitpl.py:3014-3017) runs. `update_tracing_green_key`, which the previous comment named as the retargeting mechanism, has no call sites. On aheui pi.jinseo.aheui with collection disabled, the guards whose bridges miscompiled (fail_index 50 at output byte 806, 17 at 2195) are now byte-identical to the naive interpreter, as is the full bridge set; previously it diverged at byte 806. check.py: dynasm 308/308, cranelift 308/308, wasm 305/305. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 26, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 26, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 27, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 27, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 28, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 28, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 28, 2026
…GE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 28, 2026
…GE_ONLY parsing (#787) * majit: evict per-loop side tables on retirement and harden MAJIT_BRIDGE_ONLY parsing Review findings from PR #773. `loop_header_pcs` and the new `loop_header_greens` were inserted on compile but never removed, so both outlived `compiled_loops` and grew without bound. `remove_compiled_loop` and the memmgr eviction path in `try_to_free_some_loops` now drop the retired key from both. `compiled_key_for_greens` already required `has_compiled_targets`, so a leftover entry could not resolve a bridge onto a retired loop; this is the growth fix, not a targeting fix. `MAJIT_BRIDGE_ONLY` parsed its list with `filter_map(.. .ok())`, so a single unparsable entry was dropped and `MAJIT_BRIDGE_ONLY=oops` yielded an empty allowlist — which suppresses every bridge, the inverse of the documented unset-means-all default, with no diagnostic. An unparsable entry now panics. The two `fieldnums` arity checks in `resume_box_reader.rs` are unconditional `assert_eq!` rather than `debug_assert_eq!`; release builds indexed past the end and panicked without the message. cargo test -p majit-metainterp --features dynasm: 1501 passed, 0 failed. Assisted-by: Claude * majit: allocate a headerless jitcode `new` from the interpreter's own pool `JitCodeMachine::run_one_step`'s `BC_NEW` arm allocated every struct with `std::alloc::alloc_zeroed`, bypassing the GC. blackhole.py:1301-1310 `bhimpl_new` reaches the allocation through `cpu.bh_new(descr)`, so it always lands in the pool the collector that owns the object manages. A descr flagged `headerless` says the interpreter owns the struct in its own collected pool: that is what `headerless_structs` declares, and what compiled code allocates it from, through `call_malloc_nursery_headerless`. A host-heap block there is invisible to that collector. aheui's copying collector range-checks its nursery chunks in `forward_root`, so it neither traces through such an object nor forwards the references hanging off it, and the graph below it is left in from-space for the next collection to reuse. The allocation must not collect. `BC_NEW` runs mid-jitcode with raw object pointers live in the machine's own register bank -- the `getfield` result that the `setfield` after the `new` consumes -- and that bank belongs to no root set; unlike an interpreter-side allocation there is no successor to hand over as a keep root. `GcAllocator::alloc_nursery_headerless_no_collect` carries that requirement, defaulting to the collecting form, which is what a non-moving collector wants. Non-headerless descrs keep `alloc_zeroed` unchanged and `BC_NEW_WITH_VTABLE` is untouched. aheui is the only consumer in the tree that declares `headerless_structs`. python ./pyre/check.py: dynasm 6 failed / 315 passed, cranelift 6 / 315, wasm 3 / 315 -- the same failure set, test for test, as the commit this is built on, confirmed by rerunning it with these three files reverted. The one difference between the two runs is the measured ratio of the pre-existing `const_arg_call_resume` perf-gate failure. Assisted-by: Claude * majit: drop the loop side tables on the bulk eviction paths too `clear_compiled_loops`, `mark_all_loops_for_release` and `invalidate_compiled_trace` removed `compiled_loops` entries without touching `loop_header_pcs` / `loop_header_greens`. `clear_compiled_loops` now clears both maps and `mark_all_loops_for_release` routes through it; `invalidate_compiled_trace` moves to `MetaInterp`, where it drops the side tables of each removed green key. `MAJIT_BRIDGE_ONLY` values naming no index (empty, whitespace, bare commas) produced an empty allowlist that rejected every guard without a diagnostic. Parsing moves to `parse_bridge_only`, which panics in that case. Adds unit tests for the three eviction paths and the four parse cases. Assisted-by: Claude * majit: log the bridge-path preview virtual-state mismatch The `building_bridge` branch that leaves the export empty instead of raising InvalidLoop had no trace. Log it under MAJIT_BRIDGE_DEBUG next to the other `[bridgeB]` lines. Probed with it: the branch does not fire on the aheui corpus (logo/99bottles/99dan/quine/pi.jinseo) or on pyre/bench + pyre/extra_tests. Assisted-by: Claude * jit: name the portal driver's own frame_value_count decoder `pypyjit_driver_descriptor` left `frame_value_count_fn` at None, so jd0's `-live-` decode fell back to the process-global slot in `majit_ir::resumedata`. That slot has two unarbitrated writers — this crate's `ensure_finish_setup` and majit-metainterp's `install_state_field_fvc`, each behind its own `Once` — so the last registration wins, and a decode against the wrong store returns a mistyped count rather than failing. Only `ensure_finish_setup` runs today: pyre's jd1 dispatch body does not lower, so `register_dispatch_jitcode` is skipped and `install_state_field_fvc` is never reached (measured on pyre/bench/{nbody,fib_recursive,int_loop} and an unpackiterable drain). Set the field to `frame_value_count_at`, the same shape jd1 already uses in `unpackiterable_driver_descriptor`, so `active_frame_value_count_fn` resolves both drivers off the driver rather than the global. check.py: dynasm 5/316, cranelift 6/315, wasm 3/315 — the same correctness failures as HEAD, differing only in the const_arg_call_resume perf ratio. Assisted-by: Claude * majit: own the jitcode table and the portal jitcode from the static data `MetaInterpStaticData` gains `jitcodes`, the flat table `resume.py:1051` indexes (`warmspot.py:281-282` installs it there). `register_dispatch_jitcode` publishes its drained worklist into it through `MetaInterp::install_jitcodes`, and the two `resolve_jitcode` closures read it, so `JitDriver`'s own `jitcode_registry` copy is gone. The portal JitCode moves to `JitDriverStaticData::mainjitcode`, which had no writer (`call.py:147`), at the driver's own registered slot (`call.py:46-47 jd.index`). `JitDriver` keeps only that slot index and `dispatch_jitcode()` reads through it, replacing the driver-local `Option<Arc<JitCode>>`. `call.py:148`'s back-pointer has no counterpart: the metainterp-side `JitCode` carries no `jitdriver_sd` slot, only the translate-side one does. aheui logo/99bottles/99dan/quine byte-identical between --jit and --no-jit (logo md5 7fcdbfff0af449c4283c008e3ca317ce); pi.jinseo prefix-identical at 12288 B with 0 FREE/ALLOC-OUTSIDE-CHUNKS; majit-metainterp 1418 passed; aheui-runtime 18 passed. Assisted-by: Claude * majit: pin the export preview's self-match, which keeps building_bridge dead The preview in `optimize_with_constants_and_inputs_at` exports its virtual state from `post_force_args` and re-matches that same list, so every `state[i]` derives from `args[i]` and `make_inputargs_and_virtuals` cannot raise VirtualStatesCantMatch there. Both arms of the `building_bridge` branch are therefore unreachable. Measured: five virtual-carrying fixtures (escaping tuple, escaping instance, aliased list, varying-length array, nested virtual), two of which compile bridges, produce zero hits — as do the aheui corpus and pyre/bench + pyre/extra_tests. Adds `export_state_re_matched_against_its_own_args_cannot_fail`, which fails if the preview stops being a self-match. Upstream matches against a different loop's stored state in `jump_to_existing_trace` (unroll.py:207), so moving to that shape trips the test and flags the branch as newly live. Assisted-by: Claude * jit: resume the build-time liveness buffer instead of forking the pool `Assembler::resuming_build_time_liveness` seeds the runtime codewriter's `all_liveness` with `jitcode_runtime::all_liveness()`, and `AssemblerState::new` seeds the reader-side mirror the same way, so a `publish_state` wholesale replace cannot rewind past the prefix. Every production `publish_state` caller now publishes a buffer carrying it (`Assembler::finished`, `encode_liveness_info`); the hand-built-buffer callers are all `#[cfg(test)]`. With the build-time bytes addressable from `metainterp_sd.liveness_info`, `blackhole_resume_via_rd_numb` drops its `novable` pick between the two pools and reads the one `resume.py:1022` reads, and `build_time_frame_value_count_at` reads it too — only its jitcode-table lookup stays per-driver. The `-live-` operand is 2 bytes, so the pool is capped at 64 KiB; the build-time prefix is 6952 of those bytes and the overflow assert now reports both halves. Adds `assembler_state_resumes_the_build_time_liveness_prefix`: losing the prefix does not fail loudly, it lands a baked offset inside an unrelated runtime triple and returns a mistyped value count. Assisted-by: Claude * jit: seed the runtime assembler's insns from the build-time opcode table `AssemblerState::new` and `Assembler::resuming_build_time_liveness` resumed the build-time `all_liveness` buffer but started `insns` empty. `blackhole.py:55-61` recovers `op_live` as `asm.insns['live/']`, so `MetaInterpStaticData.op_live` stayed at its unset sentinel, `blackhole_control_opcodes()` returned -1, and `can_decode_live_vars` looked for 255 as a marker byte and declined every resume against a build-time jitcode. Both sides seed from `jitcode_runtime::insns_opname_to_byte()`; `publish_state` replaces `asm.insns` wholesale, so seeding one side alone does not hold. Assisted-by: Claude * jit: bind the list-append store helpers and their jitcode shells as fnaddrs The #171 append fold descends `w_list_append` as a sub-jitcode walk, so a guard exit inside that body is numbered against `w_list_append`'s own jitcode and resumed there. The resumed body reaches its per-strategy store — `W_ListObject::object_push`, `IntArray::push`, `FloatArray::push` — each a `residual_call` whose funcptr the codewriter left as a `symbolic_fnaddr_for_path` hash, so the blackhole aborted the frame. The jd1 drain then fell back to the interpreter after `next()` had already produced an item, losing one element per compiled-loop entry (`bench/synth/unpack_drain_star_raise.py` printed 47941 instead of 48000). `fnaddr_for_target`'s `CallTarget::Method` fallback keys on `CallPath::for_impl_method(receiver, name)`, which `register_macro_helper_trace_fnaddr` derives by stripping the leading crate segment, hence the `pyre_object::<Type>::<method>` spelling. `object_push` becomes `pub` because the binding takes its address. `bhimpl_inline_call_*` calls `cpu.bh_call_*(adr2int(jitcode.fnaddr))`, so the `w_list_append` and `w_list_len` jitcode shells are bound too. The symbolic-funcptr decline now names the jitcode and position. Assisted-by: Claude * majit: drop the bridge_only_parse tests orphaned by the MAJIT_BRIDGE_ONLY removal The rebase onto origin/main takes upstream's deletion of `bridge_only_allows` / `parse_bridge_only`; the unit tests for the parser came along with this branch's hardening commits and no longer name anything. Assisted-by: Claude * jit: skip the finish_setup republish when neither writer buffer grew `ensure_finish_setup` runs on every `jitcode_for`, and it built its arguments by cloning `Assembler.insns` and `Assembler.all_liveness` whole. Both are now seeded from the build-time tables, so every call copied the entire opcode and liveness universe and handed it to `finish_setup_if_needed`, which rewrote the same `op_*` ids and rebuilt the same `liveness_info` Arc from it. `assembler.py:29-31` only appends to those two buffers, so equal lengths mean equal contents. `MetaInterpStaticData` now records the `insns` length its cached opcode ids were read off, and `ensure_finish_setup` compares both lengths before taking the snapshot. bench/synth/depth{2,3}_inline_chain_typeflip run 119873 guard failures with identical trace structure on both sides, so the copies landed once per blackhole resume: wall clock was 5.56s/7.38s before this change against 1.95s/2.54s for the same fixtures at origin/main. After it, user+sys CPU over depth{2,3,7} (min of 3) is 1.65s/1.85s/3.61s here against 1.78s/2.47s/4.50s at origin/main. Assisted-by: Claude * jit: census how many build-time Field descrs converge with the get_field_descr cache `PYRE_FIELD_IDENTITY_CENSUS=1` walks `all_descrs()` at process exit and reports, per `BhDescr::Field`, whether the `DescrRef` `make_descr_from_bh` produces is the same `Arc` `descr.py:218-239 get_field_descr` holds for that `(STRUCT, fieldname)` key. `effectinfo.py:465-547 compute_bitstrings` partitions descrs by object identity, so carrying `EffectInfo`'s raw `_*_descrs_*` sets across `descrs.bin` only means anything if each rehydrated member lands on the descr the trace itself caches. Today the census reports 422 Field slots, 325 keyed, 0 converging: the `_cache_size[key].all_fielddescrs()` list and `_cache_field[key][name]` are separate mints, and `W_ListObject`'s fields carry dot-qualified build-time names against bare runtime keys. Also drops the trailing blank line rustfmt flagged in jitdriver.rs. Assisted-by: Claude * jit: classify the field-descr identity census by miss reason and by reader The census compared one resolution against `_cache_field` and reported a single converged count. Two resolutions exist: `field_descr_ref_from_bh` (`pyjitpl/dispatch.rs`), which reads `_cache_field[STRUCT][fieldname]` and is the Arc baked into recorded getfield/setfield ops, and `field_descr_from_bh_field` (`pyre-jit-trace/src/descr.rs`), which walks `_cache_size[STRUCT].all_fielddescrs()` and fills the build-time descr pool. Export the former and report both against `_cache_field`, plus their agreement with each other and how often the pool Arc came from `all_fielddescrs()`. Misses are split into `no _cache_size[STRUCT]` / `no _cache_field[STRUCT]` / `name not in _cache_field[STRUCT]` / `different Arc`, and the samples carry the owner, `index_in_parent`, the parent's `all_fielddescrs` length and the `_cache_field` key set. On `append_hot.py` this reports 422 Field slots, 325 keyed: pool converges 124, walker 274, pool==walker 124/325, with 42 name misses whose `_cache_field` keys are inner-struct field names (`block`, `len`) registered under the outer struct key. Assisted-by: Claude * jit: mint every field descr through GcCache::get_field_descr `SimpleFieldDescr`, `SimpleFieldDescrSpec` and `BhFieldSpec` gain `field_key` — `descr.py:227`'s `fieldname` cache key, kept separate from the display `name` (`'%s.%s' % (STRUCT._name, fieldname)`). The key was previously recovered by `rsplit_once('.')` on the concatenated name, which turned `int_items.len` into `len`. `make_simple_descr_group_keyed_with_headerless` and `build_object_descr_group_with_def_path` now obtain their fields from `GcCache::get_field_descr` instead of minting fresh Arcs inside `Arc::new_cyclic`, and the walker's `field_descr_ref_from_bh` name-miss branch routes through the same cache-or-mint. `get_field_descr` takes `index` / `virtualizable`; `SimpleFieldDescr::parent_descr` becomes interior-mutable so a later `register_keyed_size` can re-point it. `register_keyed_field` is first-write-wins. `PyreObjectDescrGroup` carries its own field list instead of indexing `size_descr.all_fielddescrs()`, which is positional by `index_in_parent` (`heaptracker.py:76-101 get_fielddescr_index_in`) and need not agree with the pyre static table's order. `bh_all_field_specs_for_struct_into` flattens inline sub-structs with the root owner, a dotted `field_key` and an absolute offset. Field-identity census on a list-append workload: pool 124/325 -> 320/323 resolving to the `_cache_field` Arc, walker 274/325 -> 319/323. Assisted-by: Claude * jit: back metainterp_sd.all_descrs with the process-wide descr registry `descr_index` is stamped off the process-global `GcCache` (`descr.py:28 v.descr_index = len(all_descrs)`), but `all_descrs` was a per-`MetaInterpStaticData` field. pyre carries two of those objects — the tracing walker's thread-local one and the one `JitDriver`'s `MetaInterp` owns — and only the former ran `finish_setup_descrs`, so the numbering was assigned off a list nothing consumes while the consumed list stayed empty. `ensure_descr_index` then returned the already-assigned global index without appending, and `bridgeopt.py:155 metainterp_sd.all_descrs[descr_index]` indexed a zero-length vec (`index out of bounds: the len is 0 but the index is 8` on bridge_branchy_callee, inline_multiframe_drain_journaled_store, inline_multiframe_module_branch_deopt, fannkuch). The storage moves to `descr_registry::ALL_DESCRS`; `MetaInterpStaticData::all_descrs()` is the accessor. Upstream keeps the list on `metainterp_sd` because there is one `metainterp_sd` built from one `cpu.setup_descrs()`. The six optimizer seeds change from `std::mem::take` of the slot to a clone: emptying it for the duration of an optimize left any reader inside that window with a zero-length universe. Assisted-by: Claude * jit: ignore shrinking take_back_all_descrs write-backs `descr.py:25-47 setup_descrs` numbers `all_descrs` once and `descr.py:28 v.descr_index = len(all_descrs); all_descrs.append(v)` only ever appends, so a write-back shorter than the published list is never a new universe. `unroll.rs` hands the list to each phase with `std::mem::take` and restores it on the way out; an early exit between the two leaves the outer `UnrollOptimizer` holding an empty vector, which `compile_loop` then publishes, invalidating every `descr_index` already serialized into a compiled bridge (`index out of bounds: the len is 0 but the index is 203` from `deserialize_optimizer_knowledge` on fannkuch). Assisted-by: Claude * jit: carry EffectInfo raw descr sets across descrs.bin as gccache keys The six raw sets of `effectinfo.py:128-145 frozenset_or_none` (`_readonly_descrs_fields`, `_write_descrs_fields` and the array and interiorfield pairs) hold `Arc<dyn Descr>` and were `#[serde(skip)]`, so every call descr read back from `descrs.bin` came up with them `None` — the shape `effectinfo.py:149-162` reserves for `EF_RANDOM_EFFECTS`. `compute_bitstrings` reads the two shapes oppositely, so a deserialized concrete EI had its bitstrings cleared instead of classified. Each member is now serialized as the gccache key the analyzer minted it through: `DescrSetMember::{Field, Array, InteriorField}` carries the `(struct_id, field_name)` / `(array_id)` / `(array_id, name)` tuple that `descr.py:218-239 get_field_descr`, `descr.py:348-378 get_array_descr` and `descr.py:404-437 get_interiorfield_descr` key their caches on. Both halves of the split agree on those tuples by construction. `rehydrate_build_descr_raw_sets` resolves them before `finish_setup_descrs` and re-derives `single_write_descr_array` (`effectinfo.py:201-206`, also serde-skipped and read by `heap.rs force_from_effectinfo`). It first materializes every non-call pool slot, so each parent publishes its full `heaptracker.all_fielddescrs(STRUCT)` list before any member is looked up. Resolution is lookup-only. Minting through a member would publish a parent `SizeDescr` with an empty field list and win `_cache_field` by first-write, breaking the `heaptracker.py:76-101 get_fielddescr_index_in` positional invariant that `optimizeopt/info.rs force_box` asserts. A member whose container is absent from this process's descr universe is dropped — no recorded operation can carry a descr for it; a member whose container is published but whose key misses degrades the EI to the wildcard instead. Measured on the append/loop corpus: 92 EIs rehydrated, 2 degraded. Assisted-by: Claude * jit: list PyFrame.w_globals once and hold get_field_descr cache hits to the caller's field `PYFRAME_DESCR_GROUP` named `"PyFrame.w_globals"` twice at `PYFRAME_W_GLOBALS_OFFSET`, at positions 4 and 12 of the field list, and `pyframe_w_globals_obj_descr` read position 12. `index_in_parent` is the position, so the two entries described the same slot under two different `heaptracker.py:76-101 get_fielddescr_index_in` answers; routing field descrs through `GcCache::get_field_descr` then collapsed them onto one cached `Arc` whose `index_in_parent` was whichever minted first. The duplicate was the last entry, so dropping it shifts nothing; the accessor moves to position 4. `descr.py:218-239` derives offset, size, flag, `_immutable_fields_` rank and `index_in_parent` from `(STRUCT, fieldname)` itself, so a cache hit upstream cannot describe a different field than the caller means. Pyre passes them in, so two call sites can disagree and the cache silently keeps the first mint. `SimpleFieldDescr::describes_same_field` states the invariant and a `debug_assert` in the cache-hit path enforces it; `index` is excluded because it is the per-trace codewriter slot id the analyzer legitimately restamps. `check.py --backend dynasm` built with `-C debug-assertions=on` reports no violation over the whole corpus: 2 failed / 329 passed, both failures pre-existing. Assisted-by: Claude * jit: allocate BC_NEW / bh_new structs through the GC and barrier the tracer's ref setfield `runner.rs bh_new` allocated every struct with `libc::malloc`, ignoring the descr's `type_id` that its `bh_new_with_vtable` sibling already honours. The two now share `bh_alloc_struct`, which routes a headered GC-managed descr to the non-moving old generation, a headerless one to the interpreter's own headerless nursery, and keeps the zeroed malloc for `type_id == 0` and for a runtime with no allocator hook installed. `pyjitpl/dispatch.rs BC_NEW` took the same shape one layer up: only the headerless case reached the GC, and everything else went to `std::alloc::alloc_zeroed`. A headered GC-managed descr now allocates in the old generation there too; both GC paths are no-collect and old-gen is mark-sweep, so the pointer the tracer keeps in its register bank stays valid. `BC_SETFIELD_GC_R` wrote the field with a raw store and no write barrier, unlike the `BC_SETARRAYITEM_GC_R` arm next to it and unlike `bh_setfield_gc_r`. It now notifies the GC on the container. `BhDescr::is_headerless` replaces the `owner == "__majit_headerless_size__"` comparison open-coded in `jitcode/assembler.rs` and twice in `dispatch.rs`; the marker constant moves next to the enum it tags. Assisted-by: Claude * jit: build the global build-time descr pool inside the OnceLock initializer `install_global_build_descr_pool` materialized the whole pool — one clone per `BhDescr` in the binary, each call descr carrying its `EffectInfo` raw descr sets, plus a `JitCode::from_canonical` per jitcode entry — and then handed it to `OnceLock::set`, which drops it once a pool is installed. `drive_unpack_iterable_trace` calls it before every `_unpackiterable_unknown_length` walk, so on an unpack-heavy program that build-and-drop dominated: on `bench/synth/exception_subclass_attrs.py` a `sample` run put 233 of 2465 main-thread samples in `install_global_build_descr_pool`, 128 of them in the `Arc<JitCode>` drop of the discarded pool. Measured CPU (user+sys, min of 3) goes 7.26s -> 4.48s. `set_global_build_descr_pool(pool)` becomes `init_global_build_descr_pool(build)`, which runs the closure from inside `OnceLock::get_or_init`. Assisted-by: Claude * jit: resolve SizeDescr::w_class_obj through a frontend-registered decoder `build_object_descr_group_with_def_path` used to build a `PyreSizeDescr`, whose `w_class_obj` reads `get_instantiate(vtable)` live. Routing it through `make_simple_descr_group_keyed_with_headerless` made every runtime PyObject group a `SimpleSizeDescr`, which inherits the trait default `None`. `OptVirtualize`'s `w_class` getfield arm (virtualize.rs:860-894) folds the header read off a `new_with_vtable` virtual to that constant, and takes the "class identity unresolved -> force the virtual" exit when it is `None`. The forced `W_IntObject` then reads `w_class` out of its own freshly allocated, uninitialised memory and guards on the `PtrEq`, so the guard fails on most iterations. On `bench/synth/exception_subclass_attrs.py`: guard failures 71654, bridges 331, CPU 4.5s. `SimpleSizeDescr::w_class_obj` now goes through `majit_ir::descr::set_w_class_obj_resolver`, which pyre registers in `install_jit_call_bridge` alongside the `str`/`unicode` green resolvers, and `PyreSizeDescr::w_class_obj` calls the same decoder. The hook also covers the size descrs `size_descr_ref_from_bh` mints inside majit-metainterp, which could not carry a pyre override at all. Same corpus entry after: guard failures 42, bridges 0, CPU 0.65s — equal to the branch base on all three. Assisted-by: Claude * jit: append in the orthodox list-append fold only when the sub-walk did not `orthodox_list_append_commit` ended with an unconditional `w_list_append` on the premise that the descended sub-walk records the store as IR without touching the concrete list. The per-strategy store the arm reaches (`W_ListObject::object_push`, `IntArray::push`, `FloatArray::push`) is a `residual_call`, and `try_execute_residual_call_via_executor` executes a residual whose funcptr resolves to a real address rather than only recording it, so on a target where the arm keeps them as residuals the sub-walk has already appended and the fold appends the value a second time. Re-read the receiver's length and append only when it is unchanged. The rewind journal entry stays unconditional, so an aborted walk rewinds to `len_before` whichever side grew the list. Assisted-by: Claude * jit: resolve W_ListObject field descrs to the canonical group before the parent group `make_descr_from_bh` bridged the codewriter's `W_ListObject` field names to the canonical `W_LIST_DESCR_GROUP` entries only after the parent-struct lookup, so whenever the codewriter modeled the parent the field ended up with two descrs: the parent group's entry for a codewriter-lowered body and the canonical entry for the walker-native list specializations. `MAJIT_LOG` shows both for `int_items.len` at offset 48 — index 5 (`index_in_parent` 5) and index 268436224 (`index_in_parent` 3). The heapcache and the optimizer's heap pass key on descr identity, so the `w_list_append` sub-walk's `SetfieldGc(int_items.len)` did not invalidate the `len(xs)` read that followed it, and the read folded to the pre-append length: one skipped `list.pop(0)` in the first compiled iteration, after which the steady-state length stays one too high. Run the bridge before the parent-group lookup. Fixes `list_ops`, `delete_negative_open_slice_hot`, `exception_residual_raise_caught_in_frame`, `sre_pattern_methods` on all three backends and wasm `comprehension_object_append_hot`'s output. Assisted-by: Claude * majit: register the headerless nursery-alloc hook in the cranelift and wasm backends `register_active_hooks` installed `alloc_nursery_typed` but left `alloc_nursery_headerless_no_collect` unset on these two backends, so `majit_gc::alloc_nursery_headerless_no_collect` returned `GcRef(0)` and the jitcode tracer's `NEW` on a `headerless` descr (`pyjitpl/dispatch.rs` BC_NEW) fell through to `std::alloc::alloc_zeroed` on the host heap, where the interpreter's collector cannot see it. The dynasm backend already registers it (`runner.rs`). Assisted-by: Claude * majit(cranelift): lower CallMallocNurseryHeaderless to an inline nursery bump The arm called `gc_alloc_nursery_headerless_shim` out of line for every allocation, spilling the ref roots and installing a gcmap each time. Both dynasm backends already emit an inline bump for this opcode (`genop_call_malloc_nursery_headerless`), and the cranelift `CallMallocNursery` arm right below already emits one for the headered case. Emit the same shape here, with the headerless deltas: bump by `size` alone (no `GcHeader::SIZE` reservation), no header word zeroed, result is the old nursery base. The slow path keeps the existing shim call with its spill / gcmap / reload. A runtime reporting no bump surface (`nursery_free` / `nursery_top` at 0) stays on the helper. aheui logo under cranelift, CPU time, 16 interleaved rounds over the runs that produce the reference output: min 20.66s -> 9.08s, median 21.47s -> 11.18s. `pyre/check.py --backend cranelift` 334/334; pyre declares no `headerless_structs`, so the opcode does not occur there. Assisted-by: Claude * majit-translate: stride pointer array items by the target word in the identity-less arraydescr fallback `arraydescrof_concrete`'s branch for an array with no `array_type_id` returned a fixed item size of 8. The named-element path (`get_type_flag`) and the codewriter-less fallback in `assembler.rs` already use `target_word_size()` for pointer elements; this branch now matches them. The list / tuple items-block `getarrayitem` / `setarrayitem` / `arraylen` ops the #171 append fold emits carry no `array_type_id`, so on wasm32 the descr placed items at `block+8` with an 8-byte stride while the runtime `ItemsBlock` holds 4-byte items at `block+4`. Assisted-by: Claude * majit-backend-wasm: implement bh_arraylen_gc, zero the old-gen JitFrame `WasmBackend` inherited the `bh_arraylen_gc` trait stub, which returns 0, so every array length reached at trace time read as 0. The override reads the word-width length prefix at `ArrayDescr.lendescr`, the same offset and width `bh_new_array` stores. `execute_token` allocates its `JitFrame` from the old-gen arena, whose `ArenaCollection::malloc` returns recycled bytes, while `JitFrame::init` requires zero-filled storage (the native `execute_token` uses `calloc`; the wasm nursery zeroes on reset). Zero the allocation before `init`, so a Ref home the trace has not defined when a collection lands reads as null rather than as a stale word. Drop the `!cfg!(target_arch = "wasm32")` gate on the `arraylen_gc` constant fold in `opimpl_arraylen_gc`; it was there because the stub made the fold bake `ConstInt(0)`. Measured on `bench/synth/comprehension_object_append_hot` under wasm: the 0 length made the #171 append fold bake the at-capacity arm, whose guard then failed on most appends — 5926 compiles over 1.2M guard failures, 98s. Now 24 compiles / 3610 guard failures, 0.35s. wasm `check.py` 322/323 -> 323/323; dynasm and cranelift stay 326/326. Assisted-by: Claude * majit: run rustfmt over resolve_w_class_obj and the metainterp re-export list The two hunks `cargo fmt --all -- --check` reports on this branch. Assisted-by: Claude * majit-ir: keep the caller's struct-qualified field name in GcCache::get_field_descr `get_field_descr` always minted the display name as `T<type_id>.<field>`, so every field descr routed through the keyed group builder lost the `Owner.field` spelling the caller already held. The non-keyed builder (`make_simple_descr_group_inner`) writes `spec.name` verbatim, and `PyreFieldDescr` stores `STRUCT.field`, so the keyed path was the odd one out; `descr.py:227` names a field `'%s.%s' % (STRUCT._name, fieldname)`. Add a `display_name: Option<&str>` argument. The two callers that carry a qualified name — `make_simple_descr_group_keyed_with_headerless` (from `SimpleFieldDescrSpec.name`) and `field_descr_from_bh_field` (from `BhFieldSpec.name`) — pass it; the mint sites that only hold a bare field key pass `None` and keep the `T<type_id>.` stand-in. Fixes `descr::tests::make_descr_from_bh_field_preserves_parent_name_index` and `descr::tests::make_descr_from_bh_struct_array_preserves_type_and_interior_fields`, which have been red on this branch since field descrs started minting through `GcCache::get_field_descr`. Assisted-by: Claude * majit-metainterp: serialize the finish_setup_descrs publish across threads `MetaInterpStaticData::finish_setup_descrs` writes `set_descr_index`, `set_ei_index` and `set_effect_bitstrings` onto descrs owned by the process-global `GcCache`, but pyre holds `MetaInterpStaticData` in a thread-local, so its `finish_setup_done` guard is per-thread. Two threads reaching the publish together are two writers over one `EffectInfoCell`, whose `set_bitstrings` is documented as single-writer: each drops the `Vec<u8>` the other just installed. `warmspot.py:289` has one writer by construction — `finish_setup` runs once, in one process, before tracing. Take a process-global mutex for the publish so that holds here too. The crash it fixes is the `cargo test -p pyre-jit-trace --lib` abort (`pointer being freed was not allocated` on macOS, `double free or corruption (fasttop)` on Linux) whose faulting stack is `ensure_finish_setup -> finish_setup_descrs -> set_effect_bitstrings -> EffectInfoCell::set_bitstrings -> drop of Option<Vec<u8>>`. 9/15 runs of the test binary aborted before, 0/25 after; single-threaded runs never reproduced it. Assisted-by: Claude * jit: record the measured gap behind SetMemberLookup::AbsentContainer Codex parity review flagged the `AbsentContainer => {}` arm as an unsound drop of a serialized write-set member. The premise the arm rests on — "the container is absent, so no recorded operation can carry a descr for it" — is evaluated once, when the `BhCallDescr` is materialized, while the runtime descr universe keeps growing after that, so a container registered later leaves the EI claiming "not written" for a field the callee writes. Document that, plus why the conservative repair is not taken here: a probe over `bench/synth/comprehension_object_append_hot` counts 211 drops across ~40 distinct containers, so degrading each to `EF_RANDOM_EFFECTS` would turn most residual calls into whole-heap barriers. Name the convergence path (re-resolve from the retained `descr_set_keys` as the universe grows) and its blockers. No behavior change. 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.
Splits state-field resume data into per-frame sections by liveness on the bridge
read path, and gives up on bridges whose resume data spans more than one frame.
Background
build_state_field_snapshot(pyre/pyre-jit-trace/src/state.rs, mirroringresume.py'scapture_resumedata) writes one section per MIFrame, outer toinner, each section sized by its own jitcode's
-live-liveness at its own pc.The reader in
majit-macros'rebuild_from_resumedatapassedframe_value_count = None, so it treated the whole stream as a single frame:frame 0 swallowed the callee section's
[jitcode_index, pc, py_pc]header plusits values.
On the aheui
pi.jinseo.aheuiguard at pc=613 this decoded 14 values against 6live root registers.
setup_bridge_symthen declined to seed the bridge, thebridge was compiled with
create_sym's loop-namespaceInputArgsanyway, andOptIntBounds::propagate_forwardpanicked withgetintbound_handle: expected 'i'-typed operand, got Ref(←
compile_bridge←close_bridge).Changes
majit-metainterp/src/resume_box_reader.rs(new) — bridge-entry virtualmaterialization moved out of
pyre-jit-trace/src/state.rsso the macro pathand the pyre path share one reader.
state.rsloses ~870 lines(
emit_stroruni_oopspec_call,BridgeVirtualCache,decode_fieldnum,materialize_bridge_virtual);origin/main's profiler op counting is carriedinto the moved copy (15
count_opssites,OPS+RECORDED_OPS).majit-macros/src/jit_interp/codegen_state.rs—rebuild_from_resumedatanow decodes through the registered liveness splitter
(
majit_ir::resumedata::get_frame_value_count_fn()) instead ofNone, soeach frame section is sized by its own jitcode/pc liveness
(
jitcode.py:147 enumerate_vars,resume.py:1049-1055).majit-metainterp/src/jitdriver.rs— afterrebuild_from_resumedata, aresume record with more than one frame gives up on the bridge
(
compile.py:725-729 compile.giveup()) using the existing tracing-sessioncleanup idiom, rather than compiling a bridge that cannot be seeded.
majit-metainterp/src/optimizeopt/optimizer.rs— the bridge-path previewvirtual-state mismatch is no longer fatal.
MAJIT_BRIDGE_ONLY=<fail_index,...>— new env filter restricting bridgeformation to the listed guard fail indices, applied at both
should_bridgesites. Used to bisect which guard's bridge miscompiles; unset means "all",
so there is no behaviour change by default.
majit-metainterp/src/jitdriver.rs,pyjitpl.rs,pyjitpl/dispatch.rs,trace_ctx.rs— a bridge closing on a merge point took its JUMP target fromcurrent_trace_green_key().unwrap_or(bridge_key), the green key of the loopthe failing guard belonged to. When the trace closed on an inner merge
point (the cross-loop cut) that key names a different loop than the one
reached, and since
pcis a green baked into each compiled loop as aconstant, the JUMP entered a loop compiled for another pc while carrying the
reached pc's reds.
pyjitpl.py:3005-3007readsptoken = self.get_procedure_token(greenboxes)off the greens of the mergepoint just reached, and
pyjitpl.py:3012-3060keeps tracing (or compiles aloop) when that greenkey has no procedure token — it never substitutes
another loop's token. Both close paths now record the closing merge point's
greens on the trace ctx, each compiled loop registers the greens it closed
on, and the bridge inverts those greens back to a key; when no compiled loop
holds them,
has_targetsis false and the existingcompile_loopfall-through (
pyjitpl.py:3014-3017) runs.update_tracing_green_key,named by the previous comment as the retargeting mechanism, has no call
sites.
Verification
python3 ./pyre/check.py— dynasm 308/308, cranelift 308/308, wasm 305/305.logo,99bottles,99dan,quine) —--jitoutputbyte-identical to
--no-jit.getintbound_handlepanic is gone; bridge seed declines onpi.jinseo.aheuigo 2 → 0, with 2 giveups in their place.pi.jinseo.aheuiwas isolated to two independent defects, measured as a 2×2matrix over bridges × collection. The bridge half is what this PR fixes:
Per-guard, with collection disabled:
MAJIT_BRIDGE_ONLY=50diverged at byte806 and
=17at byte 2195; both are now byte-identical, as is the fullbridge set.
{2,3,6,7},{9}and{12}were already correct and remain so.Still open
pi.jinseo.aheui --jitis still not correct with collection enabled — aseparate GC-layer defect, unchanged by this PR and tracked apart from it.
Quarantining from-space instead of releasing it removes the SIGSEGV but not
the wrong output, which points at a JIT-held
Nodereference missing fromthe copying collector's root walk rather than at bridge compilation.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes