Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
32 changes: 32 additions & 0 deletions crates/vm/src/builtins/type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,33 @@ unsafe impl crate::object::Traverse for PyType {
.map(|(_, v)| v.traverse(tracer_fn))
.count();
}

/// type_clear: break reference cycles in type objects
fn clear(&mut self, out: &mut Vec<crate::PyObjectRef>) {
if let Some(base) = self.base.take() {
out.push(base.into());
}
if let Some(mut guard) = self.bases.try_write() {
for base in guard.drain(..) {
out.push(base.into());
}
}
if let Some(mut guard) = self.mro.try_write() {
for typ in guard.drain(..) {
out.push(typ.into());
}
}
if let Some(mut guard) = self.subclasses.try_write() {
for weak in guard.drain(..) {
out.push(weak.into());
}
}
if let Some(mut guard) = self.attributes.try_write() {
for (_, val) in guard.drain(..) {
out.push(val);
}
}
}
}

// PyHeapTypeObject in CPython
Expand Down Expand Up @@ -393,6 +420,11 @@ impl PyType {
metaclass,
None,
);

// Static types are not tracked by GC.
// They are immortal and never participate in collectable cycles.
new_type.as_object().clear_gc_tracked();

Comment on lines +424 to +427
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Untrack static types from GC state as well.

Clearing only the TRACKED bit leaves the type in GcState sets, so gc.get_objects/generation counts can still include it and diverge from is_gc_tracked(). Please also remove it from the tracking sets (similar to weakref_type).

💡 Suggested fix
-        new_type.as_object().clear_gc_tracked();
+        let obj = new_type.as_object();
+        unsafe { crate::gc_state::gc_state().untrack_object(NonNull::from(obj)); }
+        obj.clear_gc_tracked();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Static types are not tracked by GC.
// They are immortal and never participate in collectable cycles.
new_type.as_object().clear_gc_tracked();
// Static types are not tracked by GC.
// They are immortal and never participate in collectable cycles.
let obj = new_type.as_object();
unsafe { crate::gc_state::gc_state().untrack_object(NonNull::from(obj)); }
obj.clear_gc_tracked();
🤖 Prompt for AI Agents
In `@crates/vm/src/builtins/type.rs` around lines 424 - 427, When marking static
types immortal, in addition to calling new_type.as_object().clear_gc_tracked()
you must also remove the object from the VM's GC tracking sets in GcState so it
no longer appears in gc.get_objects or generation counts; update the same block
(the one handling static types) to call the same removal logic used for
weakref_type (i.e., remove the object from the tracking sets / GcState tracking
collections) immediately after clear_gc_tracked() so is_gc_tracked() and the GC
sets stay consistent.

new_type.mro.write().insert(0, new_type.clone());

// Note: inherit_slots is called in PyClassImpl::init_class after
Expand Down
4 changes: 4 additions & 0 deletions crates/vm/src/gc_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,10 @@ impl GcState {
pub unsafe fn track_object(&self, obj: NonNull<PyObject>) {
let gc_ptr = GcObjectPtr(obj);

// _PyObject_GC_TRACK
let obj_ref = unsafe { obj.as_ref() };
obj_ref.set_gc_tracked();

// Add to generation 0 tracking first (for correct gc_refs algorithm)
// Only increment count if we successfully add to the set
if let Ok(mut gen0) = self.generation_objects[0].write()
Expand Down
78 changes: 66 additions & 12 deletions crates/vm/src/object/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,28 @@ use core::{
#[derive(Debug)]
pub(super) struct Erased;

/// Default dealloc: handles __del__, weakref clearing, and memory free.
/// Default dealloc: handles __del__, weakref clearing, tp_clear, and memory free.
/// Equivalent to subtype_dealloc in CPython.
pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
let obj_ref = unsafe { &*(obj as *const PyObject) };
if let Err(()) = obj_ref.drop_slow_inner() {
return; // resurrected by __del__
}

// Extract child references before deallocation to break circular refs (tp_clear).
// This ensures that when edges are dropped after the object is freed,
// any pointers back to this object are already gone.
let mut edges = Vec::new();
if let Some(clear_fn) = obj_ref.0.vtable.clear {
unsafe { clear_fn(obj, &mut edges) };
}

// Deallocate the object memory
drop(unsafe { Box::from_raw(obj as *mut PyInner<T>) });

// Drop child references - may trigger recursive destruction.
// The object is already deallocated, so circular refs are broken.
drop(edges);
}
pub(super) unsafe fn debug_obj<T: PyPayload + core::fmt::Debug>(
x: &PyObject,
Expand All @@ -105,6 +119,12 @@ pub(super) unsafe fn try_traverse_obj<T: PyPayload>(x: &PyObject, tracer_fn: &mu
payload.try_traverse(tracer_fn)
}

/// Call `try_clear` on payload to extract child references (tp_clear)
pub(super) unsafe fn try_clear_obj<T: PyPayload>(x: *mut PyObject, out: &mut Vec<PyObjectRef>) {
let x = unsafe { &mut *(x as *mut PyInner<T>) };
x.payload.try_clear(out);
}

bitflags::bitflags! {
/// GC bits for free-threading support (like ob_gc_bits in Py_GIL_DISABLED)
/// These bits are stored in a separate atomic field for lock-free access.
Expand Down Expand Up @@ -963,10 +983,27 @@ impl PyObject {
/// _PyGC_SET_FINALIZED in Py_GIL_DISABLED mode.
#[inline]
fn set_gc_finalized(&self) {
// Atomic RMW to avoid clobbering other concurrent bit updates.
self.set_gc_bit(GcBits::FINALIZED);
}

/// Set a GC bit atomically.
#[inline]
pub(crate) fn set_gc_bit(&self, bit: GcBits) {
self.0.gc_bits.fetch_or(bit.bits(), Ordering::Relaxed);
}

/// _PyObject_GC_TRACK
#[inline]
pub(crate) fn set_gc_tracked(&self) {
self.set_gc_bit(GcBits::TRACKED);
}

/// _PyObject_GC_UNTRACK
#[inline]
pub(crate) fn clear_gc_tracked(&self) {
self.0
.gc_bits
.fetch_or(GcBits::FINALIZED.bits(), Ordering::Relaxed);
.fetch_and(!GcBits::TRACKED.bits(), Ordering::Relaxed);
}

#[inline(always)] // the outer function is never inlined
Expand Down Expand Up @@ -1046,13 +1083,9 @@ impl PyObject {
*self.0.slots[offset].write() = value;
}

/// Check if this object is tracked by the garbage collector.
/// Returns true if the object has a trace function or has an instance dict.
/// _PyObject_GC_IS_TRACKED
pub fn is_gc_tracked(&self) -> bool {
if self.0.vtable.trace.is_some() {
return true;
}
self.0.dict.is_some()
GcBits::from_bits_retain(self.0.gc_bits.load(Ordering::Relaxed)).contains(GcBits::TRACKED)
}

/// Get the referents (objects directly referenced) of this object.
Expand Down Expand Up @@ -1277,13 +1310,28 @@ impl<T: PyPayload> PyRef<T> {
}
}

impl<T: PyPayload + core::fmt::Debug> PyRef<T> {
impl<T: PyPayload + crate::object::MaybeTraverse + core::fmt::Debug> PyRef<T> {
#[inline(always)]
pub fn new_ref(payload: T, typ: crate::builtins::PyTypeRef, dict: Option<PyDictRef>) -> Self {
let has_dict = dict.is_some();
let is_heaptype = typ.heaptype_ext.is_some();
let inner = Box::into_raw(PyInner::new(payload, typ, dict));
Self {
ptr: unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) },
let ptr = unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) };

// Track object if:
// - HAS_TRAVERSE is true (Rust payload implements Traverse), OR
// - has instance dict (user-defined class instances), OR
// - heap type (all heap type instances are GC-tracked, like Py_TPFLAGS_HAVE_GC)
if <T as crate::object::MaybeTraverse>::HAS_TRAVERSE || has_dict || is_heaptype {
let gc = crate::gc_state::gc_state();
unsafe {
gc.track_object(ptr.cast());
}
// Check if automatic GC should run
gc.maybe_collect();
}

Self { ptr }
}
}

Expand Down Expand Up @@ -1546,6 +1594,12 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
heaptype_ext: None,
};
let weakref_type = PyRef::new_ref(weakref_type, type_type.clone(), None);
// Static type: untrack from GC (was tracked by new_ref because PyType has HAS_TRAVERSE)
unsafe {
crate::gc_state::gc_state()
.untrack_object(core::ptr::NonNull::from(weakref_type.as_object()));
}
weakref_type.as_object().clear_gc_tracked();
// weakref's mro is [weakref, object]
weakref_type.mro.write().insert(0, weakref_type.clone());

Expand Down
14 changes: 12 additions & 2 deletions crates/vm/src/object/traverse_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ use alloc::fmt;
use core::any::TypeId;

use crate::{
PyObject,
PyObject, PyObjectRef,
object::{
Erased, InstanceDict, MaybeTraverse, PyInner, PyObjectPayload, debug_obj, default_dealloc,
try_traverse_obj,
try_clear_obj, try_traverse_obj,
},
};

Expand All @@ -17,6 +17,9 @@ pub(in crate::object) struct PyObjVTable {
pub(in crate::object) dealloc: unsafe fn(*mut PyObject),
pub(in crate::object) debug: unsafe fn(&PyObject, &mut fmt::Formatter<'_>) -> fmt::Result,
pub(in crate::object) trace: Option<unsafe fn(&PyObject, &mut TraverseFn<'_>)>,
/// Clear for circular reference resolution (tp_clear).
/// Called just before deallocation to extract child references.
pub(in crate::object) clear: Option<unsafe fn(*mut PyObject, &mut Vec<PyObjectRef>)>,
}

impl PyObjVTable {
Expand All @@ -32,6 +35,13 @@ impl PyObjVTable {
None
}
},
clear: const {
if T::HAS_CLEAR {
Some(try_clear_obj::<T>)
} else {
None
}
},
}
}
}
Expand Down
Loading