majit: evict per-loop side tables on retirement and harden MAJIT_BRIDGE_ONLY parsing - #787
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
WalkthroughThe changes unify descriptor identity and effect metadata, add headerless non-collecting GC allocation, centralize metainterpreter JitCode and loop state, and synchronize liveness and callable-address handling across resumed Pyre JIT execution. ChangesDescriptor identity and effect metadata
Headerless non-collecting allocation
Metainterpreter state and loop lifecycle
Pyre resumed JIT assembly
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1806815). 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)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5de0daba8
ℹ️ 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".
| fn forget_loop_side_tables(&mut self, green_key: u64) { | ||
| self.loop_header_pcs.swap_remove(&green_key); | ||
| self.loop_header_greens.swap_remove(&green_key); |
There was a problem hiding this comment.
When a long-running process compiles distinct loop keys and then uses clear_compiled_loops, mark_all_loops_for_release, or invalidate_compiled_trace, those paths delete compiled_loops entries directly and never call this helper. Consequently loop_header_pcs and loop_header_greens still retain one entry per retired key, so the unbounded growth this change is intended to prevent remains for guard-recovery resets, jit_hooks.releaseall, and trace invalidation. Route every compiled-entry removal through the cleanup or clear/retain the side tables alongside it.
Useful? React with 👍 / 👎.
| v.split(',') | ||
| .filter_map(|t| t.trim().parse::<u32>().ok()) | ||
| .map(|t| t.trim()) | ||
| .filter(|t| !t.is_empty()) |
There was a problem hiding this comment.
When MAJIT_BRIDGE_ONLY is empty, whitespace-only, or comma-only, filtering empty tokens produces Some(Vec::new()) rather than triggering the new validation. allowed.contains then rejects every guard without a diagnostic, preserving exactly the confidently wrong bridge-bisection result this hardening is meant to eliminate. Reject the value when no fail indices remain, or explicitly treat it as unset.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/jitdriver.rs (1)
4909-4924: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame side-table leak as
remove_compiled_loop/try_to_free_some_loops.
mark_all_loops_for_releaseclearsself.meta.compiled_loopsbut leavesloop_header_pcs/loop_header_greenspopulated for every green key that was ever compiled. Sincecompiled_key_for_greensfilters onhas_compiled_targets, this is not a correctness bug today, but it defeats the intent of the PR's newforget_loop_side_tablescleanup at exactly the same class of eviction site.🤖 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 4909 - 4924, Update mark_all_loops_for_release to invoke the existing forget_loop_side_tables cleanup for every compiled loop before clearing self.meta.compiled_loops, so loop_header_pcs and loop_header_greens are removed during bulk eviction. Preserve the existing release_all_loops behavior and ensure cleanup covers all entries being discarded.
🤖 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/pyjitpl.rs`:
- Around line 9002-9016: Extend side-table cleanup to every compiled-loop
eviction path: update clear_compiled_loops and the
mark_all_loops_for_release/invalidate_compiled_trace flow to remove
corresponding loop_header_pcs and loop_header_greens entries, reusing
forget_loop_side_tables where applicable. Ensure bulk clearing leaves no
per-loop side-table entries behind.
---
Outside diff comments:
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 4909-4924: Update mark_all_loops_for_release to invoke the
existing forget_loop_side_tables cleanup for every compiled loop before clearing
self.meta.compiled_loops, so loop_header_pcs and loop_header_greens are removed
during bulk eviction. Preserve the existing release_all_loops behavior and
ensure cleanup covers all entries being discarded.
🪄 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: e5e0c3d5-4343-48d7-9b16-45794bda927e
📒 Files selected for processing (3)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/resume_box_reader.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0826090bf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let headerless = descr.as_size_descr().is_some_and(|sd| sd.headerless()); | ||
| let ptr = Some(size.max(1)) | ||
| .filter(|_| headerless) | ||
| .map(|n| majit_gc::alloc_nursery_headerless_no_collect(n).0 as i64) |
There was a problem hiding this comment.
When a headerless_structs interpreter runs with the Cranelift backend, majit-backend-cranelift::register_active_hooks never installs set_active_alloc_nursery_headerless_no_collect, even though Cranelift supports CallMallocNurseryHeaderless. This call therefore returns null and the fallback allocates the traced object on the host heap, outside the interpreter collector; after a collection, references reachable through that object can be lost rather than forwarded. Install the new callback for every backend supporting headerless allocation, or fail loudly instead of using an untracked allocation.
Useful? React with 👍 / 👎.
| fn alloc_nursery_headerless_no_collect(&mut self, size: usize) -> GcRef { | ||
| self.alloc_nursery_headerless(size) |
There was a problem hiding this comment.
When an existing moving, headerless-aware allocator implements only alloc_nursery_headerless, it silently inherits this default and may evacuate the nursery on overflow. That directly violates the preceding invariant: the jitcode tracer has raw pointers in an unrooted register bank, so such a collection can leave stale pointers and corrupt the trace. Make moving allocators provide an actual grow/no-collect implementation rather than giving them a collecting default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34ddb53a04
ℹ️ 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".
| insns, | ||
| all_liveness, | ||
| all_liveness_length, | ||
| ..Self::default() |
There was a problem hiding this comment.
When a runtime liveness triple already exists in the build-time prefix, this constructor copies the bytes but leaves all_liveness_positions empty via Self::default(), so the first runtime occurrence appends a duplicate rather than reusing its existing offset. Since each -live- operand is limited to a 16-bit offset, workloads that lazily compile many distinct frame layouts can exhaust the 64 KiB pool earlier than the single-Assembler upstream design; serialize or reconstruct the build-time dedup map when resuming the buffer (and seed the reader-side mirror consistently).
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
majit/majit-metainterp/src/pyjitpl/dispatch.rs (1)
10469-10484: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the new
exc_valueon the one test that exercises it.This is the test for the handler-less raise path, which is exactly where Line 1789-1803 now snapshots
last_exception_valueand hands it to the caller before clearing the channel. The..added here skips that field, so the new handoff has no coverage.exc_rawis already in scope.💚 Bind and assert `exc_value`
- let (finish_args, finish_arg_types, exit_with_exception) = match action { + let (finish_args, finish_arg_types, exit_with_exception, exc_value) = match action { TraceAction::Finish { finish_args, finish_arg_types, exit_with_exception, - .. - } => (finish_args, finish_arg_types, exit_with_exception), + exc_value, + } => ( + finish_args, + finish_arg_types, + exit_with_exception, + exc_value, + ), other => panic!( "expected TraceAction::Finish for handler-less raise, got {:?}", other ), }; assert_eq!(finish_arg_types, vec![majit_ir::Type::Ref]); assert_eq!(finish_args.len(), 1); assert!(exit_with_exception); + assert_eq!( + exc_value, exc_raw, + "the drained-stack exit must hand the exception value to the caller", + );🤖 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/dispatch.rs` around lines 10469 - 10484, Update the handler-less raise test’s TraceAction::Finish destructuring to bind the new exc_value field instead of skipping it, then assert it matches the in-scope exc_raw value before the existing finish argument and exception checks.pyre/pyre-jit/src/jit/codewriter.rs (1)
5322-5334: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSeed the metainterp
shared_asmbefore building build-time jitcodes.
JitDriver::with_optionsstill createsshared_asmwithAssembler::new(), butsync_liveness_info_from_shared_asm()/ the macro-emitted install path reads it during driver setup. UseAssembler::resuming_build_time_liveness()here so any build-time jitcodes have the same liveness prefix before decoding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit/src/jit/codewriter.rs` around lines 5322 - 5334, Update JitDriver::with_options to initialize shared_asm with Assembler::resuming_build_time_liveness() instead of Assembler::new(). Preserve the existing shared_asm setup and ensure sync_liveness_info_from_shared_asm() and the macro-emitted install path observe the seeded liveness prefix during driver initialization.pyre/pyre-interpreter/src/jit_fnaddr.rs (1)
409-437: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the missing bare-name
push_fnaddrforw_list_new_object.
w_list_new_objectis residualized (#[dont_look_inside]) but the entry only registers the qualified/aliased paths. Add a bare"w_list_new_object"binding so residual call sites that spell it directly resolve to the real funcptr instead of a fallback symbolic fnaddr hash.🛡️ Proposed fix
let w_list_new_object: fn(Vec<pyre_object::PyObjectRef>) -> pyre_object::PyObjectRef = pyre_object::listobject::w_list_new_object; push_alias_pair( &mut entries, "pyre_object::listobject::w_list_new_object", "pyre_object::w_list_new_object", w_list_new_object as *const (), ); + push_fnaddr( + &mut entries, + "w_list_new_object", + w_list_new_object as *const (), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/jit_fnaddr.rs` around lines 409 - 437, Add a bare-name push_fnaddr registration for w_list_new_object, using the same function pointer already used by its push_alias_pair entry. Place it alongside the existing w_list_new_object registrations in the JIT function-address setup, without changing the alias mappings or neighboring drain_collect_items handling.majit/majit-metainterp/src/jitdriver.rs (2)
2837-2856: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftKeep the escaping exception rooted while compiling.
exc_valueis a GC reference encoded asi64; this path stores it only inBH_LAST_EXC_VALUEand then calls the allocating compile path. Unlike the explicit shadow-stack rooting earlier in this file, the TLS cell is not registered as a GC root, so a moving collection can leave the published exception stale. Root it for the whole compile call, write the forwarded value back to the continuation channel afterward, and pop the root on every exit.As per coding guidelines, “Do not use TLS for process-global, interpreter-owned, identity-sensitive, semantic, registry, cache, or GC-relevant runtime state.”
🤖 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 2837 - 2856, Update the exit-with-exception path surrounding the compile call to explicitly shadow-root the GC reference encoded by exc_value for the entire allocating compilation, rather than relying on BH_LAST_EXC_VALUE as a root. After compilation, write the forwarded value back to BH_LAST_EXC_VALUE for the continuation and ensure the shadow root is popped on every exit, including error paths; avoid using TLS as the GC root.Source: Coding guidelines
763-786: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove the process-global/TLS frame-value-count fallback.
Noneretains the global callback path, butinstall_state_field_fvcoverwritesSTATE_FIELD_FVCon every dispatch installation. Multiple drivers on one thread can therefore decode a frame using another driver's JitCode/liveness registry, silently producing the wrong box count. Keep this state on the owningMetaInterp/driver and route every decode through that owner rather than a process-global callback plus TLS payload.As per coding guidelines, “Do not use TLS for process-global, interpreter-owned, identity-sensitive, semantic, registry, cache, or GC-relevant runtime state.”
🤖 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 763 - 786, Remove the process-global/TLS fallback used by frame-value-count decoding and make the owning MetaInterp/driver authoritative. Update frame_value_count_fn and the decode path around install_state_field_fvc so each dispatch uses its owner’s JitCode/liveness registry directly, without consulting STATE_FIELD_FVC or TLS payload; preserve correct per-driver decoding when multiple drivers share a thread.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 9097-9110: Update invalidate_compiled_trace to clear the
pending_preamble_tokens associated with each retired green_key, matching the
cleanup performed by remove_compiled_loop. Preserve the existing compiled_loops
removal and forget_loop_side_tables calls while ensuring no pending tokens
remain for invalidated keys.
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 2906-2916: Update the headerless allocation path in the BC_NEW
dispatch logic so a successful alloc_nursery_headerless_no_collect result is
zero-initialized before being returned, preserving the alloc_zeroed behavior of
the fallback path and the surrounding malloc-plus-zero contract.
---
Outside diff comments:
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 2837-2856: Update the exit-with-exception path surrounding the
compile call to explicitly shadow-root the GC reference encoded by exc_value for
the entire allocating compilation, rather than relying on BH_LAST_EXC_VALUE as a
root. After compilation, write the forwarded value back to BH_LAST_EXC_VALUE for
the continuation and ensure the shadow root is popped on every exit, including
error paths; avoid using TLS as the GC root.
- Around line 763-786: Remove the process-global/TLS fallback used by
frame-value-count decoding and make the owning MetaInterp/driver authoritative.
Update frame_value_count_fn and the decode path around install_state_field_fvc
so each dispatch uses its owner’s JitCode/liveness registry directly, without
consulting STATE_FIELD_FVC or TLS payload; preserve correct per-driver decoding
when multiple drivers share a thread.
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 10469-10484: Update the handler-less raise test’s
TraceAction::Finish destructuring to bind the new exc_value field instead of
skipping it, then assert it matches the in-scope exc_raw value before the
existing finish argument and exception checks.
In `@pyre/pyre-interpreter/src/jit_fnaddr.rs`:
- Around line 409-437: Add a bare-name push_fnaddr registration for
w_list_new_object, using the same function pointer already used by its
push_alias_pair entry. Place it alongside the existing w_list_new_object
registrations in the JIT function-address setup, without changing the alias
mappings or neighboring drain_collect_items handling.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 5322-5334: Update JitDriver::with_options to initialize shared_asm
with Assembler::resuming_build_time_liveness() instead of Assembler::new().
Preserve the existing shared_asm setup and ensure
sync_liveness_info_from_shared_asm() and the macro-emitted install path observe
the seeded liveness prefix during driver initialization.
🪄 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: b884d8fd-34b8-42de-b7f9-e230781dc7f7
📒 Files selected for processing (16)
majit/majit-backend-dynasm/src/runner.rsmajit/majit-gc/src/lib.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/virtualstate.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume_box_reader.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-jit-trace/src/assembler.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/jit/assembler.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/listobject.rs
| /// Drop every compiled loop whose root trace is `trace_id`, with the | ||
| /// per-loop side tables that belong to it. | ||
| pub fn invalidate_compiled_trace(&mut self, trace_id: u64) { | ||
| let stale: Vec<u64> = self | ||
| .compiled_loops | ||
| .iter() | ||
| .filter(|(_, entry)| entry.root_trace_id == trace_id) | ||
| .map(|(green_key, _)| *green_key) | ||
| .collect(); | ||
| for green_key in stale { | ||
| self.compiled_loops.swap_remove(&green_key); | ||
| self.forget_loop_side_tables(green_key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of invalidate_compiled_trace and any related pending_preamble_tokens handling.
rg -n 'invalidate_compiled_trace' --type=rust -C3
rg -n 'pending_preamble_tokens' --type=rust -C3Repository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
printf 'Tracked rust files around pyjitpl/compile/jitdriver:\n'
git ls-files | rg '(^|/)(pyjitpl|compile|jitdriver)\.rs$|majit-metainterp|jitrpy' | sed -n '1,120p' || true
printf '\nSearch for invalidate_compiled_trace with all files, context=all:\n'
rg -n --hidden --glob '!target/**' 'invalidate_compiled_compile|invalidate_compiled_trace|invalidate.*trace|compiled_loops|pending_preamble_tokens|loop_header_pcs|loop_header_greens|clear_compiled_loops|forget_loop_side_tables|compile_loop|InvalidLoop|Invalid.*Loop' . -S || true
printf '\nFile list names:\n'
fd -i 'pyjitpl\.rs|compile\.rs|jitdriver\.rs|metainterp' . -t f | sed -n '1,200p'Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -u
printf 'locate pyjitpl.rs candidates:\n'
git ls-files | rg 'majit/majit-metainterp/src/pyjitpl\.rs|pyre/pyre-jit-trace/src/pyjitpl\.rs'
printf '\npyjitpl compile/state definitions:\n'
python3 - <<'PY'
from pathlib import Path
p=Path('majit/majit-metainterp/src/pyjitpl.rs')
lines=p.read_text().splitlines()
for needle in ['struct .*CompiledLoop', 'pub struct .*', 'pending_preamble_tokens', 'compiled_loops', 'loop_header_pcs', 'loop_header_greens', 'remove_compiled_loop', 'clear_compiled_loops', 'invalidate_compiled_trace', 'forget_loop_side_tables']:
print(f'\n--- {needle} ---')
count=0
for i,l in enumerate(lines,1):
if needle in l:
start=max(1, i-8); end=min(len(lines), i+18)
print('\n'.join(f'{j}: {lines[j-1]}' for j in range(start,end+1)))
count+=1
if count>=4: break
PY
printf '\nsimilar state references:\n'
rg -n 'pending_preamble_tokens|loop_header_pcs|loop_header_greens|compiled_loops|pending_compile|compile_loop|remove_compiled_loop|clear_compiled_loops|invalidate_compiled_trace|forget_loop_side_tables' majit/majit-metainterp/src pyre/pyre-jit-trace/src/pypy -g '*.rs' -g '*.py' -S || falseRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
printf 'compile.rs around pending_preamble_tokens entry:\n'
rg -n 'pending_preamble_tokens|InvalidLoop|CompileStatus|compile_loop|compile_retrace|make_jitcell_token' majit/majit-metainterp/src/compile.rs -C4 --max-count 200
printf '\npyjitpl around compile_retry_start_retrace / compile_loop wrapper:\n'
sed -n '5575,5640p' majit/majit-metainterp/src/pyjitpl.rs | nl -ba -v5575
printf '\ncompile.rs line ranges summary for stale search:\n'
python3 - <<'PY'
from pathlib import Path
text=Path('majit/majit-metainterp/src/compile.rs').read_text().splitlines()
keys=['pending_preamble_tokens','CompileStatus','compile_loop','compile_retrace','make_jitcell_token','token']
for i,line in enumerate(text,1):
if any(k in line for k in keys):
start=max(1, min(i, len(text)-50)); end=max(1, min(i+50, len(text)))
# crude around first matches
PYRepository: youknowone/pyre
Length of output: 7578
🏁 Script executed:
#!/bin/bash
set -u
printf 'compile.py upstream pending_preamble_tokens/InvalidLoop references:\n'
rg -n 'pending_preamble_tokens|InvalidLoop|compile_loop|compile_retrace|make_jitcell_token|compile.*target' majit/majit-metainterp/src/compile.py majit/majit-metainterp src/pypy -S -C3 || true
printf '\nexact pyjitpl validate_retrace_start and retry area:\n'
awk '
/validate_retrace_start|must_compile_with_values|prior_retraced_count|pending_preamble_tokens|compile_retrace/ {
s=NR>5545?NR-5545:1; e=NR+50; if(e>5660) e=5660;
print "--- lines "s"-"e" ---";
for(i=s;i<=e;i++) printf "%6d:%s\n", i, lines[i]
}
{lines[NR]=$0}
' majit/majit-metainterp/src/pyjitpl.rs
printf '\nexact compile.py invalid-loop/compile retry references with line numbers:\n'
rg -n 'pending_preamble_tokens|InvalidLoop|compile_loop|compile_retrace|last_compile_status|retrace' majit/majit-metainterp/src/compile.py -C3 || trueRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -u
printf 'pyjitpl.rs compile_retrace body range:\n'
python3 <<'PY'
from pathlib import Path
p=Path('majit/majit-metainterp/src/pyjitpl.rs')
lines=p.read_text().splitlines()
start=end=0
for i,l in enumerate(lines,1):
if 'pub(crate) fn compile_retrace' in l or 'fn compile_retrace' in l:
start=max(1, i-10); end=i+80
break
print('\n'.join(f'{j}: {lines[j-1]}' for j in range(start,end+1)))
PY
printf '\npyjitpl.rs compile_retrace exact token handling and retry:\n'
rg -n 'compile_retrace|pending_preamble_tokens|prior_front_target_tokens|must_compile_with_values' majit/majit-metainterp/src/pyjitpl.rs -C4 --max-count 200
printf '\ncompile.py exact compile/compile_retrace token/PreambleCompileData area:\n'
python3 <<'PY'
from pathlib import Path
p=Path('majit/majit-metainterp/src/compile.py')
lines=p.read_text().splitlines()
for needle in ['def compile_retrace(', 'def compile_loop(', 'pending_preamble_tokens', 'if invalid', 'PreambleCompileData']:
print(f'\n--- {needle} ---')
count=0
for i,l in enumerate(lines,1):
if needle in l:
start=max(1, i-20); end=min(len(lines), i+80)
print('\n'.join(f'{j}: {lines[j-1]}' for j in range(start,end+1)))
count+=1
if count>=2:
break
PYRepository: youknowone/pyre
Length of output: 27293
Clear pending_preamble_tokens in invalidate_compiled_trace.
A key can retain preamble target tokens after an InvalidLoop/panic before it is ever recompiled. When this path retires matching green keys, it should consume the same pending tokens that remove_compiled_loop clears, in addition to dropping compiled_loops and the loop-header side tables.
♻️ Possible fix
pub fn invalidate_compiled_trace(&mut self, trace_id: u64) {
let stale: Vec<u64> = self
.compiled_loops
.iter()
.filter(|(_, entry)| entry.root_trace_id == trace_id)
.map(|(green_key, _)| *green_key)
.collect();
for green_key in stale {
self.compiled_loops.swap_remove(&green_key);
+ self.pending_preamble_tokens.swap_remove(&green_key);
self.forget_loop_side_tables(green_key);
}
}📝 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.
| /// Drop every compiled loop whose root trace is `trace_id`, with the | |
| /// per-loop side tables that belong to it. | |
| pub fn invalidate_compiled_trace(&mut self, trace_id: u64) { | |
| let stale: Vec<u64> = self | |
| .compiled_loops | |
| .iter() | |
| .filter(|(_, entry)| entry.root_trace_id == trace_id) | |
| .map(|(green_key, _)| *green_key) | |
| .collect(); | |
| for green_key in stale { | |
| self.compiled_loops.swap_remove(&green_key); | |
| self.forget_loop_side_tables(green_key); | |
| } | |
| } | |
| /// Drop every compiled loop whose root trace is `trace_id`, with the | |
| /// per-loop side tables that belong to it. | |
| pub fn invalidate_compiled_trace(&mut self, trace_id: u64) { | |
| let stale: Vec<u64> = self | |
| .compiled_loops | |
| .iter() | |
| .filter(|(_, entry)| entry.root_trace_id == trace_id) | |
| .map(|(green_key, _)| *green_key) | |
| .collect(); | |
| for green_key in stale { | |
| self.compiled_loops.swap_remove(&green_key); | |
| self.pending_preamble_tokens.swap_remove(&green_key); | |
| self.forget_loop_side_tables(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 9097 - 9110, Update
invalidate_compiled_trace to clear the pending_preamble_tokens associated with
each retired green_key, matching the cleanup performed by remove_compiled_loop.
Preserve the existing compiled_loops removal and forget_loop_side_tables calls
while ensuring no pending tokens remain for invalidated keys.
| let headerless = descr.as_size_descr().is_some_and(|sd| sd.headerless()); | ||
| let ptr = Some(size.max(1)) | ||
| .filter(|_| headerless) | ||
| .map(|n| majit_gc::alloc_nursery_headerless_no_collect(n).0 as i64) | ||
| .filter(|p| *p != 0) | ||
| .unwrap_or_else(|| { | ||
| let layout = std::alloc::Layout::from_size_align(size.max(1), 8) | ||
| .expect("BC_NEW: invalid struct layout"); | ||
| let raw = unsafe { std::alloc::alloc_zeroed(layout) }; | ||
| raw as i64 | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does any headerless nursery allocation path zero the returned block?
ast-grep run --pattern 'fn alloc_nursery_headerless($$$) -> GcRef { $$$ }' --lang rust majit/majit-gc/src
ast-grep run --pattern 'fn alloc_nursery_headerless_no_collect($$$) -> GcRef { $$$ }' --lang rust majit/majit-gc/src majit/majit-backend-dynasm/src
rg -n --type=rust -C5 'write_bytes|zeroed|memset' majit/majit-gc/src/nursery.rsRepository: youknowone/pyre
Length of output: 2382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant files =="
git ls-files | rg 'majit/majit-gc/src/lib.rs|majit/majit-metainterp/src/pyjitpl/dispatch.rs' || true
echo
echo "== GC allocation implementations =="
sed -n '160,185p;1735,1770p' majit/majit-gc/src/lib.rs
echo
echo "== dispatch allocation context =="
sed -n '2893,2922p' majit/majit-metainterp/src/pyjitpl/dispatch.rs
echo
echo "== Headerless allocator references/config =="
rg -n --type=rust -C3 'headerless|alloc_nursery_headerless_no_collect|alloc_nursery_headerless' majit/majit-gc/src majit/majit-metainterp/src/pyjitpl/dispatch.rsRepository: youknowone/pyre
Length of output: 24195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for headerless allocator implementations / hooks:"
rg -n --type=rust -C4 'add_alloc_nursery_headerless|alloc_nursery_headerless|set_active_alloc_nursery_headerless_no_collect|call_malloc_nursery_headerless|call_malloc.*headerless' .
echo
echo "Search for headerless zeroing / write_bytes nearby:"
rg -n --type=rust -C3 'headerless|zeroed|write_bytes|memset' majit/majit-gc/src majit/majit-backend-dynasm/src majit/majit-metainterp/src/pyjitpl/dispatch.rs | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 39918
Zero the block returned by the headerless nursery allocator.
The active headerless path returns a raw bump-pointer nursery block with no zeroing, while the fallback host path uses alloc_zeroed and the surrounding comment still describes bh_new as “malloc + zero”. Zero the returned nursery object after alloc_nursery_headerless_no_collect succeeds, or tighten these sites' guards with an explicit zeroing invariant at the allocator boundary.
🤖 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/dispatch.rs` around lines 2906 - 2916,
Update the headerless allocation path in the BC_NEW dispatch logic so a
successful alloc_nursery_headerless_no_collect result is zero-initialized before
being returned, preserving the alloc_zeroed behavior of the fallback path and
the surrounding malloc-plus-zero contract.
|
Rebased onto
Measured on
(user+sys CPU, min of 3. Before the fix, wall clock was 5.56s / 7.38s on depth2 / depth3 against 1.95s / 2.54s for
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/14e33b1be05dc433cf0f616ab4ff81a9843eba61/pyre-interpreter/src/jit_fnaddr.rs#L1342-L1346
When the default-enabled jd1 unpack loop exits a guard inside w_list_append, these bindings do not resolve the resumed store: Charon identifies the methods by their defining paths (listobject::W_ListObject::object_push, int_array::IntArray::push, and float_array::FloatArray::push), while the new entries register only the crate-root re-export spellings. Because target_to_path finds the module-qualified function graph first, fnaddr_for_target misses these entries and retains a symbolic hash; the blackhole then aborts instead of appending the already-fetched item. Register both the defining-module and root aliases, as the nearby PyFrame method bindings do.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/a439550d6966d7b0150e7437fc9a47cd4d6a15a6/pyre-interpreter/src/jit_fnaddr.rs#L1342-L1345
When a resumed append resolves these extracted inherent methods through target_to_path, the path is module-qualified (listobject::W_ListObject::object_push, and likewise int_array::IntArray::push / float_array::FloatArray::push); fnaddr_for_target returns immediately for that resolved path and never tries the graph-less two-segment fallback. These root-only registrations therefore miss, leaving the symbolic address that the blackhole rejects and still dropping the in-flight append item. Register both root and module-qualified spellings with push_alias_pair, as the nearby PyFrame method bindings do.
AGENTS.md reference: AGENTS.md:L14-L20
ℹ️ 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".
90e298c to
88992d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88992d03a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for m in members { | ||
| match descr_from_set_member(m) { | ||
| SetMemberLookup::Resolved(d) => out.push(d), | ||
| SetMemberLookup::AbsentContainer => {} |
There was a problem hiding this comment.
When a build-time call's effect set references a container that is absent during rehydrate_build_descr_raw_sets but is registered lazily later, this arm permanently drops the member from the frozen raw set. compute_bitstrings can then classify the call as not writing that descriptor, allowing the heap optimizer to reuse a stale field or array read across a call that actually mutates it and silently miscompile the program. Retain and re-resolve these keys as the descriptor universe grows, or conservatively degrade the effect to random effects instead of treating absence as an empty effect.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| } else if type_id != 0 { | ||
| majit_gc::alloc_oldgen_typed(type_id, size).0 |
There was a problem hiding this comment.
When BC_NEW creates a headered GC-managed object and a collecting residual call or user gc.collect() runs before that object escapes the machine register bank, the only reference is the raw pointer stored by set_ref_reg. Allocating it in old generation prevents movement during a minor collection but does not make it a GC root, so a major collection can sweep the object and leave subsequent SETFIELD or GETFIELD operations dereferencing freed memory. Expose the live reference registers to the collector or keep the new object temporarily rooted across collecting operations.
Useful? React with 👍 / 👎.
…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
… 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
`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
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
`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
`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
…ge 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
`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
`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
…naddrs 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
…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
`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
…eld_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
…eader 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
`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
`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
`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
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
…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
…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
…alizer `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
…oder `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
…id 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
…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
…d 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
…ery 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
… 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
`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
…ort list The two hunks `cargo fmt --all -- --check` reports on this branch. Assisted-by: Claude
…et_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
…reads `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
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
1 participant
Follow-up to #773, which was merged before its review comments were addressed.
Four findings from CodeRabbit and Codex; each is described with what it does and
does not affect.
Changes
pyjitpl.rs— per-loop side tables outlived the loops they describe.loop_header_pcs(pre-existing) andloop_header_greens(added by majit: split state-field resume frames by liveness and give up on multi-frame bridges #773) wereinserted on compile and never removed, so both kept growing as loops were
retired.
remove_compiled_loopand the memmgr eviction path intry_to_free_some_loopsnow drop the retired key from both, through a sharedforget_loop_side_tables.This is a growth fix, not a targeting fix:
compiled_key_for_greensalreadygates its match on
has_compiled_targets, so a stale entry could never haveresolved a bridge onto a retired loop.
jitdriver.rs—MAJIT_BRIDGE_ONLYsilently swallowed unparsable entries.The list was parsed with
filter_map(|t| t.parse().ok()), soMAJIT_BRIDGE_ONLY=oopsproduced an empty allowlist — which suppresses everybridge, the exact inverse of the documented "unset means all" default, with no
diagnostic. Since this knob exists to bisect a miscompiling bridge out of a
run, a typo would have produced a confidently wrong result. An unparsable entry
now panics.
resume_box_reader.rs— the twofieldnumsarity checks are unconditionalassert_eq!. Asdebug_assert_eq!they compiled out of release builds,which then indexed past the end of the slice and panicked without the message
the check exists to print.
jitdriver.rs— dropped a redundantclone()of the close greens; thevalue is not used again.
Not addressed here, with reasons
Multi-frame bridge giveup (
jitdriver.rs, Codex P1). majit: split state-field resume frames by liveness and give up on multi-frame bridges #773 gave up onbridges whose resume data spans more than one frame, in the same commit that
fixed the liveness splitter those bridges decode through. The giveup may
therefore now be unnecessary conservatism costing bridge coverage. That is a
behaviour change needing its own measurement, not a review fixup, so it is
left for a separate change together with CodeRabbit's request for
building_bridgeregression tests inoptimizeopt/optimizer.rs.Shared
FRAME_VALUE_COUNT_FN(majit-ir/src/resumedata.rs:20, Codex P1).Reported as thread-local state; it is a process-global
AtomicUsizethatpredates majit: split state-field resume frames by liveness and give up on multi-frame bridges #773 and is already consumed by the cranelift backend. It is only
hazardous with two
#[jit_interp]drivers registering different splitters inone process, which no current configuration does. Making it per-driver is a
cross-crate signature change through
rebuild_from_numbering.Verification
cargo test -p majit-metainterp --features dynasm— 1501 passed, 0 failed.🤖 Generated with Claude Code
Summary by CodeRabbit