Visitar URL original
Keep automatic GC requests local to each interpreter (#8902) · RustPython/RustPython@b09402a · GitHub
Skip to content

Commit b09402a

Browse files
1ndahous31ndahous3
authored
Keep automatic GC requests local to each interpreter (#8902)
Assisted-by: Codex:GPT-6
1 parent 55a5f1c commit b09402a

3 files changed

Lines changed: 99 additions & 20 deletions

File tree

‎crates/vm/src/gc_state.rs‎

Lines changed: 95 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,7 @@ impl GcGeneration {
135135
/// empties its own interpreter's objects; another interpreter's stay behind with
136136
/// the count already zeroed, and untracking one of those must not wrap.
137137
fn release_count(count: &AtomicUsize) {
138-
if count.load(Ordering::Relaxed) > 0 {
139-
count.fetch_sub(1, Ordering::Relaxed);
140-
}
138+
let _ = count.try_update(Ordering::Relaxed, Ordering::Relaxed, |n| n.checked_sub(1));
141139
}
142140

143141
/// Whether `owner`'s collections act on `obj`.
@@ -552,7 +550,7 @@ impl GcState {
552550
// (e.g. a lazily-initialized frame locals cell) that another
553551
// thread is blocked on with no way to reach a safepoint —
554552
// a deadlock. At a safepoint no such lock is held.
555-
crate::signal::schedule_gc();
553+
gc.scheduled.store(true, Ordering::Relaxed);
556554
return false;
557555
}
558556
// Without threading there is no safepoint to defer to and no other
@@ -574,6 +572,8 @@ impl GcState {
574572
force: bool,
575573
) -> CollectResult {
576574
if !force && !gc.is_enabled() {
575+
#[cfg(feature = "threading")]
576+
gc.scheduled.store(false, Ordering::Relaxed);
577577
return CollectResult::default();
578578
}
579579

@@ -582,6 +582,12 @@ impl GcState {
582582
return CollectResult::default();
583583
};
584584

585+
// A busy collector must not consume another interpreter's request.
586+
// Clear only after acquiring the lock, before callbacks can request
587+
// a later collection.
588+
#[cfg(feature = "threading")]
589+
gc.scheduled.store(false, Ordering::Relaxed);
590+
585591
let start_time = cfg_select! {
586592
target_arch = "wasm32" => (),
587593
_ => std::time::Instant::now(),
@@ -1268,6 +1274,9 @@ pub struct GcInterpreterState {
12681274
pub generations: [GcGeneration; 3],
12691275
/// GC enabled flag
12701276
enabled: AtomicBool,
1277+
/// Automatic collection requested at this interpreter's next safepoint.
1278+
#[cfg(feature = "threading")]
1279+
scheduled: AtomicBool,
12711280
/// Debug flags
12721281
debug: AtomicU32,
12731282
/// Uncollectable objects saved by this interpreter's collections, drained
@@ -1289,6 +1298,8 @@ impl GcInterpreterState {
12891298
GcGeneration::new(0), // old[1]
12901299
],
12911300
enabled: AtomicBool::new(true),
1301+
#[cfg(feature = "threading")]
1302+
scheduled: AtomicBool::new(false),
12921303
debug: AtomicU32::new(0),
12931304
garbage: PyMutex::new(Vec::new()),
12941305
py_garbage: ctx.new_list(Vec::new()),
@@ -1304,6 +1315,14 @@ impl GcInterpreterState {
13041315
self.enabled.load(Ordering::Relaxed)
13051316
}
13061317

1318+
/// Leave requests pending while a collector is busy, without repeatedly
1319+
/// entering the bytecode loop's slow path during its Python callbacks.
1320+
#[cfg(feature = "threading")]
1321+
#[inline]
1322+
pub(crate) fn collection_ready(&self) -> bool {
1323+
self.scheduled.load(Ordering::Relaxed) && !gc_state().collecting.is_locked()
1324+
}
1325+
13071326
/// Enable GC
13081327
pub fn enable(&self) {
13091328
self.enabled.store(true, Ordering::Relaxed);
@@ -1524,4 +1543,76 @@ mod tests {
15241543
assert_ne!(first.owner, GC_NO_OWNER);
15251544
assert_ne!(second.owner, GC_NO_OWNER);
15261545
}
1546+
1547+
#[cfg(feature = "threading")]
1548+
#[test]
1549+
fn automatic_gc_request_stays_with_allocating_interpreter() {
1550+
let first = crate::Interpreter::without_stdlib(Default::default());
1551+
let second = crate::Interpreter::without_stdlib(Default::default());
1552+
first.enter(|vm| vm.state.gc.scheduled.store(false, Ordering::Relaxed));
1553+
second.enter(|vm| vm.state.gc.scheduled.store(false, Ordering::Relaxed));
1554+
// An isolated allocation counter makes crossing the threshold
1555+
// deterministic without depending on the rest of the test process.
1556+
let allocations = GcState::new();
1557+
allocations.counts[0].store(1, Ordering::Relaxed);
1558+
first.enter(|vm| {
1559+
vm.state.gc.set_threshold(1, None, None);
1560+
assert!(!allocations.maybe_collect(&vm.state.gc));
1561+
});
1562+
second.enter(|vm| {
1563+
assert!(!vm.state.gc.scheduled.load(Ordering::Relaxed));
1564+
vm.run_scheduled_gc();
1565+
});
1566+
first.enter(|vm| {
1567+
assert!(vm.state.gc.scheduled.swap(false, Ordering::Relaxed));
1568+
});
1569+
}
1570+
1571+
#[cfg(feature = "threading")]
1572+
#[test]
1573+
fn automatic_gc_request_survives_busy_collector() {
1574+
let state = interpreter_state();
1575+
let _guard = gc_state().collecting.lock();
1576+
state.scheduled.store(true, Ordering::Relaxed);
1577+
state.collect(0);
1578+
assert!(state.scheduled.load(Ordering::Relaxed));
1579+
assert!(!state.collection_ready());
1580+
1581+
state.disable();
1582+
state.collect(0);
1583+
assert!(!state.scheduled.load(Ordering::Relaxed));
1584+
}
1585+
1586+
#[test]
1587+
fn release_count_does_not_wrap_during_reset() {
1588+
use std::sync::Barrier;
1589+
1590+
let count = AtomicUsize::new(1);
1591+
let start = Barrier::new(3);
1592+
let finish = Barrier::new(3);
1593+
let mut underflows = 0;
1594+
std::thread::scope(|scope| {
1595+
for reset in [false, true] {
1596+
let (count, start, finish) = (&count, &start, &finish);
1597+
scope.spawn(move || {
1598+
for _ in 0..10_000 {
1599+
start.wait();
1600+
if reset {
1601+
count.store(0, Ordering::Relaxed);
1602+
} else {
1603+
release_count(count);
1604+
}
1605+
finish.wait();
1606+
}
1607+
});
1608+
}
1609+
for _ in 0..10_000 {
1610+
count.store(1, Ordering::Relaxed);
1611+
start.wait();
1612+
finish.wait();
1613+
underflows += usize::from(count.load(Ordering::Relaxed) > 1);
1614+
}
1615+
});
1616+
assert_eq!(underflows, 0);
1617+
}
15271618
}

‎crates/vm/src/signal.rs‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ bitflagset::bitflag! {
3131
enum EvalBreakerFlag {
3232
Signal = 0,
3333
Qsbr = 1,
34-
Gc = 2,
3534
Stop = 3,
3635
Finalizing = 4,
3736
}
@@ -189,20 +188,10 @@ mod mt {
189188
EVAL_BREAKER.insert(EvalBreakerFlag::Finalizing);
190189
}
191190

192-
/// Schedule an automatic collection to run at the next bytecode safepoint.
193-
pub(crate) fn schedule_gc() {
194-
EVAL_BREAKER.insert(EvalBreakerFlag::Gc);
195-
}
196-
197-
/// Clear the scheduled-GC bit, returning whether it had been set.
198-
pub(crate) fn take_gc_scheduled() -> bool {
199-
EVAL_BREAKER.remove(EvalBreakerFlag::Gc)
200-
}
201-
202191
/// Drop every process-wide eval-breaker bit. Tests that assert a single
203192
/// thread's `stop_requested` must not trip `eval_breaker_pending` have to
204193
/// start from a clean word: cargo's Windows runner shares the process
205-
/// across `#[test]` functions, so a sibling can leave SIGNAL/QSBR/GC/STOP.
194+
/// across `#[test]` functions, so a sibling can leave SIGNAL/QSBR/STOP.
206195
#[cfg(test)]
207196
pub(crate) fn clear_eval_breaker_for_test() {
208197
EVAL_BREAKER.clear();
@@ -211,8 +200,7 @@ mod mt {
211200

212201
#[cfg(feature = "threading")]
213202
pub(crate) use mt::{
214-
clear_qsbr_bit, clear_stop_bit, qsbr_bit_set, schedule_gc, set_finalizing_bit, set_qsbr_bit,
215-
set_stop_bit, take_gc_scheduled,
203+
clear_qsbr_bit, clear_stop_bit, qsbr_bit_set, set_finalizing_bit, set_qsbr_bit, set_stop_bit,
216204
};
217205

218206
#[cfg(all(test, feature = "threading"))]

‎crates/vm/src/vm/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3545,7 +3545,7 @@ impl VirtualMachine {
35453545
#[inline]
35463546
pub(crate) fn eval_breaker_tripped(&self) -> bool {
35473547
#[cfg(feature = "threading")]
3548-
if thread::stop_requested_for_current_thread() {
3548+
if thread::stop_requested_for_current_thread() || self.state.gc.collection_ready() {
35493549
return true;
35503550
}
35513551
#[cfg(not(target_arch = "wasm32"))]
@@ -3594,7 +3594,7 @@ impl VirtualMachine {
35943594
/// against a thread blocked on a lock this thread would otherwise hold.
35953595
#[cfg(feature = "threading")]
35963596
pub(crate) fn run_scheduled_gc(&self) {
3597-
if crate::signal::take_gc_scheduled() {
3597+
if self.state.gc.collection_ready() {
35983598
self.state.gc.collect(0);
35993599
}
36003600
}

0 commit comments

Comments
 (0)