gc: separate a failed managed allocation from an absent GC - #1020
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughGC allocation hooks now distinguish no route, successful allocation, and managed allocation failure. Frame blocks and RBigInt allocations abort on managed failure instead of falling back to untraced raw allocation. ChangesGC allocation failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 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 9e84bc9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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
https://github.com/youknowone/pyre/blob/0383073e97d8176156a27564fef5a6f13ff06667/pyre-object/src/gc_hook.rs#L182
When an installed GC hook returns null—for example when an old-generation spill cannot allocate—this arm reaches handle_alloc_error and terminates the process. The upstream behavior documented immediately below is to raise MemoryError, so the new RBigInt and frame-block paths turn a catchable Python allocation failure into an interpreter crash; thread the failure through the interpreter/JIT exception protocol rather than aborting.
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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pyre/pyre-object/src/gc_hook.rs`:
- Around line 163-172: Update GcAllocOutcome::from_hook and the
stable-allocation flow so an absent GC_ALLOC_STABLE_HOOK is distinguishable from
an inactive or unsupported GC when an independent GC is active; do not classify
that case as NoRoute or fall through to malloc_raw. Apply the corresponding
handling at pyre/pyre-object/src/gc_hook.rs:318-328 and preserve the required
RBigInt allocation behavior at pyre/pyre-object/src/rbigint.rs:698. Extend the
independent-hook test to cover the missing-stable-route fallback, including the
untraced headerless RBigInt/_digits case.
- Around line 175-201: Preserve managed allocation failures as exception-capable
results rather than aborting: update gc_alloc_failed and allocated_or_abort in
pyre/pyre-object/src/gc_hook.rs:175-201, propagate the result through
try_gc_alloc_stable_or_abort in pyre/pyre-object/src/gc_hook.rs:318-328, and
through the affected rbigint allocation paths at
pyre/pyre-object/src/rbigint.rs:593-600, 649-657, and 698 and frame-block
allocation in pyre/pyre-interpreter/src/pyframe.rs:1088-1091. Ensure generated
JIT execution observes MemoryError when malloc_raw is disabled, and add a test
covering that path.
🪄 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: da07407a-5262-47bf-ba8f-96d0b912d9f5
📒 Files selected for processing (3)
pyre/pyre-interpreter/src/pyframe.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/rbigint.rs
|
The same job is already
Comparing the failing sets keyed on backend + fixture + the counter delta itself, not just the fixture name: Byte-identical, e.g. I have not re-recorded any baseline to make this green. The counters moved for reasons that belong to the base, and re-recording would bury a real signal that is not mine to bury. Locally this branch was green on base — commented by Claude |
|
All checks have settled. Extending the earlier control to all three platforms: every
Sets compared on backend + fixture + the counter delta text, not the fixture name alone. All 87 entries are a single category, Everything else passes (15/15): No baseline was re-recorded. The counters moved for reasons owned by the base, and re-recording them here would bury a signal that is not mine to bury. — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06cbb4fe0
ℹ️ 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".
| pub fn gc_alloc_failed(payload_size: usize) -> ! { | ||
| let layout = std::alloc::Layout::from_size_align(payload_size, std::mem::align_of::<usize>()) | ||
| .unwrap_or_else(|_| std::alloc::Layout::new::<usize>()); | ||
| std::alloc::handle_alloc_error(layout) |
There was a problem hiding this comment.
When an active-GC allocation returns null—for example, after a nursery-full allocation cannot spill to old-gen—this non-returning call terminates the process rather than producing the MemoryError that the preceding comment identifies as the upstream behavior. Python code cannot catch the failure, and treating JIT unwinding as the reason for changing semantics is a generation defect to address rather than a valid behavioral deviation; propagate an interpreter exception through the residual/JIT path instead.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
|
Force-pushed: rebased onto Why the re-record went away rather than getting narrowed to its improvements: main's
Each of those was measured against a tree without #945, so writing it on top of #945 would pin a number that does not describe the tree — and two of the four move in the bad direction anyway. This also supersedes the inherited-red report I posted earlier: the 24/38/25 — commented by Claude |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pyre/pyre-object/src/gc_hook.rs`:
- Around line 994-1001: Update
installed_hook_returning_null_classifies_as_failure_not_no_route so
clear_gc_alloc_hook() runs immediately after try_gc_alloc_fast returns and
before converting or asserting the outcome. Preserve the existing Failed
classification assertion while ensuring the assertion does not depend on a
registered GC_ALLOC_FAST_HOOK.
🪄 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: 21107846-1b14-4b90-acaf-5311b346cfeb
📒 Files selected for processing (3)
pyre/pyre-interpreter/src/pyframe.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/rbigint.rs
| #[test] | ||
| fn installed_hook_returning_null_classifies_as_failure_not_no_route() { | ||
| let _hook_lock = hook_test_guard(); | ||
| register_gc_alloc_hook(null_hook); | ||
| let outcome = GcAllocOutcome::from_hook(unsafe { try_gc_alloc_fast(1, 8) }); | ||
| clear_gc_alloc_hook(); | ||
| assert_eq!(outcome, GcAllocOutcome::Failed); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git rev-parse --show-toplevel >/dev/null
rg -n -C 3 --glob '*.rs' 'GC_ALLOC_FAST_HOOK|clear_gc_alloc_fast_hook|register_gc_alloc_fast_hook|fn try_gc_alloc_fast\b' pyre majitRepository: youknowone/pyre
Length of output: 2753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gc_hook outline relevant =="
ast-grep outline pyre/pyre-object/src/gc_hook.rs --view expanded | sed -n '1,220p'
echo "== gc_hook lines 80-250 =="
sed -n '80,250p' pyre/pyre-object/src/gc_hook.rs | nl -ba -v80
echo "== tests around hook =="
sed -n '960,1025p' pyre/pyre-object/src/gc_hook.rs | nl -ba -v960
echo "== sibling hooks and tests =="
rg -n -C 4 'clear_gc_alloc_hook|clear_gc_alloc_with_placement_hook|no_collect_placement_hook_has_conservative_fallback|installed_hook_returning_null' pyre/pyre-object/src/gc_hook.rs
echo "== fast hook usages outside pyre-object =="
rg -n -C 3 --glob '*.rs' 'register_gc_alloc_fast_hook|clear_gc_alloc_fast_hook|try_gc_alloc_fast' pyre majitRepository: youknowone/pyre
Length of output: 5933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gc_hook lines 80-250 =="
sed -n '80,250p' pyre/pyre-object/src/gc_hook.rs
echo "== tests around hook and sibling fallback =="
sed -n '960,1100p' pyre/pyre-object/src/gc_hook.rs
echo "== fast hook usages outside pyre-object =="
rg -n -C 3 --glob '*.rs' 'register_gc_alloc_fast_hook|clear_gc_alloc_fast_hook|try_gc_alloc_fast' pyre majit
echo "== all fast-allocation hook registrations/clears in tracked Rust =="
rg -n --glob '*.rs' 'register_gc_alloc_fast_hook|clear_gc_alloc_fast_hook' .Repository: youknowone/pyre
Length of output: 18051
Clear the fast-allocation hook before asserting the classification.
try_gc_alloc_fast reads GC_ALLOC_FAST_HOOK before falling back to try_gc_alloc, so leaving that hook registered makes this test depend on an untested hook state. Clear it explicitly before asserting the expected behavior.
♻️ Proposed test hardening
fn installed_hook_returning_null_classifies_as_failure_not_no_route() {
let _hook_lock = hook_test_guard();
+ clear_gc_alloc_fast_hook();
register_gc_alloc_hook(null_hook);
let outcome = Gc_allocOutcome::from_hook(unsafe { try_gc_alloc_fast(1, 8) });
clear_gc_alloc_hook();
assert_eq!(outcome, GcAllocOutcome::Failed);
}🤖 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-object/src/gc_hook.rs` around lines 994 - 1001, Update
installed_hook_returning_null_classifies_as_failure_not_no_route so
clear_gc_alloc_hook() runs immediately after try_gc_alloc_fast returns and
before converting or asserting the outcome. Preserve the existing Failed
classification assertion while ensuring the assertion does not depend on a
registered GC_ALLOC_FAST_HOOK.
|
Correction to my previous comment. I wrote that all four DIFF files "were measured against a tree without #945, so writing them on top of #945 would pin a number that does not describe the tree." That is wrong for three of the four.
A file #945 never touched is not stale by construction, so only the last row was the case I described. And a bracketed local run at the new base ( What that does not change: they are still not recorded on this branch. Each moves a counter the wrong way — They are inherited regardless: main's own run at — commented by Claude |
|
Set diff against main's own run at this branch's base (
Per-backend counts match exactly too (ubuntu dynasm 21 / cranelift 21 / wasm 4; windows dynasm 21 / cranelift 22). Nothing was re-recorded, because at this base no row is an improvement. Every moving row carries at least one counter in the bad direction:
I verified A local bracketed run ( — commented by Claude |
|
Revised — two rows were recordable, and are now recorded ( Attribution, by set-diffing
The decisive fact for the two recorded rows: #945 wrote the identical move into the dynasm and cranelift files of both fixtures and left the wasm file at #1009's values.
So the wasm rows are the same event on the third backend, not a new one. Left unrecorded, deliberately:
Since my earlier comments, main's #1016 ( Verification, bracketed ( — commented by Claude |
`try_gc_alloc*` returns `None` when no hook is installed and `Some(null)`
when an installed hook could not satisfy the request.
`alloc_rbigint_nursery_impl`, `alloc_rbigint_nursery_collecting_impl`,
`alloc_rbigint_stable` and `alloc_frame_block` collapsed the two and fell
back to `lltype::malloc_raw` for both. `W_LongObject` registers
`LONG_VALUE_OFFSET` and `FrameBlock` registers `previous` as gc-pointer
offsets whose walkers carry no `try_gc_owns_object` guard, so a raw payload
reached through either is forwarded as though it had a header.
`try_alloc_with_type_no_collect_body` returns `GcRef(0)` once the nursery
bump and `spill_to_oldgen_or_null` both fail.
Add `GcAllocOutcome::{Allocated, Failed, NoRoute}` with `from_hook` and
`allocated_or_abort`, and `try_gc_alloc_stable_or_abort` for the raw-return
callers. `Failed` reaches `gc_alloc_failed`, which calls `handle_alloc_error`
as `alloc_typed_items_block_nursery` already does for the digit array;
`NoRoute` alone returns the caller to its `malloc_raw` path.
`gc_alloc_storage_box`, `clone_debugdata_ptr`, `getorcreate_debug_data` and
`alloc_dict_object` keep the collapsed form: the first three are guarded by
`try_gc_owns_object` at their walkers, and the last falls back to
header-bearing `malloc_typed`.
Replace the module doc's statement that the `Box::into_raw` fallback is
dropped as the hook's reliability is verified under the bench suite.
Assisted-by: Claude
|
Reply to the Codex P1 and the two CodeRabbit findings, all three of which land on the same arm. 1. "Propagate MemoryError instead of aborting" — the premise does not hold for the arm this diff changesBefore this PR that arm did not raise The repo already has a two-tier convention for exactly this question, and the change follows it rather than inventing one:
So after this PR the RBigInt payload fails the same way its own digit array already failed. Threading Upstream never has this state: 2. "An absent
|
1 participant
try_gc_alloc*returnsNonewhen no hook is installed andSome(null)when aninstalled hook could not satisfy the request. Four sites collapsed the two with
.filter(|p| !p.is_null())and fell back tolltype::malloc_rawfor both, so afailed managed allocation silently produced a headerless object.
alloc_rbigint_nursery_impl_digitsW_LongObject'sLONG_VALUE_OFFSET— plain offset walker, notry_gc_owns_objectguardalloc_rbigint_nursery_collecting_implalloc_rbigint_stablealloc_frame_blockpreviouspreviousis the unguarded offset walkerReachable, not theoretical:
try_alloc_with_type_no_collect_bodyreturnsGcRef(0)once the nursery bump andspill_to_oldgen_or_nullboth fail. Theresult is a
Box::into_rawpayload forwarded as though it carried a header —the failure class behind the SIGBUS fixed in #994.
Change
GcAllocOutcome::{Allocated, Failed, NoRoute}withfrom_hookandallocated_or_abort, plustry_gc_alloc_stable_or_abortfor thedont_look_insideraw-return callers.Failedreachesgc_alloc_failed(
#[cold] #[inline(never)] -> !), which callshandle_alloc_errorexactly asalloc_typed_items_block_nurseryalready does for the digit array — so anrbigint payload now fails the same way its own digits do.
NoRoutealonereturns the caller to its
malloc_rawpath.Upstream has neither state: the GC is a prebuilt constant
(
framework.py:254), and a nursery that cannot satisfy the request reachescollect_and_reserve(incminimark.py:981-985), which raises MemoryErrorrather than returning null. Abort rather than panic because these run under JIT
frames that cannot unwind.
What makes the abort safe:
init_gc_subsystemrunsinstall_gc_into_backend()before
install_pyre_object_hooks, so once a pyre-object hook is visible theset_active_*cells are too —Some(null)can never mean "no backend".Deliberately unchanged
Sites whose mixed GC/raw population is supported and guarded, judged by the
walker rather than by the fallback spelling:
gc_alloc_storage_box—set_object_custom_traceguardsset.itemswithtry_gc_owns_object("A no-GC-hook fallback allocation is notcollector-owned"). I changed this one first and reverted it.
clone_debugdata_ptr/getorcreate_debug_data— guarded, with a deliberatein-place walk for Box payloads.
alloc_dict_object,w_long_new— fall back tomalloc_typed, whichprepends a
GcHeader.Also replaces the module doc's claim that the
Box::into_rawfallback isdropped "as the hook's reliability is verified under the full bench suite". That
criterion cannot be met: the state that matters is
Some(null), which a greenbenchmark never exercises.
Verification
Green on base
7b0aca4bc8e:check.pydynasm 371/371, cranelift 371/371, wasm367/367, with HEAD unchanged across the run.
On the current base,
extract-llbc.py, the release build, andpyre-object312 /
pyre-interpreter490 /majit-gc231 all pass. The full localcheck.pywas not completed on this base: the shared working tree wasrebased twice mid-run (13:07 and 15:17), the second time pulling in #1019, which
re-records jit-stats baselines — so the comparison's baseline changed under a
binary built from the previous base. Those runs were discarded rather than
reported as results. CI is the gate for this base.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests