Repository navigation
fib_recursive: depth-2 recursive inline (gh#343) + PYPY_GC_MAX bounded-heap MemoryError (Task#3) - #674
Conversation
TO_BOOL emits no JitCode, so the forward walk never lands on it, but a guard-capture reconcile flagged after_residual_call passes an explicit resume py_pc that can equal the TO_BOOL pc. The `_ => Unmodeled` fallthrough then decoded TO_BOOL as the previous opcode and set vstack_valid = false, so an operand-stack value kept below a residual-call condition (`s = s + (i if f(...) else 0)`) became unrestorable at the following branch guard and its store was dropped once at the compile-threshold iteration. Classify Instruction::ToBool as PopOnlyOrSideStore (net 0): truncate to the unchanged depth and keep the surviving boxes, including the operand slots below the tested value. TO_BOOL's result is always consumed by the immediately following POP_JUMP_IF_*, so its untracked TOS box is inert. Assisted-by: Claude
Flip fbw_rec_multiframe_enabled() default from OFF to ON. Primary-trace self-recursive Python calls now route through the multiframe inline path while the framestack depth is below fbw_max_multiframe_depth (default 1), instead of folding the first self-recursive occurrence straight to the recursive portal CALL_ASSEMBLER. `=0`/`false` opts out. Update the flag docstring and the self-recursive call-site comment to match. check.py green: dynasm 223/223, cranelift 223/223, wasm 222/222. fib(32) dynasm: 0.146s (flag off) -> 0.093s (default on). Assisted-by: Claude
When walker_abort_if_mayforce_null_ref_arg raises MayForceNullRefArgUnsupported, dump the failing pc, call opcode, pyre helper, arg index, arg count, funcbox, and each arg's type/opref/value. Gated on PYRE_P2_DIAG and computed only on the abort path. Assisted-by: Claude
…ested-inline pause emit_new_pyframe_inline_with_params seeds an inlined-callee frame's locals_cells_stack_w virtual array with locals only; its operand-stack slots kept the NewArrayClear zero-fill. When a guard fired inside a nested inline (the paused caller being the middle frame at depth>=2), the multi-frame snapshot numbered those unwritten operand slots to NULLREF and reconstruct_inline_recipe decoded them to concrete-NULL Ref, aborting the reconstructed middle frame with MayForceNullRefArgUnsupported at its next CALL_MAY_FORCE. compute_nested_inline_caller_frame now, at the point it pauses a frame to enter a nested inline, records SetarrayitemGc plus the matching heapcache_setarrayitem for each live non-local, non-pending operand slot of that frame, materializing the operand register boxes into the virtual array before any in-callee guard numbers it. Mirrors the locals seed at helpers.rs:1414-1433 (same struct op descr / vinfo item-descr cache-key split) and virtualizable.py:101-113 write_boxes force semantics. Assisted-by: Claude
…framestack walk drive_bridge_framestack_walk aborted (P2Framestack::NoRecipes) when a bridge carrier had no recipes: a guard failure whose multi-frame snapshot held only the root frame, with no paused inline callee to reconstruct. Such a carrier is state-equivalent to a no-carrier guard failure, the root sym being seeded by setup_bridge_sym independent of the carrier. Fall through to full_body_walk_trace from the root pc, mirroring the carrier.is_none() plain-bridge path (resume.py:1042-1057 rebuilds a no-inline-frame resume as the root and continues), unless the key is fbw_declined or PYRE_FULL_BODY_WALK=0. Assisted-by: Claude
…_inline_recipe (gh#343) The vable-callee gate required reg_indices.ref_ to carry both the frame red and the ec portal red. When an in_a_call callee is suspended at one of its own CALL sites and its pending result shares the ec portal red color, removing the pending value from the ref bank leaves ref_ with only the frame red. pec_reg is consumed solely by this classifier gate; the reconstruction body threads pframe_reg -> frame_vidx -> the locals_cells_stack_w array and never reads ec. Accept this in_call_portal reduced form (in_a_call, pending value present, single frame red) alongside the full two-ref portal shape. Assisted-by: Claude
The two bool fields each occupied a full 8-byte slot under repr(C) padding. Replace `escaped` in place with a `flags: u8` carrying FLAG_ESCAPED and FLAG_FRAME_FINISHED, remove the standalone `frame_finished_execution` field, and route all reads/writes through escaped()/set_escaped()/frame_finished_execution()/ set_frame_finished_execution(). The removed field sat after vable_token, so only the cold trailing fields shift and the JIT-baked early offsets are unchanged. size_of::<PyFrame>() 136 -> 128. Assisted-by: Claude
The helper implemented PyPy's _same_static_method staticmethod unwrap (objectobject.py:113-116) for object.__new__ override detection. pyre targets CPython 3.14, which keeps a staticmethod-wrapped __new__ as a distinct tp_new slot — an override — so `Cls(1, 2)` for `__new__ = staticmethod(object.__new__)` raises "object.__new__() takes exactly one argument", not "Cls() takes no arguments". The object __new__/__init__ excess-args checks therefore use raw slot identity (same_inherited_slot) and never called this helper. Remove it and document the CPython-vs-PyPy divergence at the comparison site. Assisted-by: Claude
… message
object.__dir__ called with no receiver indexed args[0] and panicked; guard
the receiver ("unbound method object.__dir__() needs an argument") and reject
surplus positionals ("object.__dir__() takes no arguments (N given)").
super_check's rejection now names the operands — "obj (instance of str)" for
a non-type, "obj (type str)" for a type, and "type (int)" for the start type
— matching CPython 3.14.
Assisted-by: Claude
`recursive_decision` and `should_inline_core` computed `callee_compiled` from `compiled_loops` + `pending_token` only, while the sibling `recursive_target` resolver returns a token via `warm_state.get_compiled` (which includes the tmp-callback tokens `get_assembler_token` installs). A callee whose only token lived on a warm cell was reported as uncompiled and took the ResidualCall/abort leg even though `recursive_target` would have resolved a token. Add the `warm_state.get_compiled` disjunct at both decision sites so the inline decision reads the same token source as the resolver. check.py dynasm 225/225 with no jit-stats change; majit-metainterp lib tests 1396 pass. Assisted-by: Claude
finish_incremental_cycle dropped the `bounded` result of
set_major_threshold_from, leaving the PYPY_GC_MAX heap cap advisory. Add
`max_heap_size_already_raised` and `oom_pending` fields and carry the
triggering allocation size (`pending_reserving_size`) across the nursery-full
collection. When a bounded major collection leaves the heap over
`max_heap_size`, set `oom_pending` so the triggering `alloc_with_type`
returns NULL — the NULL the backend `CHECK_MEMORY_ERROR` path already
converts to `MemoryError`; a second occurrence aborts ("using too much
memory, aborting"), matching incminimark.py:2601-2615. Inert in the default
unbounded config (`max_heap_size == 0` never sets `bounded`); JIT slow-path
and interpreter-chokepoint handling of the NULL land in follow-up slices.
majit-gc lib tests 192 pass plus two new bounded/unbounded OOM policy tests.
Assisted-by: Claude
…e's exception table A PropagateExceptionDescr exit (CHECK_MEMORY_ERROR's propagate_exception_path) returned the pending exception straight out of the frame as Done(Err), bypassing the frame's own exception table; the blackhole-resume path instead unwinds it during resume. As a result a same-frame try/except did not catch a compiled-loop exception such as a PYPY_GC_MAX MemoryError, though a caller frame still did. Offer such exits to handle_exception first, resuming at the handler pc on a match and propagating otherwise: - add LoopResult::ExitFrameWithException; execute_assembler returns it and eval_loop_jit's maybe_compile_and_run site routes it through handle_exception inline (continue on catch). - deliver_exit_frame_exception resolves the compile-once path (handle_jit_outcome) the same way, resuming via handle_jitexception on a match. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 17 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 (19)
✨ 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 |
Extract emit_memory_error_check from the CheckMemoryError arm and also call it after the four inline/collecting nursery allocation arms (CallMallocNursery, CallMallocNurseryHeaderless, CallMallocNurseryVarsize, CallMallocNurseryVarsizeFrame). Their slow paths return GcRef(0) when a bounded PYPY_GC_MAX major collection is out of memory; previously only CheckMemoryError guarded a NULL, so an inline-bump arm wrote the header / jf_gcmap field to the null pointer and crashed. The check branches to the propagate_exception_descr exit on NULL, matching the dynasm backend's emit_propagate_memory_error_if_null placement on the same arms. For CallMallocNurseryVarsizeFrame the check precedes the jf_gcmap init store, which dereferences the frame pointer. Assisted-by: Claude
a510e89 to
e883a68
Compare
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 96c6308). Files in the reviewed diff1. 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
|
![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: a510e890c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return TraceAction::Abort; | ||
| } | ||
| crate::jitcode_dispatch::census_record("P2Framestack::NoRecipesPlainBridge"); | ||
| return full_body_walk_trace(ctx, sym, w_code, root_pc, cf_addr, WalkJournals::Reset); |
There was a problem hiding this comment.
In this 0-recipe carrier path, root_pc is still the carrier's JitCode pc: trace_bytecode sets the carrier start from c.root_pc and separately back-translates it for lasti_pc, and the P2Drain path does the same before calling full_body_walk_trace. full_body_walk_trace and make_green_key expect a Python pc, so a base-case guard with no inline frames will start or decline the bridge at the wrong coordinate whenever the pc map is not identity, leading to bad bridges or losing the intended depth-2 payoff.
Useful? React with 👍 / 👎.
| // chokepoint. Never taken in the unbounded default (`PYPY_GC_MAX` | ||
| // unset), so the fallback below is unchanged there. | ||
| if std::mem::take(&mut self.oom_pending) { | ||
| return GcRef(0); |
There was a problem hiding this comment.
When this new bounded-heap path is reached through the collecting pyre-object hook, returning GcRef(0) does not actually raise MemoryError: pyre_object_gc_alloc_collecting_trampoline forwards the null, and callers such as alloc_bigint_nursery_collecting filter null and fall back to the no-collect/raw allocation path (pyre/pyre-object/src/longobject.rs:159-169). With PYPY_GC_MAX set, those allocations can therefore continue past the cap instead of surfacing the OOM signal; the signal needs to reach callers as an error or use a path they cannot treat as “GC unavailable”.
Useful? React with 👍 / 👎.
…trike On the first PYPY_GC_MAX breach, finish_incremental_cycle set oom_pending and fell through to execute_finalizer_triggers before the pending NULL raised MemoryError, so queued finalizer notifications ran ahead of the error. incminimark.py:2614-2615 sets STATE_SCANNING and raises immediately, exiting major_collection_step before the finalizing phase. Hoist the Scanning state assignment above the breach check and return before the triggers on the breach path. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96c63088bf
ℹ️ 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".
| self.pending_reserving_size = total_size; | ||
| self.do_collect_nursery(); |
There was a problem hiding this comment.
When PYPY_GC_MAX is set and the heap is just below the capped threshold, this saved pending_reserving_size is only read if a major cycle actually reaches finish_incremental_cycle. The nursery collection called here still goes through run_major_progress_after_minor(), which returns early on threshold_reached(0) while gc_state == Scanning, so an allocation whose own size crosses the cap does not start the major cycle and never sets oom_pending; it succeeds until some later allocation happens to exceed the threshold without the reservation. Thread this size into the threshold gate, matching incminimark's minor_collection_with_major_progress(extrasize) path.
Useful? React with 👍 / 👎.
Rebased onto
main(#671). 13 commits in three groups.gh#343 — depth-2 self-recursive inline
_ => Unmodeledfallthrough latchedvstack_valid = falseand dropped an operand kept below a residual-call condition. Model TO_BOOL asPopOnlyOrSideStore.PYRE_FBW_REC_MULTIFRAME— inline self-recursive Python calls below the depth bound before folding the deepest call to the recursive portalCALL_ASSEMBLER, matchingopimpl_recursive_call/do_recursive_call; the prior fold-only default was the deviation.PYRE_P2_DIAGprobe — abort-path-only diagnostic pinpointing which Ref arg folded to concrete NULL at a mid-bodyCALL_MAY_FORCE.NULLREF, aborting the reconstructed middle at its nextCALL_MAY_FORCE; materialize the live operand boxes into the frame array before the nested body walk.reconstruct_inline_recipe.Task #3 —
PYPY_GC_MAXbounded-heap OOM → catchableMemoryErrorfinish_incremental_cycle— onbounded && threshold_reached, first returnGcRef(0), second strikepanic!(incminimark parity).ExitFrameWithExceptionthrough the frame's exception table — a JIT-loop OOM now delivers aMemoryErrorthat a same-frametry/exceptcatches (previously only a caller frame caught it), via the CPython-3.14 exception table (handle_exception).MemoryErroron the nursery malloc arms' NULL return — the four inline/collecting nursery allocation arms lacked the inline null→propagate check thatmalloc_condinlines (and dynasm already emits), so a bounded-OOMGcRef(0)wrote a header to a null pointer and crashed. Extractemit_memory_error_checkand apply it toCallMallocNursery{,Headerless,Varsize,VarsizeFrame}.Layout / parity
escapedandframe_finished_executioninto a flags byte.object.__dir__arg-count guard andsuper(type, obj)message; typedef: remove unusedobject_slot_inheritedhelper.warm_state.get_compiledinto recursivecallee_compiled.Verification
pyre/check.py --backend dynasm→ 225/225pyre/check.py --backend cranelift→ 225/225PYPY_GC_MAXOOM repros (same-frame / caller-frame / nested / varsize / bare) catchMemoryError(or print a proper traceback) on both backends.🤖 Generated with Claude Code