Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions majit/majit-gc/src/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ pub fn take_deferred_major_request() -> bool {
majit_ir::eval_breaker_word::take_gc()
}

/// Arm a major collection request for the next root-complete interpreter
/// dispatch. Hosts use this when collection is required for semantics rather
/// than because an allocation crossed the collector's threshold.
pub fn request_deferred_major_collection() {
majit_ir::eval_breaker_word::set_gc();
}

/// Configuration for the MiniMarkGC.
pub struct GcConfig {
/// Nursery size in bytes.
Expand Down
2 changes: 1 addition & 1 deletion pyre/cpython_tests/baseline.json
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,7 @@
"dynasm": "PASS"
},
"test.test_threading": {
"dynasm": "FAIL"
"dynasm": "PASS"
},
"test.test_threading_local": {
"dynasm": "PASS"
Expand Down
5 changes: 4 additions & 1 deletion pyre/pyre-interpreter/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2149,7 +2149,10 @@ fn eval_loop(frame: &mut PyFrame) -> PyResult {
// the flag is on and enough interpreter objects have accumulated.
// Without it, a JIT-off run reclaims interpreter-routed old-gen
// allocations only at explicit `gc.collect`, so RSS grows unbounded.
if dispatch_breaker & majit_ir::eval_breaker_word::EB_GC_INTERP != 0 {
if dispatch_breaker
& (majit_ir::eval_breaker_word::EB_GC_INTERP | majit_ir::eval_breaker_word::EB_GC)
!= 0
{
pyre_object::gc_interp::safepoint();
}
// Free-threaded stop-the-world rendezvous. Worker threads deliberately
Expand Down
28 changes: 11 additions & 17 deletions pyre/pyre-interpreter/src/module/posix/interp_posix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5927,23 +5927,17 @@ pub fn register_module(ns: pyre_object::PyObjectRef) {
Ok(0) => {
crate::module::thread::after_fork_child();
run_fork_callbacks("child");
// rposix.py `_exit` from the fork wrapper returns
// directly after `gc_thread_after_fork` and the
// registered child hooks. Do not introduce a
// full collection/finalizer drain here: arbitrary
// inherited Python objects may be mid-lifecycle,
// and PyPy only runs app-level finalizers at their
// ordinary safe points after the child resumes.
//
// What a collection here would buy is the stale
// `_MainThread` that `threading._after_fork` drops
// on return: a refcounting collector clears its
// `_dangling` weakref before `os.fork()` returns, a
// tracing one does not. That is not a defect to
// repair — pypy3 7.3.20 prints the same two
// `MainThread` entries this arm leaves behind, so
// `test_main_thread_after_fork_from_foreign_thread`
// and its dummy-thread twin fail there too.
// CPython's refcounting drops the replaced
// `_MainThread` before os.fork() returns, so its
// weakref has disappeared from `_dangling` when
// child Python code next runs. A tracing GC needs
// an explicit reachability pass for the same
// observable result. Defer a non-moving old-gen
// pass to the next bytecode boundary: collecting
// here would run while this native builtin still
// owns unregistered Rust-stack temporaries, and a
// moving full collection would be unsafe.
pyre_object::gc_interp::request_oldgen_collection();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not force an old-generation collection after fork

When automatic GC is disabled and the child inherits an unreachable cycle, this unconditional request still sweeps the entire old generation at the next opcode, clearing weakrefs and potentially scheduling finalizers that neither CPython nor PyPy runs merely because fork() returned. For example, after gc.disable(); a.self = a; r = weakref.ref(a); del a, CPython preserves r() in the child across os.fork(), while this path collects a; fixing _MainThread's acyclic refcount-timing difference with a process-wide tracing pass therefore changes unrelated observable lifetime semantics and conflicts with the required PyPy structural parity.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

drop(fork_serial);
Ok(pyre_object::w_int_new(0))
}
Expand Down
5 changes: 4 additions & 1 deletion pyre/pyre-jit/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7855,7 +7855,10 @@ fn eval_loop_jit(frame: &mut PyFrame) -> LoopResult {
// pyframe root walker; no bytecode handler holds a Rust-stack temporary
// here. A no-op unless the flag is on and enough interpreter objects
// have accumulated to warrant a collection.
if dispatch_breaker & majit_ir::eval_breaker_word::EB_GC_INTERP != 0 {
if dispatch_breaker
& (majit_ir::eval_breaker_word::EB_GC_INTERP | majit_ir::eval_breaker_word::EB_GC)
!= 0
{
pyre_object::gc_interp::safepoint();
}

Expand Down
62 changes: 47 additions & 15 deletions pyre/pyre-object/src/gc_interp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,25 +35,35 @@
//! turns it off where the env is readable.

use std::cell::Cell;
use std::sync::atomic::{AtomicU8, Ordering};
use std::sync::atomic::{AtomicBool, AtomicU8, Ordering};

/// Tri-state: 0 = not yet read from env, 1 = disabled, 2 = enabled.
static STATE: AtomicU8 = AtomicU8::new(0);

/// A semantic old-gen collection requested by a fork child callback.
///
/// This is process-global because the request belongs to the post-fork
/// interpreter, not to the native thread that happened to execute the hook.
/// Keep it outside the eval-breaker word while a re-entrant callback is too
/// deep to expose a complete root set; [`note_eval_activation_exit`] re-arms
/// the breaker once that callback has unwound.
static EXPLICIT_OLDGEN_REQUEST: AtomicBool = AtomicBool::new(false);

thread_local! {
/// Number of interpreter eval-loop activations currently on this
/// thread's call stack (both the plain `eval_loop` and the JIT
/// `eval_loop_jit`). Maintained only while the flag is on, via
/// [`EvalActivationGuard`]. The dispatch-loop safepoint consults it so
/// `eval_loop_jit`). The dispatch-loop safepoint consults it so
/// a collection only fires at the OUTERMOST activation — see
/// [`at_outermost_activation`].
static EVAL_NESTING: Cell<u32> = const { Cell::new(0) };
}

/// RAII guard that counts one interpreter eval-loop activation. Construct it
/// at the top of `eval_loop` / `eval_loop_jit`; the matching `Drop` rewinds
/// the depth on every exit path (normal return, `?`, unwind). A no-op when
/// the flag is off, so the un-gated interpreter pays nothing.
/// the depth on every exit path (normal return, `?`, unwind). `armed` records
/// whether the ordinary interpreter-allocation GC integration is enabled;
/// nesting itself is always tracked because an explicit post-fork request must
/// obey the same root-completeness rule when that feature is off.
pub struct EvalActivationGuard {
armed: bool,
}
Expand All @@ -73,16 +83,21 @@ pub fn note_eval_activation_enter() {
/// same residual contract.
#[majit_macros::dont_look_inside]
pub fn note_eval_activation_exit() {
EVAL_NESTING.with(|d| d.set(d.get().saturating_sub(1)));
let depth = EVAL_NESTING.with(|d| {
let depth = d.get().saturating_sub(1);
d.set(depth);
depth
});
if depth <= 2 && EXPLICIT_OLDGEN_REQUEST.load(Ordering::Acquire) {
majit_gc::collector::request_deferred_major_collection();
}
}

impl EvalActivationGuard {
#[inline]
pub fn enter() -> Self {
let armed = enabled();
if armed {
note_eval_activation_enter();
}
note_eval_activation_enter();
Self { armed }
}

Expand All @@ -95,9 +110,7 @@ impl EvalActivationGuard {
impl Drop for EvalActivationGuard {
#[inline]
fn drop(&mut self) {
if self.armed {
note_eval_activation_exit();
}
note_eval_activation_exit();
}
}

Expand Down Expand Up @@ -265,14 +278,24 @@ pub fn would_collect() -> bool {
/// residual adds no hot-path cost the un-residualized form did not already pay.
#[majit_macros::dont_look_inside]
pub fn safepoint() {
if !enabled() {
return;
}
// Take any request the old-gen allocator armed, and take it before the
// decision below: the bit is a compiled loop's deopt trigger, so one left
// armed by a safepoint that declined to collect fires again on the next
// back edge, and the next.
let requested = majit_gc::collector::take_deferred_major_request();
if EXPLICIT_OLDGEN_REQUEST.load(Ordering::Acquire) {
if !at_outermost_activation() {
// Leave the semantic request pending outside the breaker word.
// Re-arming here would deopt every compiled back edge until the
// native callback unwinds; the activation guard does it once on
// the eligible transition instead.
return;
}
if EXPLICIT_OLDGEN_REQUEST.swap(false, Ordering::AcqRel) {
crate::gc_hook::try_gc_collect_oldgen();
return;
}
}
if !would_collect() {
return;
}
Expand All @@ -285,6 +308,15 @@ pub fn safepoint() {
}
}

/// Ask the next root-complete bytecode dispatch to run a non-moving old-gen
/// collection. Unlike [`majit_gc::collect_full`], this cannot relocate a
/// nursery reference held by the caller while it unwinds back to the loop.
#[majit_macros::dont_look_inside]
pub fn request_oldgen_collection() {
EXPLICIT_OLDGEN_REQUEST.store(true, Ordering::Release);
majit_gc::collector::request_deferred_major_collection();
}

/// Whether the safepoint actually collects. Off via `PYRE_GC_INTERP_COLLECT=0`
/// to isolate the allocation routing from the collection while diagnosing.
///
Expand Down
Loading