Skip to content

Commit 82a7be9

Browse files
youknowoneyouknowone
committed
gc: size the interpreter owner tag to the header padding
`gc_owner` was a u32. With 4-byte pointers its alignment pushed it out of the padding that follows the gc bits and generation, growing every object by a word and tripping the `SIZEOF_PYOBJECT_HEAD` assertion on 32-bit targets. Introduce `GcOwner = u16`, which the `repr(C)` layout places at offset 10 on 32-bit and 18 on 64-bit, leaving the header at 6 words on both. Tags now run out after 65535 interpreters; `alloc_owner` already falls back to `GC_NO_OWNER`, so an interpreter past that collects as it did before tagging. Also widen three test deadlines that measure liveness, not speed. Assisted-by: Claude
1 parent ce3d5ec commit 82a7be9

4 files changed

Lines changed: 31 additions & 23 deletions

File tree

‎crates/vm/src/gc_state.rs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
55
use crate::common::linked_list::LinkedList;
66
use crate::common::lock::{PyMutex, PyRwLock};
7-
use crate::object::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink};
7+
use crate::object::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink, GcOwner};
88
use crate::{AsObject, PyObject, PyObjectRef};
99
use core::ptr::NonNull;
10-
use core::sync::atomic::{AtomicBool, AtomicU32, AtomicUsize, Ordering};
10+
use core::sync::atomic::{AtomicBool, AtomicU16, AtomicU32, AtomicUsize, Ordering};
1111
use std::collections::HashSet;
1212

1313
fn elapsed_secs(
@@ -143,7 +143,7 @@ fn release_count(count: &AtomicUsize) {
143143
///
144144
/// Objects with no owner — everything the shared context allocates, and anything
145145
/// allocated with no interpreter current — belong to all of them.
146-
fn is_owned_by(obj: &PyObject, owner: u32) -> bool {
146+
fn is_owned_by(obj: &PyObject, owner: GcOwner) -> bool {
147147
let obj_owner = obj.gc_owner();
148148
obj_owner == owner || obj_owner == GC_NO_OWNER
149149
}
@@ -262,11 +262,11 @@ pub struct GcState {
262262
/// Allocation counter for gen0
263263
alloc_count: AtomicUsize,
264264
/// Next `gc_owner` tag to hand to an interpreter.
265-
next_owner: AtomicU32,
265+
next_owner: AtomicU16,
266266
/// Tags of interpreters that are gone. Their objects outlived them, so a
267267
/// collection adopts them — tags them `GC_NO_OWNER` again — as it walks,
268268
/// rather than leaving them for a collector that will never come.
269-
retired: PyMutex<Vec<u32>>,
269+
retired: PyMutex<Vec<GcOwner>>,
270270
}
271271

272272
// SAFETY: All fields are either inherently Send/Sync (atomics, RwLock, Mutex) or protected by PyMutex.
@@ -300,15 +300,15 @@ impl GcState {
300300
permanent_count: AtomicUsize::new(0),
301301
collecting: PyMutex::new(()),
302302
alloc_count: AtomicUsize::new(0),
303-
next_owner: AtomicU32::new(GC_NO_OWNER + 1),
303+
next_owner: AtomicU16::new(GC_NO_OWNER + 1),
304304
retired: PyMutex::new(Vec::new()),
305305
}
306306
}
307307

308308
/// Reserve a tag for a new interpreter. Tags are never reused; exhausting
309309
/// the 32-bit space falls back to `GC_NO_OWNER`, which costs isolation but
310310
/// stays correct, rather than aliasing a live interpreter.
311-
fn alloc_owner(&self) -> u32 {
311+
fn alloc_owner(&self) -> GcOwner {
312312
self.next_owner
313313
.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |next| {
314314
next.checked_add(1)
@@ -320,7 +320,7 @@ impl GcState {
320320
/// whatever it left behind. Retagging the objects here would mean walking
321321
/// every list under an interpreter drop, which happens while a collection
322322
/// holds the collecting lock.
323-
fn retire_owner(&self, owner: u32) {
323+
fn retire_owner(&self, owner: GcOwner) {
324324
if owner == GC_NO_OWNER {
325325
return;
326326
}
@@ -343,7 +343,7 @@ impl GcState {
343343
///
344344
/// # Safety
345345
/// obj must be a valid pointer to a PyObject
346-
pub unsafe fn track_object(&self, obj: NonNull<PyObject>, owner: u32) {
346+
pub unsafe fn track_object(&self, obj: NonNull<PyObject>, owner: GcOwner) {
347347
let obj_ref = unsafe { obj.as_ref() };
348348
obj_ref.set_gc_tracked();
349349
obj_ref.set_gc_generation(0);
@@ -407,10 +407,10 @@ impl GcState {
407407
/// interpreter owns.
408408
/// If generation is None, returns all such objects.
409409
/// If generation is Some(n), returns those in generation n only.
410-
pub fn get_objects(&self, generation: Option<i32>, owner: u32) -> Vec<PyObjectRef> {
410+
pub fn get_objects(&self, generation: Option<i32>, owner: GcOwner) -> Vec<PyObjectRef> {
411411
fn collect_from_list(
412412
list: &LinkedList<GcLink, PyObject>,
413-
owner: u32,
413+
owner: GcOwner,
414414
) -> impl Iterator<Item = PyObjectRef> + '_ {
415415
list.iter()
416416
.filter(move |obj| is_owned_by(obj, owner))
@@ -1060,7 +1060,7 @@ impl GcState {
10601060
/// Freeze the objects `owner` could collect (move them to the permanent
10611061
/// generation).
10621062
/// Lock order: generation_lists[i] → permanent_list (consistent with unfreeze).
1063-
fn freeze(&self, owner: u32) {
1063+
fn freeze(&self, owner: GcOwner) {
10641064
let mut count = 0usize;
10651065

10661066
for (gen_idx, gen_list) in self.generation_lists.iter().enumerate() {
@@ -1087,7 +1087,7 @@ impl GcState {
10871087

10881088
/// Unfreeze the objects `owner` froze (move them from permanent to gen2).
10891089
/// Lock order: generation_lists[2] → permanent_list (consistent with freeze).
1090-
fn unfreeze(&self, owner: u32) {
1090+
fn unfreeze(&self, owner: GcOwner) {
10911091
let mut count = 0usize;
10921092

10931093
{
@@ -1148,7 +1148,7 @@ impl GcState {
11481148
/// objects end up.
11491149
pub struct GcInterpreterState {
11501150
/// Tag written into every object this interpreter tracks.
1151-
owner: u32,
1151+
owner: GcOwner,
11521152
/// Per-generation thresholds and statistics.
11531153
pub generations: [GcGeneration; 3],
11541154
/// GC enabled flag
@@ -1288,7 +1288,7 @@ impl Drop for GcInterpreterState {
12881288

12891289
/// The tag `track_object` should write for the interpreter running now.
12901290
#[must_use]
1291-
pub fn current_owner() -> u32 {
1291+
pub fn current_owner() -> GcOwner {
12921292
// SAFETY: the pointee is owned by the `PyGlobalState` of the VM on top of
12931293
// this thread's VM stack, which outlives the section this call runs in.
12941294
crate::vm::thread::current_gc_state().map_or(GC_NO_OWNER, |gc| unsafe { gc.as_ref() }.owner)

‎crates/vm/src/object/core.rs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -303,10 +303,18 @@ bitflags::bitflags! {
303303
/// GC generation constants
304304
pub(crate) const GC_UNTRACKED: u8 = 0xFF;
305305
pub(crate) const GC_PERMANENT: u8 = 3;
306+
/// Width of an interpreter's `gc_owner` tag.
307+
///
308+
/// Sized to the padding the header alignment already forces, so the tag costs
309+
/// no space on either pointer width. Running out of tags is not an error: an
310+
/// interpreter that gets none uses [`GC_NO_OWNER`] and its objects stay
311+
/// collectable by every interpreter, which is how they behaved before tagging.
312+
pub(crate) type GcOwner = u16;
313+
306314
/// `gc_owner` of an object that belongs to no single interpreter: everything
307315
/// the shared context allocates, and anything allocated with no interpreter
308316
/// current. Every interpreter collects these.
309-
pub(crate) const GC_NO_OWNER: u32 = 0;
317+
pub(crate) const GC_NO_OWNER: GcOwner = 0;
310318

311319
/// Link implementation for GC intrusive linked list tracking
312320
pub(crate) struct GcLink;
@@ -396,7 +404,7 @@ pub(super) struct PyInner<T> {
396404
/// Interpreter that tracked this object, or `GC_NO_OWNER`. Written by
397405
/// `track_object`; read to scope a collection to one interpreter.
398406
/// Sits in what would otherwise be padding, so it costs no space.
399-
pub(super) gc_owner: PyAtomic<u32>,
407+
pub(super) gc_owner: PyAtomic<GcOwner>,
400408
/// Intrusive linked list pointers for GC generational tracking
401409
pub(super) gc_pointers: Pointers<PyObject>,
402410

@@ -1751,15 +1759,15 @@ impl PyObject {
17511759

17521760
/// The interpreter whose collections consider this object.
17531761
#[inline]
1754-
pub(crate) fn gc_owner(&self) -> u32 {
1762+
pub(crate) fn gc_owner(&self) -> GcOwner {
17551763
self.0.gc_owner.load(Ordering::Relaxed)
17561764
}
17571765

17581766
/// Set the owning interpreter. Written by `track_object` before the object
17591767
/// enters a generation list, and reset to `GC_NO_OWNER` when the owning
17601768
/// interpreter goes away.
17611769
#[inline]
1762-
pub(crate) fn set_gc_owner(&self, owner: u32) {
1770+
pub(crate) fn set_gc_owner(&self, owner: GcOwner) {
17631771
self.0.gc_owner.store(owner, Ordering::Relaxed);
17641772
}
17651773

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,5 @@ pub use self::core::*;
99
pub use self::ext::*;
1010
pub use self::payload::*;
1111
pub(crate) use core::SIZEOF_PYOBJECT_HEAD;
12-
pub(crate) use core::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink};
12+
pub(crate) use core::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink, GcOwner};
1313
pub use traverse::{MaybeTraverse, Traverse, TraverseFn};

‎crates/vm/src/vm/interpreter.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -823,7 +823,7 @@ mod tests {
823823
use core::time::Duration;
824824
use std::time::Instant;
825825

826-
let deadline = Instant::now() + Duration::from_secs(5);
826+
let deadline = Instant::now() + Duration::from_secs(30);
827827
while runtime::lookup_interpreter(id).is_some() {
828828
assert!(
829829
Instant::now() < deadline,
@@ -1069,7 +1069,7 @@ mod tests {
10691069
let sub_worker = spawn_worker(&sub);
10701070

10711071
let (lock, ready) = &*state;
1072-
let deadline = Instant::now() + Duration::from_secs(2);
1072+
let deadline = Instant::now() + Duration::from_secs(30);
10731073
let mut state_guard = lock.lock().unwrap();
10741074
while state_guard.entered < 2 {
10751075
let now = Instant::now();
@@ -1115,7 +1115,7 @@ mod tests {
11151115
std::thread::spawn(move || {
11161116
thread_vm.run(|vm| {
11171117
main_started_worker.store(true, Ordering::Release);
1118-
let deadline = Instant::now() + Duration::from_secs(2);
1118+
let deadline = Instant::now() + Duration::from_secs(30);
11191119
let mut operations = 0;
11201120
while !sub_finished_worker.load(Ordering::Acquire) && Instant::now() < deadline
11211121
{

0 commit comments

Comments
 (0)