jit: FBW bridge always-commit slices — CA adopted-frame completion, orphan stamp default-on, merge-point cut#508
Conversation
|
Warning Review limit reached
Next review available in: 30 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 Run ID: 📒 Files selected for processing (13)
✨ 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: 763ec26416
ℹ️ 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 cross_loop_cut = ctx | ||
| .get_merge_point_at(green_key, ctx.header_pc) | ||
| .filter(|mp| mp.position._pos > 0) |
There was a problem hiding this comment.
When this new unconditional lookup produces a cut for a guard-origin same-key trace (mp.position > 0 while cut_inner_green_key is None), the existing safety check later in compile_loop_body is skipped because it is still gated on cut_inner_green_key.is_some(). For any such cut trace where cross_loop_cut_label_jump_null_guard_slot() would detect a class-guarded LABEL input fed back as Const(NULL), the loop is now installed instead of aborting, which the local comment describes as a NULL dereference on the loop back edge. Please gate that rejection on cross_loop_cut.is_some() (or the same condition introduced here), not only on the old cross-loop marker.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ddb2d73). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64ea55efb4
ℹ️ 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".
| ; mov [Rq(scratch)], Rq(new_free_reg) // *nursery_free = base + 16 | ||
| ; mov [Rq(base_reg)], Rq(value_reg) // value @ base+0 | ||
| ; mov [Rq(base_reg) + 8], Rq(next_reg) // next @ base+8 | ||
| ; mov rax, Rq(base_reg) // result = base |
There was a problem hiding this comment.
When a NurseryAllocRef call takes this fast path under the normal majit GC, it advances the active GC nursery and returns base as a Ref after writing only the two Node payload words. The collector's nursery objects are laid out as GcHeader followed by the payload (Collector::alloc_with_type initializes the header and returns ptr + GcHeader::SIZE), and JIT Ref values are later treated as GC roots; a subsequent collection will therefore subtract a header from this headerless pointer and interpret the Node fields / preceding bytes as a GC header, causing invalid type ids or memory corruption. Please either allocate through a valid typed GC-object layout on the fast path or avoid using the GC nursery/Ref rooting protocol for headerless nodes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa5f6ce66
ℹ️ 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".
| dynasm!(self.mc ; .arch aarch64 | ||
| ; ldr x17, [x16] // x17 = base = *nursery_free | ||
| ; add x14, x17, NURSERY_ALLOC_NODE_SIZE // x14 = newf = base + 16 |
There was a problem hiding this comment.
On AArch64, when NurseryAllocRef takes this new fast path with the normal majit GC installed, it reserves only the 16-byte Node payload and returns x17 as the Ref; the collector's nursery API allocates GcHeader::SIZE + payload and returns the payload pointer after initializing the header. Any later GC root scan or type guard on this returned value will subtract GcHeader::SIZE and interpret the preceding bytes as a GC header, so this corrupts object metadata or crashes under collection. The same fast path was added to AArch64 here, so it needs the same typed/headered layout or must not participate in the GC Ref protocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49901b552a
ℹ️ 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".
| static ITEMS_BLOCK_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(|| { | ||
| build_object_descr_group_with_def_path( | ||
| pyre_object::object_array::ITEMS_BLOCK_ITEMS_OFFSET, | ||
| 0, |
There was a problem hiding this comment.
When items_block_capacity_descr() now supplies a parent SizeDescr, ensure_ptr_info_arg0 can create a StructPtrInfo whose short-preamble guards use SizeDescr.type_id(). This line hardcodes that type id to 0, but object-strategy ItemsBlocks are allocated as PY_OBJECT_ARRAY_GC_TYPE_ID blocks (pyre-object/src/object_array.rs:290, constant value 9), so any optimized bridge/loop that guards a capacity read emits GUARD_GC_TYPE(..., 0) and rejects real list/tuple item blocks. Please use the actual PY_OBJECT_ARRAY_GC_TYPE_ID for this parent descriptor, or mark it non-GC-managed if this is meant to describe the std-alloc fallback.
Useful? React with 👍 / 👎.
Port llhelper/LLHelperEntry annotation+rtyping specialization and the placeholder_sigarg/ADTInterface signature surfaces against annlowlevel.py; add the LLHelper emulated-pbc-call key to the bookkeeper. Surfaces still lacking a Rust carrier keep returning structured pending errors naming the missing primitive. cargo test -p majit-translate --lib: 2983 passed / 0 failed / 4 ignored (+6 new tests over the 2977 baseline). Assisted-by: Claude
try_execute_residual_call_via_executor now returns
ResidualExecOutcome::{Executed, Declined(ResidualDecline)} instead of
Option, distinguishing ValueUnavailable (an argument's concrete value
is unknown to the walker) from Symbolic (deliberate leave-symbolic
contracts). The unjournaled-effect flag splits into the two causes;
fbw_has_unjournaled_effect() keeps its OR semantics, so no consumer
behavior changes.
Count executed residuals per walk (void / may-force / plain, on both
the normal and raised arms) and, under PYRE_FBW_CENSUS, print one
walk-end disposition line per authoritative walk: end variant,
committed, bridge, decline kinds, executed-residual counts, journal
length.
No behavior change with the env var unset. check.py 162/162 on
dynasm, cranelift and wasm; e_min/m4/gcd-kept-while/dict-store repro
outputs unchanged.
Assisted-by: Claude
Behind PYRE_FBW_BRIDGE_STAMP (default off), setup_bridge_sym seeds concrete values onto orphaned bridge-resume colors: colors whose decoded value exists but whose OpRef sits in no semantic-mirror slot, so the deferred overlay stage never re-stamps them and bridge-walk residuals later decline with ValueUnavailable. NONE oprefs, semantic-mirror members, Void and null refs are skipped; the off path allocates nothing. With the gate on, the nested-FOR_ITER varying-trip repro corrects to 10000 and its walk-end census shows the declining bridges committing; the 3-nested gcd repro regresses (2661 to 1073) because concrete residual execution exposes the FOR_ITER in-flight accounting - the known hazard the next slice addresses. Gate off is byte-identical: check.py 162/162 on dynasm, cranelift and wasm; all anchor repros unchanged. Assisted-by: Claude
make_vtable_field_descr minted the typeptr FieldDescr with parent_descr None, so recording a getfield with it would panic the optimizer's ensure_ptr_info_arg0 parent lookup. get_field_descr always attaches parent_descr = get_size_descr(STRUCT, vtable), and for rclass.OBJECT the vtable is a nullptr (descr.py:234-238), so the parent SizeDescr reports is_object() false. Rebuild the singleton as a cyclic group: the OBJECT-modelling SizeDescr (word-sized, no vtable, all_fielddescrs = [typeptr]) owns the field descr, which holds the weak parent backreference. All field values are unchanged; consumers keep the same singleton identity. majit-ir 295, majit-metainterp 1395 tests pass; check.py 162/162 on dynasm, cranelift and wasm; anchor repros unchanged. Assisted-by: Claude
items_block_capacity_descr minted a fresh parentless PyreFieldDescr on every call, so an inlined sub-walk reading the ItemsBlock capacity header (rlist.py:84/251 len(l.items)) aborted at the missing-parent guard rather than record a getfield the optimizer's ensure_ptr_info_arg0 parent lookup would reject - measured as 15 effects-executed-then-replayed walks across the list synth benches. Add ITEMS_BLOCK_DESCR_GROUP (DictStorage-group precedent: header-sized SizeDescr, immutable capacity field at offset 0) and return the cached group entry, which also stabilizes the descr identity. FieldDescrMissingParentDescr census ends drop 5 to 0 on list_ops/list_pop_append/list_reverse; bench outputs and anchor repros unchanged; check.py 162/162 on dynasm, cranelift and wasm. Assisted-by: Claude
The CALL_ASSEMBLER guard-failure slow path runs the bridge hook and then unconditionally runs the blackhole hook from guard-time values. When the bridge walk committed its end-of-walk state and the callee frame adopted it, that guard-state blackhole re-runs the region over the already-advanced heap and its stale return value reaches the CA caller, dropping the committed state. Record the adopted callee frame in a thread-local when the CA bridge walk commits (raise_continue_running_normally analogue, pyjitpl.py:3048-3091 + warmspot.py:970-983); the blackhole hook consumes it and completes the callee from the adopted state via the portal runner instead of the guard-state resume. Uncommitted walks fall through to the legacy path unchanged. Assisted-by: Claude
The orphan-color concrete stamp for divergent bridge resumes was opt-in behind PYRE_FBW_BRIDGE_STAMP while a committed CALL_ASSEMBLER bridge walk could still be dropped by the guard-state blackhole. With that fixed, stamp-on matches stamp-off on every anchor (m4 N-sweep, gcd_kept_while, repro_467) and closes the e_min iteration deficit (9997 -> 10000). Flip the gate to default-on with a =0 opt-out, the same idiom as PYRE_FBW_BRIDGE_LOCAL_SEED. check.py: cranelift and wasm 162/162; dynasm failures limited to the documented nested_loop/raise_catch borderline timing flakes, which show no on/off timing difference when measured directly. Assisted-by: Claude
compile_loop applied the merge-point trace cut only under the cross-loop-cut marker. Upstream compile_loop always compiles from the merge point the first header visit registered (pyjitpl.py:3018-3030 — start and original_boxes come from current_merge_points regardless of where tracing began), which is a no-op for portal traces starting at their own header. A guard-origin bridge walk that closes at its own green key — the target procedure is FINISH-only (a CALL_ASSEMBLER callee) so it has no jumpable front target token and the compile_trace attach path is skipped — registers the header mid-trace. Compiling the whole trace paired the guard's fail-arg inputargs with the merge point's full-shape JUMP: the unroll pass declined with InvalidLoop and the no-unroll retry aborted on the root entry/jump arity mismatch, so the bridge never produced compiled code. Apply the cut unconditionally (position 0 filters itself out); the bridge region now compiles as a root loop for the key. Verified: default anchors unchanged (e_min 10000, m4_300 2661, gcd_kept_while 2663, repro_467 300001; m4 N-sweep stamp-on == off), cargo test --all dynasm green, check.py cranelift/wasm 162/162 and dynasm 161/162 (raise_catch borderline timing flake; the cut fires in neither flaky bench). Assisted-by: Claude
JitInterpConfig::uses_inline_pipeline has no callers. The libc stat type-only re-export triggers non_camel_case_types. Assisted-by: Claude
Assisted-by: Claude
A top-level walk that ends in an uncaught raise records Finish(exc, exit_frame_with_exception) but stashed no finish-concrete, so the terminate-no-replay predicate declined: the journal rolled back and the caller re-ran the region from the guard, double-executing the residuals the walk had already applied (the exception shape of the value-return case). Upstream propagates the exception directly — opimpl_raise → finishframe_exception → compile_exit_frame_with_exception (pyjitpl.py:1688-1698 + 3238-3242) raises ExitFrameWithExceptionRef (jitexc.py:44); nothing re-runs. Widen the finish-concrete stash to a FinishConcrete Return/Raise disposition (single cell keeps the reset/rooting/leak discipline). The three top-level exit-frame-with-exception sites stash the concrete exception when it is a non-null Ref. The bridge consumer surfaces it as BridgeResolution::FinishedException → HandleFailOutcome::BridgeRaised, built with the same PyError::from_exc_object as the blackhole ExitFrameWithExceptionRef path; the two portal consumers return it as the frame's exception result. CA callback stays disarmed. Census: exceptions.py Terminate-bridge non-commit walks 38 → 0; bench outputs identical to the opt-out path. Anchors unchanged (e_min 10000, m4_300 2661, gcd_kept_while 2663, repro_467 300000). cargo test --all dynasm green. Assisted-by: Claude
execute_varargs (pyjitpl.py:1940-1941) opens every residual call with metainterp.clear_exception(); the opimpl_catch_exception assert (pyjitpl.py:504) relies on that invariant. The walker cleared last_exc_value only on the concrete-execution success arm, so declined/folded residual paths kept a stale caught exception and the walk aborted at the next catch_exception/L (CatchExceptionWithActiveException) after effects were executed. Clear at the entry of the three residual dispatch arms instead. Corpus census hazard walks: exception_loop_warmup 9 -> 0. Assisted-by: Claude
The bridge Terminate no-replay shortcut was armed only for the general guard path; the CALL_ASSEMBLER callback returns a bare bool and had no channel for a concrete result, so a CA bridge walk that ran the callee to its finishframe concretely was rolled back and re-run from the guard state by the unconditional blackhole (double-applying the walked region's residual effects). Arm the shortcut for any single-frame resume. The CA callback leaves a kept stash in its GC-rooted cell, records the callee frame in a CA_WALK_FINISHED_FRAME handshake (the CA_WALK_ADOPTED_FRAME pattern), and returns ResumeBlackhole; jit_blackhole_resume_from_guard consumes the stash to complete the callee — Return becomes the call result, Raise publishes the exception (publish_residual_call_exception) — matching the DoneWithThisFrame/ExitFrameWithExceptionRef an assembler caller catches when a retrace reaches finishframe. Corpus census: Terminate-bridge non-commit walks 1 -> 0 (generator_tree_recursion); e_min with PYRE_FBW_BRIDGE_STAMP=0 moves 9997 -> 10000. Assisted-by: Claude
stack_effects fell through to the conservative same-depth default for 24 instructions that really move the operand stack, so depth_at_py_pc drifted upward past each occurrence. CALL_FUNCTION_EX (pop 4, push 1) alone inflated the depth by +3; two of them before a hot while loop reported depth 6 at a header whose real stack is empty, and the walk-end flush read six phantom NULL stack slots and declined (seqiter_pickle_parity CloseLoop non-commit walk). Add explicit arms mirroring the interpreter implementations in pyre-interpreter eval.rs/pyopcode.rs: CallFunctionEx -3, CallKw -(argc+2), CallIntrinsic1 0 / CallIntrinsic2 -1, BuildSlice -1/-2, BuildTemplate -1, BuildInterpolation -1/-2, UnpackEx -1+before+1+after, FormatWithSpec -1, EndAsyncFor/CleanupThrow -2, WithExceptStart/ GetAnext/LoadCommonConstant/ReturnGenerator +1, and net-0 arms for ConvertValue/FormatSimple/ToBool/LoadFromDictOrDeref/ LoadFromDictOrGlobals/MakeFunction/MakeCell/SetupAnnotations/ ExitInitCheck/Send. Send is also registered as a branch instruction in the forward worklist and target_pc (equal depth on both paths). The remaining default-arm variants are adaptive specializations, Instrumented* forms, and internal dispatch controls that never appear in canonical bytecode. Corpus census hazard walks 4 -> 3; the seqiter_pickle_parity CloseLoop walk now commits. Anchors unchanged. Assisted-by: Claude
BranchGuardKeptStackUnsupported walks (user-defined-iterator FOR_ITER loops) executed their residual calls concretely and then replayed from loop entry on abort. The abort maps to the FOR_ITER header py-pc, but the in-flight entry there is stale: entries are replaced on the next successful consume and never popped at body completion, so an abort firing during the NEXT consume attempt still holds the prior item whose body already ran — Shape-A delivery would re-run that body (seqiter repro returned 200004/800024). - InflightForiter.body_completed, set by fbw_foriter_inflight_mark_attempt at the start of every for_iter_next residual dispatch for the same body_pc (pre-call, so a mid-attempt abort still records it). - fbw_foriter_inflight_take_for_resume refuses completed entries; delivery stays a fresh-item continuation only. - New no-delivery arm in the branch-flush leg, Unsupported variant only: a body-completed header entry with no effect since the consume adopts the walk end state via the same all-or-nothing flush; the interpreter re-attempts the consume against the advanced iterator. The Permanent variant's behavior is unchanged. - The Unsupported raise site stamps abort_in_subwalk like the Permanent one, so a callee-coordinate abort cannot flush the outer frame. Corpus hazard census 3 -> 1 (list_ops only). iteration_protocol and seqiter_getitem_lazy outputs byte-identical to the interpreter; anchors e_min 10000 / m4_300 2661 / gcd_kept_while 2663 / repro_467 300000 unchanged; cargo test green; check.py 162/162 x3 backends. Assisted-by: Claude
PYRE_FBW_DEBUG_ABORT now prints op_pc, arg_index, helper kind and call opcode when a residual call declines with ValueUnavailable (NO_CONCRETE Ref or box_value None), so a census run can attribute the unj_val walks without re-instrumenting. Debug-gated output only; no behavior change. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3c6ef076d
ℹ️ 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".
| SimpleSizeDescr::new(u32::MAX, std::mem::size_of::<usize>(), 0) | ||
| .with_all_fielddescrs(all_fielddescrs) |
There was a problem hiding this comment.
When a non-virtual heap object reaches a getfield_gc_* read of object.typeptr, this new parent descriptor makes ensure_ptr_info_arg0 install a StructPtrInfo; StructPtrInfo.make_guards then emits GUARD_GC_TYPE using this parent SizeDescr's type id (0). Most real PyObjects are allocated with their concrete GC header ids (e.g. ints/lists/types), not the OBJECT root id, so short-preamble or bridge guards derived from a typeptr read will reject valid objects. Please either avoid creating a GC-managed StructPtrInfo for the typeptr header field (as with the w_class special case) or mark this synthetic OBJECT parent as not GC-managed so it only proves non-nullness.
Useful? React with 👍 / 👎.
1 participant
Epic #493 (FBW always-commit protocol) slice series, plus supporting descr/observability work. Rebased onto main (#509 WalkContext refactor).
What's in here
ResidualExecOutcomesplit (ValueUnavailable/Symbolic), executed-residual counters,PYRE_FBW_CENSUSwalk-end line. Observability only.PYRE_FBW_BRIDGE_STAMP).ItemsBlockcapacity read), draining theFieldDescrMissingParentDescrwalk-abort class.raise_continue_running_normallyparity, pyjitpl.py:3048-3091 + warmspot.py:970-983).PYRE_FBW_BRIDGE_STAMP=0opts out.compile_loopalways compiles from the registered merge point (pyjitpl.py:3018-3030). Pyre gated the cut on the cross-loop marker, so a guard-origin bridge closing at its own key never compiled. The cut is now unconditional; portal traces (position 0) are unaffected.execute_varargsclears the exception register at every residual-call entry (pyjitpl.py:1940-1941; catch_exception assert pyjitpl.py:504); pyre cleared it only on the concrete-exec success arm, so a stale exception declined the next residual call (CatchExceptionWithActiveExceptionclass, 9 → 0).DoneWithThisFrame/ExitFrameWithExceptionRefparity, pyjitpl.py:1688-1698). Also closes thePYRE_FBW_BRIDGE_STAMP=0e_min deficit (9997 → 10000).stack_effectsdefault arm modeled 24 real stack-moving opcodes (CALL_FUNCTION_EX net −3, CALL_KW −(argc+2), UNPACK_EX, …) as net-0, inflatingdepth_at_py_pcpast each occurrence; the walk-end flush then read phantom NULL stack slots and declined (CloseLoop non-commit class). All 24 modeled against theeval.rsimplementations;Sendregistered as a branch instruction.body_completedtracking marked at everyfor_iter_nextdispatch start; the delivery selector refuses completed entries; a new no-delivery flush arm (Unsupported variant only) adopts the walk end state at the header and lets the interpreter re-attempt the consume. The Permanent variant is byte-identical.PYRE_FBW_DEBUG_ABORTprints op_pc/arg/helper when a residual declines with ValueUnavailable, attributing the remaining unj_val census walks without re-instrumenting.Verification
cargo test --all --features dynasmgreen.PYRE_FBW_CENSUS=1): non-committed effectful walks 77 → 1 across the series (remaining: list_ops CompileTracePending+unj — box_value-None on residual args, the Epic: FBW always-commit protocol — retire replay-from-entry #493 P1 epic).Full root-cause records: #493 comments 6-17.
🤖 Generated with Claude Code