jit: trace the cranelift force token as a GC slot, plus _pickle and JitCodeBuilder follow-ups - #1084
Conversation
WalkthroughThe PR updates GC-managed tracing-rescall tokens, JIT root tracing, compilation timing, struct-layout caching, bridge diagnostics, and cached pickle writers. ChangesGC tracing and virtualizable tokens
Cached pickle writers
JIT compilation and layout instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JIT as pyre-jit GC initialization
participant VirtualRef as virtualref
participant GC as GC heap
participant Frame as PyFrame tracing
JIT->>VirtualRef: register sentinel GC type
VirtualRef->>GC: allocate and root tracing sentinel
VirtualRef-->>Frame: provide tracing-rescall token
Frame->>Frame: forward vable_token
Frame->>GC: trace virtual_token and forced
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 b713cb9). 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
Here are some automated review suggestions for this pull request.
Reviewed commit: c1966c6182
ℹ️ 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".
| if let Some(write_slot) = self.write_slot { | ||
| call_fn( | ||
| pyre_object::gc_roots::shadow_stack_get(write_slot), | ||
| &[pyre_object::gc_roots::shadow_stack_get(bytes_slot)], | ||
| )?; |
There was a problem hiding this comment.
When a custom output stream replaces its write attribute after Pickler construction, this calls the previously bound method rather than the current attribute. The old call_meth path matched pypy/module/_pickle/interp_pickle.py:353-354, where every flush uses space.call_method(self.w_file, 'write', ...); retain that dynamic lookup instead of caching the callable.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Probed rather than assumed, because this is a behaviour claim rather than a structural one — and the two references disagree.
interp_pickle.py does re-resolve per write, as quoted. But _pickle is an accelerator whose observable behaviour has to match CPython, and both CPython implementations capture file.write once at construction: _Pickler_SetOutputStream in the C accelerator, and pickle.py:465 (self._file_write = file.write, handed to _Framer.__init__) in the pure-Python class. Note interp_pickle.py:555-560 already resolves write in __init__ — it just discards the result and re-resolves in _Framer.file_write (:353).
Rebinding file.write after construction and then calling dump():
pickle.Pickler |
pickle._Pickler |
|
|---|---|---|
| CPython 3.14.5 | orig=1 rebound=0 — cached |
orig=3 rebound=0 — cached |
| pypy3 3.11.15 | orig=0 rebound=3 — re-resolved |
orig=3 rebound=0 — cached |
| pyre, this branch | orig=1 rebound=0 — cached |
orig=3 rebound=0 — cached |
So caching moves pyre onto the target; the per-write lookup it replaced was the deviation. pypy3's own app-level _Pickler disagrees with its interp-level one, which is the tell.
The same probe caught an adjacent divergence worth recording: with a file lacking write and buffer_callback at protocol 4, 3.14 reports TypeError: file must have a 'write' attribute while pypy3 reports ValueError: buffer_callback needs protocol >= 5 (descr__new__, interp_pickle.py:1822, checks it first). pyre already ordered these the 3.14 way and this patch preserved it.
Disposition: won't-fix, documented. Both measurements are now recorded at their sites in 9b27a774ee6 so this does not get re-filed.
— commented by Claude
| unsafe { | ||
| pyre_object::listobject::w_list_clear(cur(slot).w_stack); | ||
| pyre_object::listobject::w_list_clear(cur(slot).w_metastack); |
There was a problem hiding this comment.
On second and subsequent load() calls, this clears and reuses the constructor-owned list objects, while upstream W_Unpickler.load installs fresh metastack and stack lists on every invocation (pypy/module/_pickle/interp_pickle.py:2042-2043). This is the allocation-reuse shortcut that the repository's strict structural-parity rule forbids; create and assign fresh lists in load() as upstream does.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and the reason is stronger than the one filed. Reverted in 033471d66d8.
My first instinct was to defend it the way I defended the Pickler half — "CPython's accelerator reuses too" — and that defence is measurably wrong as applied. CPython's stack is one Pdata for the unpickler's whole lifetime, invariant even across a load that ends with an unbalanced MARK, because MARK is tracked by num_marks/fence indices inside that one object (_pickle.c Pdata_New:454, :6888-6899). There is no metastack of containers at all. pyre keeps PyPy's metastack-of-lists (unpickler.rs mark() rebinds w_stack to a fresh list) and bolted CPython's reuse policy on top, producing a shape neither upstream has. Measured:
3.14 Pdata identity stable across open-MARK loads: True
pyre same-container-across-loads: True (two plain loads)
same-container-across-loads: False (open-MARK load between)
So the comment the commit introduced — "Reset the constructor-owned stacks in place" — was already false: after any load reaching STOP with an open MARK, w_stack points at a mark()-allocated list and the constructor's is dropped.
It is observable, not just structural. Capture the live stack during one load via gc.get_referents and append through it during a later one:
load3: (<function poison>, 'z') reused stacks
load3: (1, 'z') 3.14.5, pypy3 3.11.15, and after the revert
Everything else came back identical to 3.14 across ~2000 differential fuzz cases, double/triple loads, framed proto-4, cross-load memo, mid-load reducer failure, truncation, unbalanced-MARK carry-over, and __reduce__ / __setstate__ / persistent_load / find_class re-entry.
One thing worth filing separately, which this revert does not close: gc.get_referents(unpickler) hands pyre's live internal stack / metastack / memo to Python as ordinary mutable lists (module/gc/mod.rs:213-224), so the same corruption is reachable inside a single load. CPython hands out a Pdata, which has no append. That predates this PR.
cargo test --all --no-default-features --features dynasm exit 0 (7495 passed) and pyre/check.py exit 0 (391/391 · 391/391 · 387/387) after the revert.
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d614830a7b
ℹ️ 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".
| assert!( | ||
| TRACING_RESCALL_DUMMY_PTR.get().is_none(), | ||
| "JITFRAME_DUMMY type registered after the tracing sentinel was minted", | ||
| ); |
There was a problem hiding this comment.
When any GC-stress test reaches a traced residual call, token_tracing_rescall() permanently initializes TRACING_RESCALL_DUMMY_PTR; the next test calls reset_gc_fresh_for_test() (pyre/pyre-jit/tests/gc_stress.rs:50), which rebuilds the GC and invokes this setter again, so this assertion panics before the replacement heap is installed. Make repeat registration compatible with the test-only GC reset so the required cargo test --features dynasm suite can run more than one GC-stress case.
AGENTS.md reference: AGENTS.md:L236-L237
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e6c1e754891 — thank you, this was a real landmine.
The chain is as described: reset_gc_fresh_for_test → build_gc (pyre-jit/src/eval.rs:1338) → set_tracing_rescall_dummy_gc_type_id (:3639), once per GC-stress worker. It does not fire today only because no program in gc_stress.rs reaches a traced residual call, so the sentinel is never minted — all 32 pass. One test that does would have hit it.
Digging into it turned up the larger problem behind the assertion: the sentinel was a OnceLock, so even without any assertion a rebuilt heap would keep the address minted in the previous one. is_managed_heap_object stops recognising it once that heap is replaced, which puts the traced virtual_token / vable_token slots back on an address the collector does not own — exactly what this PR set out to remove. Blocking re-registration would have frozen that state rather than fixed it.
So the address now lives in an AtomicUsize that the setter clears, and the next request mints in the heap that is current; publication goes through a compare-exchange so racing minters agree on one address. The assertion is gone.
Added tracing_sentinel_is_reminted_for_a_rebuilt_heap to gc_stress.rs, which mints the sentinel between two resets and requires a fresh address. Against the previous commit it panics at virtualref.rs:214 with the message you predicted; with the fix the binary is 33/33.
— commented by Claude
`set_tracing_rescall_dummy_gc_type_id` is called from `build_gc` (`pyre-jit/src/eval.rs:1338,3639`), which `reset_gc_fresh_for_test` runs again per GC-stress worker. The sentinel was a `OnceLock`, so a second heap kept the address minted in the first — `is_managed_heap_object` no longer recognises it once the heap it belongs to has been replaced, putting the traced `virtual_token` / `vable_token` slots back on an address the collector does not own. The assertion added with the previous commit turned that into a panic on the second registration instead. Hold the address in an `AtomicUsize` the setter clears, so the next request mints in the heap that is now current, and publish it with a compare-exchange so racing minters agree on one address. The GC-stress harness produces this ordering the moment one of its programs reaches a traced residual call; none does today, so the new test mints the sentinel between two resets directly. It panics at the old assertion without this change. Reported by the Codex review bot on #1084. Assisted-by: Claude
`interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every
`load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime
with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`,
`_pickle.c:454`). Reuse is coherent only inside the second structure; layered
onto the metastack-of-lists port it produced a shape neither has, and
`mark()` rebinding `w_stack` to a fresh list meant the constructor's list was
dropped by any load that reached STOP with an open MARK.
It is observable. Capturing the live stack during one `load` through
`gc.get_referents` and appending through it during a later one changes that
load's result:
load3: (<function poison>, 'z') reused stacks
load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit
Reported by the Codex review bot on #1084.
Assisted-by: Claude
youknowone
changed the title
`Unpickler.load` allocated a fresh stack and metastack list on every call; both are now owned by the constructor, initialized lazily for the `__new__`-only path, and cleared in place instead. `Pickler` resolves `file.write` once in the constructor and stores it in a rooted `w_write` field; `Framer::flush` calls it directly rather than re-resolving the method on every flush. The constructor's error order is unchanged: the `write` check still precedes the protocol-5 `buffer_callback` check. Assisted-by: Claude
`virtualref.py:19` declares `virtual_token` and `rvirtualizable.py:29` declares `vable_token` as `llmemory.GCREF`, and `jitframe.py:59-61` makes JITFRAME a `GcStruct` allocated by `lltype.malloc` at `:50`. cranelift allocates its JITFRAME from the nursery accordingly, but both token slots and the FORCE_TOKEN result were excluded from GC tracing on the written premise that a JITFRAME address is `libc::calloc`'d and therefore outside the GC heap — which holds only for the dynasm backend. Register `virtual_token` in the vref `gc_ptr_offsets`, visit `vable_token` in `pyframe_object_custom_trace`, and stop excluding the FORCE_TOKEN result from the cranelift relocatable ref-root slots. `TOKEN_TRACING_RESCALL` was the `u64::MAX` sentinel, which is not a legal value for a traced slot; it becomes the address of a registered GC leaf, matching `virtualizable.py:326-330` where the sentinel is the prebuilt `_dummy` object. Host-side active-token stores now take the same write barrier a compiled SETFIELD_GC store would. Assisted-by: Claude
The two sibling cross-loop close sites bump `bridge_declined_close` (50) when an attempt is declined; the JUMP block's own Declined arm bumped nothing. Bump it on the same `attempted` condition the decline latch uses, so the tally counts closes an optimizer pass rejected rather than headers the gate skipped. Reported by CodeRabbit on #1040. Assisted-by: Claude
`add_struct_field_descr` deep-copied the whole parent `BhSizeSpec` (one owned `String` per field) on every field-descr mint, although `patch_field_descr_parents` — called unconditionally from `try_finish` after the decline early-return — replaces that snapshot with the final merged spec, and `struct_size_specs` entries are only inserted or merged, never removed. Carry the scalar fields and leave `all_fielddescrs` empty; `type_id` is the only part the patch pass reads. `register_struct_layout` rebuilt `field_specs_from_layout` on each of its ~211 calls for ~5 distinct layouts. When the cached spec already lists an offset for every incoming field the merge pushes nothing and the following re-sort/re-index are no-ops, and the branch never writes `size`, `is_gc_managed` or `headerless`, so return before building the discarded vector. Measured on aheui's `mainloop` fixed per-process init (never-tracing, 200 iterations, min of 5 interleaved rounds): 243.7 -> 214.6 us/call. Assisted-by: Claude
…est) `allocate_tracing_rescall_dummy` guarded its unmanaged fallback with `#[cfg(test)]`, which is set only while majit-metainterp compiles its own test harness. Built as an ordinary dependency the arm disappears, so `pyre-jit-trace`'s `may_force_vable_escape_surfaces_typed_abort` and `may_force_with_active_vable_executes_and_clears_token` — which drive the token protocol without a collector — reached the `assert_ne!` and panicked. Branch on the unset type id itself: the leaf type is registered by the same setup that installs a collector, so an unset id means there is no managed heap to mint the object in, and the host address stays outside it where `is_managed_heap_object` rejects it before any tracing path reads its header. `alloc_virtual_ref` spells the same window the same way. The ordering requirement moves to `set_tracing_rescall_dummy_gc_type_id`, which asserts the sentinel has not already been minted. Assisted-by: Claude
…ivergences `interp_pickle.py` resolves `file.write` at `:555-560` only to validate and re-resolves it per write in `_Framer.file_write` (`:353`), and checks `buffer_callback` before the file in `descr__new__` (`:1822`). Measured on 3.14.5 neither holds: rebinding `file.write` after construction is not observed by a later `dump()` — `pickle.py:465` captures the callable the same way — and a call carrying both constructor faults reports the `write` TypeError. pyre matches the measurements; note them at both sites. Assisted-by: Claude
`set_tracing_rescall_dummy_gc_type_id` is called from `build_gc` (`pyre-jit/src/eval.rs:1338,3639`), which `reset_gc_fresh_for_test` runs again per GC-stress worker. The sentinel was a `OnceLock`, so a second heap kept the address minted in the first — `is_managed_heap_object` no longer recognises it once the heap it belongs to has been replaced, putting the traced `virtual_token` / `vable_token` slots back on an address the collector does not own. The assertion added with the previous commit turned that into a panic on the second registration instead. Hold the address in an `AtomicUsize` the setter clears, so the next request mints in the heap that is now current, and publish it with a compare-exchange so racing minters agree on one address. The GC-stress harness produces this ordering the moment one of its programs reaches a traced residual call; none does today, so the new test mints the sentinel between two resets directly. It panics at the old assertion without this change. Reported by the Codex review bot on #1084. Assisted-by: Claude
`interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every
`load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime
with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`,
`_pickle.c:454`). Reuse is coherent only inside the second structure; layered
onto the metastack-of-lists port it produced a shape neither has, and
`mark()` rebinding `w_stack` to a fresh list meant the constructor's list was
dropped by any load that reached STOP with an open MARK.
It is observable. Capturing the live stack during one `load` through
`gc.get_referents` and appending through it during a later one changes that
load's result:
load3: (<function poison>, 'z') reused stacks
load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit
Reported by the Codex review bot on #1084.
Assisted-by: Claude
rustfmt collapses the `compare_exchange` call in `token_tracing_rescall` onto one line. No behaviour change. Assisted-by: Claude
`WarmState::log_compile` takes `opt_time` and `compile_time` and forwards them to the jitlog, which prints them as the `Optimization time:` / `Compilation time:` lines of the `MAJIT_STATS` summary (`majit-trace/src/logger.rs:194-207`, summed over every compile). Both production call sites passed `Duration::ZERO`, so both lines read `0.0ms` for every program. `compile_loop_body` now times the optimize block — the primary `optimize_trace_with_constants_and_inputs_vable_out`, the without-unroll retry taken on `InvalidLoop`, and loop vectorization — and separately the `self.backend.compile_loop` call already wrapped by `profiler.enter_backend()`. `compile_retrace` gets the same treatment for its own optimizer and backend calls. `jitprof.rs` privately imported `std::time::Instant`, or its `wasm_clock::Instant` shim on wasm32 where `Instant::now()` panics; both become `pub use` so `pyjitpl.rs` reaches the platform-agnostic type instead of naming `std::time::Instant` directly. Measured on aheui: `standard/loop` (85 recorded ops) reports 3.1ms / 1.3ms, `logo` (33177) reports 149.0ms / 25.3ms. Assisted-by: Claude
`self.stats.loops_compiled += 1` appears at five places in pyjitpl.rs, but only `compile_loop_body` (:6808) and `compile_retrace` (:7978) called `warm_state.log_compile`. `finish_and_compile`, `compile_simple_loop` and `compile_entry_bridge` compiled a loop and told the jitlog nothing. `log_compile` is the sole source of the `=== JIT Statistics ===` block that `MAJIT_STATS=1` prints (`majit-trace/src/logger.rs:194-207`), so a run whose loop arrived through one of those three paths reported `Traces compiled: 0`, `Total ops recorded: 0` and both times `0.0ms` while the run had in fact compiled a loop. Each of the three now times its own optimizer invocation and its own `backend.compile_loop` call and passes the counts and durations, matching the two working sites. On aheui's logo at `MAJIT_TRACE_LIMIT=30000` — a limit low enough that the whole-program trace aborts and the loop arrives through one of these paths — the block goes from `0 / 0 / 0.0ms / 0.0ms` to `Traces compiled: 1`, 24004 recorded ops, 7065 after optimization, 42.3ms and 9.7ms. At the default limit the same program is unchanged at 1 / 33177 / 20564, so no compile is now counted twice. Assisted-by: Claude
![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 `@majit/majit-backend-cranelift/src/compiler.rs`:
- Around line 4701-4707: Add a regression test near the existing loop-invariant
deoptimization and call-may-force tests that captures a ForceToken, performs an
intervening nursery collection via CallMallocNursery, then verifies a later
token use—such as a fail argument, CallMayForceI, or force() result—contains the
moved JITFRAME address rather than the stale address. Model the setup and
assertions on
loop_invariant_deopt_ref_with_preamble_use_survives_nursery_collection.
In `@majit/majit-metainterp/src/virtualref.rs`:
- Around line 198-219: Update the tracing rescall sentinel synchronization in
allocate_tracing_rescall_dummy and token_tracing_rescall: use Acquire for
fast-path pointer loads and failed compare_exchange outcomes, AcqRel for
successful compare_exchange operations, and Release when resetting
TRACING_RESCALL_DUMMY_PTR in set_tracing_rescall_dummy_gc_type_id. Leave
TRACING_RESCALL_DUMMY_GC_TYPE_ID ordering Relaxed.
🪄 Autofix
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: ebb9ec11-2210-48cf-bd7f-fd9102ee4283
📒 Files selected for processing (10)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/jitprof.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/virtualizable.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/tests/gc_stress.rs
| fn build_force_token_set(_inputargs: &[InputArg], _ops: &[Op]) -> IndexSet<u32> { | ||
| // FORCE_TOKEN is a GCREF to the active JITFRAME | ||
| // (`virtualizable.py:315-318`, `resoperation.py:1090`). Keep its in-frame | ||
| // copies in the ordinary Ref root set so moving collectors update them. | ||
| // The empty compatibility set leaves the existing exit-layout plumbing in | ||
| // place while giving FORCE_TOKEN the same treatment as every other Ref. | ||
| IndexSet::new() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for FORCE_TOKEN surviving a GC move.
This diff's purpose is to keep FORCE_TOKEN's in-frame copies in the ordinary Ref root set so a moving collector updates them. This file has no test exercising that exact property: existing loop_invariant_deopt_ref_* tests cover ordinary Ref inputargs/op-results across a nursery collection, and test_call_may_force_* tests exercise forcing/guard-not-forced without triggering GC between ForceToken and its use. Add a test that captures a ForceToken, triggers a nursery collection through an intervening collecting call (e.g., CallMallocNursery), and asserts the token used later (in a fail-arg, a CallMayForceI, or the frame returned by force()) reflects the moved JITFRAME address rather than a stale one.
Do you want me to generate this test, modeled on loop_invariant_deopt_ref_with_preamble_use_survives_nursery_collection?
🤖 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-backend-cranelift/src/compiler.rs` around lines 4701 - 4707, Add
a regression test near the existing loop-invariant deoptimization and
call-may-force tests that captures a ForceToken, performs an intervening nursery
collection via CallMallocNursery, then verifies a later token use—such as a fail
argument, CallMayForceI, or force() result—contains the moved JITFRAME address
rather than the stale address. Model the setup and assertions on
loop_invariant_deopt_ref_with_preamble_use_survives_nursery_collection.
| /// Lazy initialisation of the `_dummy` address, as a `usize` because raw | ||
| /// pointers are not `Sync`. Zero means "not minted yet"; a null sentinel would | ||
| /// collide with `TOKEN_NONE`, so it is not a value this can ever hold. | ||
| static TRACING_RESCALL_DUMMY_PTR: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| const TRACING_RESCALL_DUMMY_GC_TYPE_ID_UNSET: u32 = u32::MAX; | ||
| static TRACING_RESCALL_DUMMY_GC_TYPE_ID: AtomicU32 = | ||
| AtomicU32::new(TRACING_RESCALL_DUMMY_GC_TYPE_ID_UNSET); | ||
|
|
||
| /// Publish the registered leaf type used by the prebuilt | ||
| /// `virtualizable.py:326-330 JITFRAME_DUMMY` object. | ||
| /// | ||
| /// Registration comes from `build_gc`, so a second call means a second heap. | ||
| /// A sentinel minted in the previous one is no longer part of the live heap — | ||
| /// `is_managed_heap_object` would stop recognising it and the traced | ||
| /// `virtual_token` / `vable_token` slots would be back to holding an address | ||
| /// the collector does not own. Drop it so the next request mints in the heap | ||
| /// that is now current. | ||
| pub fn set_tracing_rescall_dummy_gc_type_id(type_id: u32) { | ||
| TRACING_RESCALL_DUMMY_GC_TYPE_ID.store(type_id, Ordering::Relaxed); | ||
| TRACING_RESCALL_DUMMY_PTR.store(0, Ordering::Relaxed); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Survey other lazy-publish atomic patterns for the established ordering convention.
rg -n -B2 -A2 'compare_exchange' majit/majit-metainterp/src/ majit/majit-gc/src/ --type=rust
rg -n -B2 -A2 'OnceLock' majit/majit-metainterp/src/virtualref.rsRepository: youknowone/pyre
Length of output: 1318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## virtualref.rs relevant lines"
sed -n '150,285p' majit/majit-metainterp/src/virtualref.rs
echo
echo "## rgil.rs compare_exchange and surrounding ordering context"
sed -n '170,225p' majit/majit-gc/src/rgil.rs
echo
echo "## Rust AtomicUsize::compare_exchange documentation in std source if vendored? (no-op)"
rg -n "compare_exchange_ordering|compare_exchange" /usr/local/rustup/toolchains/$(rustc +stable --version 2>/dev/null | awk '{print $2}')/lib/rustlib/src/rust/library/std/src/sync/atomic.rs 2>/dev/null | head -80 || trueRepository: youknowone/pyre
Length of output: 9095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import urllib.request
url = "https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#method.compare_exchange"
text = urllib.request.urlopen(url, timeout=10).read().decode("utf-8", errors="replace")
for needle in [
"memory ordering",
"Acquire semantics",
"Release semantics",
"compare_exchange",
]:
idx = text.find(needle)
if idx != -1:
start = max(idx - 300, 0)
end = min(idx + 600, len(text))
print(f"## found: {needle}")
print(text[start:end])
print()
PYRepository: youknowone/pyre
Length of output: 2439
🌐 Web query:
Rust std sync atomic compare_exchange memory ordering Acquire Release semantics documentation
💡 Result:
In Rust, the compare_exchange method for atomic types takes two memory ordering arguments: success and failure [1][2]. These arguments define how the operation interacts with the memory model's happens-before relationship [3][4]. The success ordering describes the requirement for the full read-modify-write operation if the comparison succeeds (the value matches the current value and the new value is stored) [1][2]. The failure ordering describes the requirement for the load operation performed if the comparison fails (the value does not match) [1][2][5]. Acquire and Release semantics are commonly used together to establish synchronization [6][7]: 1. Release semantics, when applied to a store, ensure that all memory operations occurring before the store remain ordered before it [6][7]. 2. Acquire semantics, when applied to a load, ensure that all memory operations occurring after the load remain ordered after it [6][7]. When used in compare_exchange: - Using Acquire as the success ordering makes the store part of the operation Relaxed [1][2]. - Using Release as the success ordering makes the successful load part of the operation Relaxed [1][2]. - Therefore, AcqRel is typically used for the success ordering if both Acquire and Release semantics are required for the read-modify-write operation [6][5]. - The failure ordering must be one of Relaxed, Acquire, or SeqCst [1][2]. Since a failing compare_exchange is only a load operation, it cannot provide Release semantics [1][2][5]. Rust's memory orderings follow C++20 atomic rules [6][3]. For a detailed understanding of how these orderings affect code execution, consult the official Rust documentation for std::sync::atomic or The Rustonomicon [6][3][7].
Citations:
- 1: https://doc.rust-lang.org/stable/std/sync/atomic/struct.Atomic.html
- 2: https://doc.rust-lang.org/core/sync/atomic/struct.Atomic.html
- 3: https://doc.rust-lang.org/std/sync/atomic/
- 4: https://dev-doc.rust-lang.org/stable/src/core/sync/atomic.rs.html
- 5: https://mara.nl/atomics/memory-ordering.html
- 6: https://doc.rust-lang.org/stable/std/sync/atomic/enum.Ordering.html
- 7: https://doc.rust-lang.org/nightly/nomicon/atomics.html
🌐 Web query:
Rust compare_and_swap AcqRel release acquire ordering failure
💡 Result:
In Rust, compare_exchange and compare_exchange_weak require two memory ordering arguments: one for success and one for failure [1][2]. The failure ordering specifies the memory consistency guarantees applied when the operation fails to update the atomic value (i.e., when it effectively performs a load) [1][2][3]. Key constraints and behaviors for the failure ordering include: 1. Permitted Values: The failure ordering must be no stronger than the success ordering [4][5]. Additionally, only Relaxed, Acquire, and SeqCst are permitted as failure orderings [1][2][5]. Release and AcqRel are explicitly disallowed because they would imply store semantics, which do not occur when the compare-and-swap operation fails [4]. 2. Relationship to Success Ordering: The failure ordering cannot be stronger than the success ordering [4][5]. For example, if you choose AcqRel for success, you may use Acquire or Relaxed for failure [5]. 3. Why AcqRel/Release are disallowed on failure: Because a failed compare-and-swap operation performs only a load (it does not write to the memory location), it cannot satisfy Release semantics, which require a store [4]. Using AcqRel or Release as a failure ordering is logically inconsistent with the nature of a failed atomic read-modify-write operation [4][6]. 4. Practical Application: When using AcqRel for success, the failure ordering should generally be chosen based on what synchronization is required when the CAS fails. If the failure path requires synchronizing with previous writes (e.g., in a lock-free data structure), Acquire is the appropriate choice to ensure visibility of preceding memory operations [7][8]. If no such synchronization is needed, Relaxed is sufficient and often more performant [9][5]. In summary, Rust enforces these rules to align with the C++ memory model, ensuring that the specified orderings match the actual memory access patterns (load vs. read-modify-write) triggered by the outcome of the atomic operation [6][10].
Citations:
- 1: https://doc.rust-lang.org/core/sync/atomic/type.AtomicUsize.html
- 2: https://doc.rust-lang.org/stable/core/sync/atomic/type.AtomicUsize.html
- 3: https://users.rust-lang.org/t/which-memory-ordering-is-used-in-compare-exchange/118963
- 4: https://users.rust-lang.org/t/why-does-rust-not-permit-cas-operations-with-release-acquire-ordering/40394
- 5: https://rust-lang.github.io/rfcs/1443-extended-compare-and-swap.html
- 6: https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html
- 7: https://users.rust-lang.org/t/correct-memory-ordering-for-compare-exchange-fail-path/118953/1
- 8: https://users.rust-lang.org/t/correct-memory-ordering-for-compare-exchange-fail-path/118953
- 9: https://users.rust-lang.org/t/what-does-the-compare-exchange-fail-ordering-mean/75791
- 10: https://doc.rust-lang.org/core/sync/atomic/
🌐 Web query:
std::sync::atomic AtomicUsize compare_exchange orderings Acquire Release success failure Rust
💡 Result:
The compare_exchange method on Rust atomic types, such as AtomicUsize, allows for precise control over memory ordering by requiring two distinct Ordering parameters: success and failure [1][2]. Success Ordering The success parameter defines the memory ordering applied to the read-modify-write operation if the comparison succeeds (i.e., the atomic's value matches the current value and the new value is stored) [1][2]. If the operation succeeds, it behaves as a full read-modify-write operation using the specified ordering [3]. For example, if you provide Acquire as the success ordering, the store part of the operation is treated as Relaxed; if you provide Release, the load part is treated as Relaxed [1][2]. Failure Ordering The failure parameter defines the memory ordering applied to the load operation if the comparison fails (i.e., the atomic's value does not match the current value, and no store occurs) [1][2][3]. Because the operation acts strictly as a load in this case, the failure ordering must be weaker than or equal to the success ordering [4]. Consequently, the failure ordering can only be Ordering::SeqCst, Ordering::Acquire, or Ordering::Relaxed [1][2]. Usage Note The compiler ensures the generated instructions satisfy both constraints, even if it must choose a stronger ordering than requested for one of the cases [5]. Providing distinct orderings is often critical for performance and correctness; for instance, when implementing a spinlock, you might only need Acquire ordering on failure to synchronize with a previous store, while requiring stronger guarantees on success [3]. Finally, note that compare_exchange is the idiomatic replacement for the now-deprecated compare_and_swap [6][2]. For loops, consider using compare_exchange_weak, which allows for spurious failures but can produce more efficient machine code [7][4].
Citations:
- 1: https://doc.rust-lang.org/core/sync/atomic/type.AtomicUsize.html
- 2: https://dev-doc.rust-lang.org/stable/src/core/sync/atomic.rs.html
- 3: https://mara.nl/atomics/memory-ordering.html
- 4: https://rust-lang.github.io/rfcs/1443-extended-compare-and-swap.html
- 5: https://users.rust-lang.org/t/what-does-the-compare-exchange-fail-ordering-mean/75791
- 6: https://doc.rust-lang.org/1.93.1/std/sync/atomic/struct.AtomicUsize.html
- 7: https://doc.rust-lang.org/nightly/core/sync/atomic/type.AtomicUsize.html
🌐 Web query:
documentation AtomicUsize compare_exchange_acquire_release_acquire ordering failure
💡 Result:
In Rust's AtomicUsize (and other atomic types), the compare_exchange and compare_exchange_weak methods accept two ordering parameters: success and failure [1][2]. These specify the memory ordering constraints for the operation when it succeeds (performing a read-modify-write) and when it fails (performing only a load) [1][3]. The failure ordering is restricted to Ordering::SeqCst, Ordering::Acquire, or Ordering::Relaxed [1][3]. Additionally, the failure ordering must be equivalent to or weaker than the success ordering [4][5]. Using success = Ordering::AcqRel and failure = Ordering::Acquire is a valid and commonly used configuration [6]. In this case: - Success: The operation performs an Acquire-Release read-modify-write [1][7]. - Failure: The operation performs an Acquire load [1][7]. Documentation and compiler checks enforce that the failure ordering cannot be Release or AcqRel, as these are not applicable to the load operation that occurs on failure [7][8]. The "failure" ordering argument specifically describes the ordering requirement for the load that occurs when the comparison fails [1][3]. For context, developers sometimes encounter issues if they attempt to pass an invalid combination, such as a failure ordering that is stronger than the success ordering or an inapplicable ordering (like Release) for the failure case [8]. If you are looking to combine AcqRel for success and Acquire for failure, this is supported and standard for synchronizing state changes while ensuring memory visibility [6].
Citations:
- 1: https://doc.rust-lang.org/core/sync/atomic/type.AtomicUsize.html
- 2: https://doc.rust-lang.org/1.87.0/std/sync/atomic/struct.AtomicUsize.html
- 3: https://dev-doc.rust-lang.org/stable/src/core/sync/atomic.rs.html
- 4: https://doc.rust-lang.org/1.47.0/core/sync/atomic/struct.AtomicUsize.html
- 5: https://rust-lang.github.io/rfcs/1443-extended-compare-and-swap.html
- 6: https://doc.rust-lang.org/1.77.1/std/intrinsics/fn.atomic_cxchg_acqrel_acquire.html
- 7: https://doc.rust-lang.org/stable/std/sync/atomic/enum.Ordering.html
- 8: https://users.rust-lang.org/t/why-does-rust-not-permit-cas-operations-with-release-acquire-ordering/40394
🌐 Web query:
"draft" "compare_exchange" "AtomicUsize" "Ordering::AcqRel" "failure ordering"
💡 Result:
When using AtomicUsize::compare_exchange with Ordering::AcqRel as the success ordering, the failure ordering must be a strictly weaker or equivalent ordering that is valid for a load operation [1][2][3]. Specifically, when compare_exchange fails, it performs a load operation, and the failure ordering specifies the memory ordering requirements for that load [1][4]. For compare_exchange, the failure ordering can only be Ordering::SeqCst, Ordering::Acquire, or Ordering::Relaxed [1][5][4]. Using Ordering::AcqRel as a success ordering implies that if the operation succeeds, it performs a read-modify-write operation with Acquire and Release semantics [6][7]. If it fails, the Release part of the AcqRel requirement is irrelevant because no store occurs; therefore, the operation defaults to Acquire semantics [1][4][3]. In practice, if you specify AcqRel for success, the standard failure ordering is Ordering::Acquire [1][3]. This ensures that even if the comparison fails, the operation still provides Acquire semantics, which is necessary to synchronize with preceding releases in other threads [1][8]. Attempting to pass an invalid ordering for failure (such as Release or AcqRel) will cause the operation to panic [2][3]. Summary of standard mappings for compare_exchange failure: - If success is AcqRel, the failure ordering should be Acquire [1][3]. - The failure ordering must be equal to or weaker than the success ordering [2][3]. - Only SeqCst, Acquire, and Relaxed are valid as failure orderings [1][5][4].
Citations:
- 1: https://doc.rust-lang.org/core/sync/atomic/type.AtomicUsize.html
- 2: https://doc.rust-lang.org/1.25.0/std/sync/atomic/struct.AtomicUsize.html
- 3: https://rust-lang.github.io/rfcs/1443-extended-compare-and-swap.html
- 4: https://doc.rust-lang.org/src/core/sync/atomic.rs.html
- 5: https://doc.rust-lang.org/stable/core/sync/atomic/type.AtomicUsize.html
- 6: https://doc.rust-lang.org/stable/std/sync/atomic/enum.Ordering.html
- 7: https://dev-doc.rust-lang.org/stable/std/sync/atomic/enum.Ordering.html
- 8: https://users.rust-lang.org/t/what-does-the-compare-exchange-fail-ordering-mean/75791
🌐 Web query:
Rust atomic compare_exchange success Acquire store is relaxed
💡 Result:
In Rust's atomic compare_exchange operation, using Ordering::Acquire as the success memory ordering implies that the store part of the read-modify-write operation is performed with Ordering::Relaxed semantics [1][2][3]. When you specify an Ordering for the success case of compare_exchange, it defines the behavior of the entire read-modify-write operation [1][3]. According to the official Rust documentation [1][2][3]: * When Ordering::Acquire is used for the success case, the operation performs an acquire load (to observe memory) and a relaxed store (to update the value). * Conversely, using Ordering::Release for the success case makes the load part of the operation relaxed, while maintaining release semantics for the store [1][2][3]. This distinction is important because it dictates how the operation synchronizes memory with other threads. An Acquire success ordering ensures that the load component synchronizes with previous release operations in other threads, while the store component only provides the atomic update without adding additional release synchronization requirements [4][5]. For context, compare_exchange requires two ordering arguments: 1. success: The ordering used if the comparison succeeds (the read-modify-write operation) [1][3]. 2. failure: The ordering used if the comparison fails (a simple load operation) [1][3]. The failure ordering is restricted to Ordering::SeqCst, Ordering::Acquire, or Ordering::Relaxed [1][2].
Citations:
- 1: https://doc.rust-lang.org/core/sync/atomic/struct.Atomic.html
- 2: https://doc.rust-lang.org/stable/std/sync/atomic/struct.Atomic.html
- 3: https://dev-doc.rust-lang.org/stable/src/core/sync/atomic.rs.html
- 4: https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html
- 5: https://doc.rust-lang.org/nomicon/atomics.html
Use acquire/release ordering for the traced token_tracing_rescall sentinel.
allocate_tracing_rescall_dummy writes the dummy ObjectHeader with std::ptr::write, then token_tracing_rescall publishes the address with Relaxed loads and a Relaxed compare_exchange. Relaxed ordering does not establish the needed happens-before relationship, so another thread can observe the published address without seeing the object initialization.
Use Acquire for the fast-path load and for the failing CAS outcome. Since success performs an atomic update, use AcqRel for the successful compare_exchange success ordering and Acquire for its failure ordering, and use Release for the unsetting store in set_tracing_rescall_dummy_gc_type_id. TRACING_RESCALL_DUMMY_GC_TYPE_ID can remain Relaxed because the Release store to TRACING_RESCALL_DUMMY_PTR orders the preceding write in that function.
This also applies to the same sentinel pattern around allocate_tracing_rescall_dummy and the type-id unset path.
🤖 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/virtualref.rs` around lines 198 - 219, Update the
tracing rescall sentinel synchronization in allocate_tracing_rescall_dummy and
token_tracing_rescall: use Acquire for fast-path pointer loads and failed
compare_exchange outcomes, AcqRel for successful compare_exchange operations,
and Release when resetting TRACING_RESCALL_DUMMY_PTR in
set_tracing_rescall_dummy_gc_type_id. Leave TRACING_RESCALL_DUMMY_GC_TYPE_ID
ordering Relaxed.
…#1120) * _pickle: reuse the unpickler stacks and cache the pickler write callable `Unpickler.load` allocated a fresh stack and metastack list on every call; both are now owned by the constructor, initialized lazily for the `__new__`-only path, and cleared in place instead. `Pickler` resolves `file.write` once in the constructor and stores it in a rooted `w_write` field; `Framer::flush` calls it directly rather than re-resolving the method on every flush. The constructor's error order is unchanged: the `write` check still precedes the protocol-5 `buffer_callback` check. Assisted-by: Claude * _pickle: allocate the unpickler stack and metastack per load again `interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every `load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`, `_pickle.c:454`). Reuse is coherent only inside the second structure; layered onto the metastack-of-lists port it produced a shape neither has, and `mark()` rebinding `w_stack` to a fresh list meant the constructor's list was dropped by any load that reached STOP with an open MARK. It is observable. Capturing the live stack during one `load` through `gc.get_referents` and appending through it during a later one changes that load's result: load3: (<function poison>, 'z') reused stacks load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit Reported by the Codex review bot on #1084. Assisted-by: Claude * jit: map live values onto the front target LABEL's own argument order A dispatch-key entry lands on a LABEL whose arguments are its own, not the frontend's state-field layout. Three sites re-mapped the values only when the two disagreed in length: if live_values.len() != expected { pack_front_target_live_values(...) } Equal arity was taken as equal order. `aheui`'s `pi/pi.jinseo` under the cranelift backend produced a four-argument `(Int, Ref, Ref, Ref)` LABEL and a four-value `(selected: Int, stacksize: Int, selected_ref: Ref, storage_ref: Ref)` state, so the mapping was skipped and `Int(2)` entered slot 1. Compiled code loads a Ref slot as a pointer without checking it: `EXC_BAD_ACCESS address=0x1c`. Only cranelift reaches this — every other backend reports `supports_dispatch_key_entry() == false`, so its dispatch key is always 0 and the peeled preamble runs. Run the mapping unconditionally at all three sites and decline the entry when it is unavailable; the fallback is the key-0 entry the other backends take. `try_resume_into_compiled_loop`'s full-live fallback is mapped where it is produced, so `direct_live_values` is now LABEL-ordered at every caller. `pack_front_target_live_values` additionally confirms each packed value's type against the LABEL's declared inputarg type, and `back_edge_internal` confirms the same for the compact values it does not re-map. For `pi.jinseo` the recorded mapping is `sources=[1, 2, 3]` against a four-argument LABEL: the fourth argument is a trace-internal value with no state field behind it, so the entry is declined. Official aheui suite, whole corpus, at MAJIT_THRESHOLD 100/1000/5000/20000/ 100000: cranelift 62/62 at every step (previously 61/62 with `pi.jinseo` segfaulting at 1000, 5000, 20000 and 100000), dynasm 62/62 unchanged, no crash or abort lines. jitstats 0 failed, baselines not re-recorded. Assisted-by: Claude
`interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every
`load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime
with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`,
`_pickle.c:454`). Reuse is coherent only inside the second structure; layered
onto the metastack-of-lists port it produced a shape neither has, and
`mark()` rebinding `w_stack` to a fresh list meant the constructor's list was
dropped by any load that reached STOP with an open MARK.
It is observable. Capturing the live stack during one `load` through
`gc.get_referents` and appending through it during a later one changes that
load's result:
load3: (<function poison>, 'z') reused stacks
load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit
Reported by the Codex review bot on #1084.
Assisted-by: Claude
…descr (#1142) * _pickle: reuse the unpickler stacks and cache the pickler write callable `Unpickler.load` allocated a fresh stack and metastack list on every call; both are now owned by the constructor, initialized lazily for the `__new__`-only path, and cleared in place instead. `Pickler` resolves `file.write` once in the constructor and stores it in a rooted `w_write` field; `Framer::flush` calls it directly rather than re-resolving the method on every flush. The constructor's error order is unchanged: the `write` check still precedes the protocol-5 `buffer_callback` check. Assisted-by: Claude * _pickle: allocate the unpickler stack and metastack per load again `interp_pickle.py:2042-2043` installs a fresh `stack` and `metastack` on every `load`, and `_pickle.c` reuses one `Pdata` for the unpickler's whole lifetime with MARK tracked by `num_marks`/`fence` indices inside it (`Pdata_New`, `_pickle.c:454`). Reuse is coherent only inside the second structure; layered onto the metastack-of-lists port it produced a shape neither has, and `mark()` rebinding `w_stack` to a fresh list meant the constructor's list was dropped by any load that reached STOP with an open MARK. It is observable. Capturing the live stack during one `load` through `gc.get_referents` and appending through it during a later one changes that load's result: load3: (<function poison>, 'z') reused stacks load3: (1, 'z') 3.14.5, pypy3 3.11.15, and this commit Reported by the Codex review bot on #1084. Assisted-by: Claude * majit: carry a struct field's declared width and signedness into its descr `jit_inline`/`jit_interp` gain an `int_fields = { Struct::field => u32 }` clause. A field named there registers `size_of::<ty>()` and the type's signedness with the struct layout; an undeclared field keeps the signed machine-word default. A generated `const _: fn(&Struct) -> ty` makes the declaration a compile error unless the Rust field really has that type. `field_specs_from_layout`, `add_struct_field_descr` and the `struct_fields_write_effect_info` twin now read the width and flag from the registered layout instead of re-deriving one signed machine word from the IR type, so whichever of them mints the field descr first records the same width. The registered layout tuple grows from `(offset, is_ref, name)` to `(offset, is_ref, name, field_size, is_signed)`. Assisted-by: Claude * majit: spell the int-field width default as i64, and update the layout tuple's test callers The macro's width for an undeclared integer field was `size_of::<usize>()`, which is 4 on wasm32. `scalar_size` reports `size_of::<i64>()` for every non-`Ref` field, so the default spells that instead; a `Ref` field's width comes from `Type::Ref` and ignores the declared one either way. `generate_inline_helper_jitcode_with_calls` took a new `int_fields` parameter and the registered layout tuple grew two elements, but six `#[cfg(test)]` call sites in majit-macros and majit-metainterp still used the old arity. `cargo check` does not compile those, so only `cargo test` sees them. Also reformat the `newlist_clear` layout literal, which `cargo fmt --check` rejected. Assisted-by: Claude
1 participant
Four independent changes: one GC-tracing correctness fix in the JIT, two allocation/lookup reuses, and one diag counter.
jit: trace the force token as a GC slotvirtualref.py:19declaresvirtual_tokenandrvirtualizable.py:29declaresvable_tokenasllmemory.GCREF, andjitframe.py:59-61makes JITFRAME aGcStructallocated bylltype.mallocat:50. cranelift allocates itsJITFRAME from the nursery accordingly
(
compiler.rs:7353run_compiled_code_inner, realloc:3800, bothgc.alloc_nursery_no_collect_typed), but three copies of that address wereexcluded from GC tracing, each citing the same premise — "an active JITFRAME
address —
libc::calloc'd on a host-side pool, not nursery/oldgen". Thatpremise holds only for dynasm (
alloc_off_gc_jitframe,runner.rs:2815). Oncranelift a minor collection moves the frame, updates the shadow-stack root,
and leaves all three copies dangling;
force_token_to_dead_frame(
compiler.rs:2888) validates only!= 0, so a recycled word passes andjitframe_resolve'sjf_forwardwalk faults.The fix is the tracing, not the allocation —
jitframe.pysays JITFRAME ismovable and GC-allocated, so cranelift's nursery allocation is the
parity-correct half.
rgc.pinappears underrpython/jit/only inzrpy_gc_test.py, so pinning the frame has no upstream basis either.virtual_tokenjoins the vrefgc_ptr_offsetspyframe_object_custom_tracevisitsvable_tokenref-root slots
TOKEN_TRACING_RESCALLwas theu64::MAXsentinel, which is not a legalvalue for a traced slot; it becomes the address of a registered GC leaf,
matching
virtualizable.py:326-330where the sentinel is the prebuilt_dummyobject. This matters because the two tracing paths differ: minor(
collector.rs:2457/:2490) gates every slot onis_nursery_object_startso junk is inert, but major
grey_child(:3266) gates onis_managed_heap_objectand then panics on a bad type id.SETFIELD_GCstore wouldEvidence
The defect surfaces under three different exit codes — 139 (SIGSEGV), 134
(
force_token_to_dead_frame: jf_force_descr is null,compiler.rs:2896) and101 (GC BUG panic) — so the crash predicate is
exit NOT IN {0,1}.PYPY_GC_NURSERY=2G(minors effectively off) is the decisive control: it makesthe reproducer clean, which is what identifies the family.
Same-base A/B, the commit checked in and out at file level, full
lib-python/3/test/test_pickle.pyon cranelift, 3 runs each:Reduced reproducer (
test_pickle.py InMemoryPickleTests PyPicklerTests PyPicklingErrorTests): 8/13 crashes → 0/6. GC stress atPYPY_GC_NURSERY=64K/512K: clean 6/6. Fulltest_picklenow runsRan 999 testswith no panic._pickle: reuse the unpickler stacks and cache the pickler write callablePicklerresolvesfile.writeonce in the constructor into a rootedw_writefield, and
Framer::flushcalls it directly instead of re-resolving the methodper flush. The constructor's error order is unchanged: the
writecheck stillprecedes the protocol-5
buffer_callbackcheck.This commit also moved the unpickler's stack and metastack to the constructor;
that half is reverted by
_pickle: allocate the unpickler stack and metastack per load againbelow. Reviewing the two commits together shows onlythe
Picklerchange.majit: drop two dead allocation sources in JitCodeBuilderadd_struct_field_descrdeep-copied the whole parentBhSizeSpec(one ownedStringper field) on every field-descr mint, althoughpatch_field_descr_parentsreplaces that snapshot with the final merged specand
struct_size_specsentries are only inserted or merged, never removed.register_struct_layoutrebuiltfield_specs_from_layouton each of its ~211calls for ~5 distinct layouts, where the merge pushes nothing and the following
re-sort/re-index are no-ops.
Measured on aheui's
mainloopfixed per-process init (never-tracing, 200iterations, min of 5 interleaved rounds): 243.7 → 214.6 us/call.
majit: report the JUMP block's declined close to the diag censusThe two sibling cross-loop close sites bump
bridge_declined_close(50) when anattempt is declined; the JUMP block's own
Declinedarm bumped nothing.Reported by CodeRabbit on #1040.
Follow-ups on this branch
jit: mint the tracing sentinel on the runtime GC condition, not cfg(test)The sentinel commit above guarded its unmanaged fallback with
#[cfg(test)],which is set only while
majit-metainterpcompiles its own test harness.Built as an ordinary dependency the arm disappears, so
pyre-jit-trace'smay_force_vable_escape_surfaces_typed_abortandmay_force_with_active_vable_executes_and_clears_token— which drive the tokenprotocol without a collector — reached the
assert_ne!and panicked.cargo buildstays green either way; the failing tests live in a different crate fromthe guard.
Branching on the unset type id itself is the profile-independent condition: the
leaf type is registered by the same setup that installs a collector, so an unset
id means there is no managed heap to mint the object in, and the host address
stays outside it where
is_managed_heap_objectrejects it before any tracingpath reads its header.
alloc_virtual_refin the same file already spells thiswindow the same way. The ordering requirement moves to
set_tracing_rescall_dummy_gc_type_id, which asserts the sentinel has notalready been minted — checked where it is provable rather than guessed from
cross-crate initialization order.
_pickle: record the measured write-resolution and constructor-order divergencesComments only.
interp_pickle.pyresolvesfile.writeat:555-560only tovalidate and re-resolves it per write in
_Framer.file_write(:353), andchecks
buffer_callbackbefore the file indescr__new__(:1822). Probed on3.14.5, neither holds — so both sites now carry the measurement:
pickle.Picklerwrite resolutionTypeError(write first)ValueError(buffer_callback first)TypeError_pickle: allocate the unpickler stack and metastack per load againinterp_pickle.py:2042-2043installs a freshstackandmetastackon everyload. CPython does reuse — but it reuses onePdatafor the unpickler'swhole lifetime, with MARK tracked by
num_marks/fenceindices inside thatone object (
_pickle.cPdata_New:454, :6888-6899); there is no metastack ofcontainers. Reuse is coherent only inside that structure. Layered onto the
metastack-of-lists port it produced a shape neither upstream has, and since
mark()rebindsw_stackto a fresh list, any load reaching STOP with an openMARK dropped the constructor's list entirely.
It is observable. Capturing the live stack during one
loadthroughgc.get_referentsand appending through it during a later one changes thatload's result:
load3(<function poison>, 'z')(1, 'z')Separately filed on the PR and not closed by this revert:
gc.get_referentsexposes pyre's internal unpickler lists as ordinary mutable
lists(
module/gc/mod.rs:213-224), so the same corruption is reachable inside asingle load. CPython hands out a
Pdata, which has noappend. That predatesthis branch.
Verification
pyre/check.pycargo test --all --no-default-features --features dynasmcargo test --workspace --features craneliftmay_forcetests, before/after the sentinel fix🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability
Diagnostics
Developer Experience