Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions .cspell.dict/cpython.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ finalizers
firsttraceable
flowgraph
formatfloat
freelist
freevar
freevars
fromlist
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use crate::{
vm::VirtualMachine,
};
use alloc::fmt;
use core::cell::Cell;
use core::ptr::NonNull;
use rustpython_common::lock::PyMutex;
use rustpython_common::wtf8::Wtf8Buf;

Expand Down Expand Up @@ -60,11 +62,43 @@ impl fmt::Debug for PyDict {
}
}

thread_local! {
static DICT_FREELIST: Cell<Vec<*mut PyObject>> = const { Cell::new(Vec::new()) };
}

impl PyPayload for PyDict {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.dict_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
DICT_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
DICT_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl PyDict {
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/float.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use crate::{
protocol::PyNumberMethods,
types::{AsNumber, Callable, Comparable, Constructor, Hashable, PyComparisonOp, Representable},
};
use core::cell::Cell;
use core::ptr::NonNull;
use malachite_bigint::{BigInt, ToBigInt};
use num_complex::Complex64;
use num_traits::{Signed, ToPrimitive, Zero};
Expand All @@ -32,11 +34,43 @@ impl PyFloat {
}
}

thread_local! {
static FLOAT_FREELIST: Cell<Vec<*mut PyObject>> = const { Cell::new(Vec::new()) };
}

impl PyPayload for PyFloat {
const MAX_FREELIST: usize = 100;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.float_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
FLOAT_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
FLOAT_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl ToPyObject for f64 {
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ use crate::{
use rustpython_common::wtf8::Wtf8Buf;

use alloc::fmt;
use core::cell::Cell;
use core::ops::DerefMut;
use core::ptr::NonNull;

#[pyclass(module = false, name = "list", unhashable = true, traverse = "manual")]
#[derive(Default)]
Expand Down Expand Up @@ -72,11 +74,43 @@ unsafe impl Traverse for PyList {
}
}

thread_local! {
static LIST_FREELIST: Cell<Vec<*mut PyObject>> = const { Cell::new(Vec::new()) };
}

impl PyPayload for PyList {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.list_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
LIST_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
LIST_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl ToPyObject for Vec<PyObjectRef> {
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/slice.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// sliceobject.{h,c} in CPython
// spell-checker:ignore sliceobject
use core::cell::Cell;
use core::ptr::NonNull;
use rustpython_common::wtf8::{Wtf8Buf, wtf8_concat};

use super::{PyGenericAlias, PyStrRef, PyTupleRef, PyType, PyTypeRef};
Expand All @@ -23,11 +25,43 @@ pub struct PySlice {
pub step: Option<PyObjectRef>,
}

thread_local! {
static SLICE_FREELIST: Cell<Vec<*mut PyObject>> = const { Cell::new(Vec::new()) };
}

impl PyPayload for PySlice {
const MAX_FREELIST: usize = 1;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.slice_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
SLICE_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
SLICE_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
Comment on lines +48 to +84
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if PySlice has a proper clear/tp_clear implementation

# Search for Traverse impl on PySlice
echo "=== PySlice Traverse implementation ==="
rg -nA 15 'impl Traverse for PySlice' crates/vm/src/builtins/slice.rs

# Check the pyclass attribute for traverse behavior
echo -e "\n=== PySlice pyclass attributes ==="
rg -nB 2 'pub struct PySlice' crates/vm/src/builtins/slice.rs

# Check if there's a manual clear implementation
echo -e "\n=== Manual clear implementations in slice.rs ==="
rg -n 'fn clear' crates/vm/src/builtins/slice.rs

# Also check the full struct definition to see field types
echo -e "\n=== Full PySlice struct definition ==="
rg -nA 10 'pub struct PySlice' crates/vm/src/builtins/slice.rs

Repository: RustPython/RustPython

Length of output: 673


🏁 Script executed:

#!/bin/bash
# Look at other PyPayload implementations with both traverse and freelist
echo "=== PyPayload implementations with freelist ==="
rg -l 'impl PyPayload' crates/vm/src/builtins/ | head -5

# Check one of them to see if they have explicit clear impls
echo -e "\n=== Example: PyTuple with traverse and freelist ==="
rg -nB 5 'TUPLE_FREELIST' crates/vm/src/builtins/tuple.rs | head -30

# Look for Traverse trait definition to understand what it provides
echo -e "\n=== Traverse trait definition ==="
rg -nA 20 'pub trait Traverse' crates/vm/object/mod.rs

# Search for how traverse macro generates clear
echo -e "\n=== Search for clear generation in macro or docs ==="
rg -n 'fn clear' crates/vm/src/builtins/tuple.rs | head -10

Repository: RustPython/RustPython

Length of output: 556


🏁 Script executed:

#!/bin/bash
# Find where Traverse trait is defined
echo "=== Locate Traverse trait ==="
rg -n 'pub trait Traverse' crates/

# Look at PyTuple's clear implementation in detail
echo -e "\n=== PyTuple clear implementation ==="
rg -nB 3 -A 10 'fn clear\(&mut self' crates/vm/src/builtins/tuple.rs

# Check if PyTuple has freelist and traverse
echo -e "\n=== PyTuple pyclass and freelist setup ==="
rg -nB 2 'pub struct PyTuple' crates/vm/src/builtins/tuple.rs | head -20

# Check what the traverse attribute does in the macro
echo -e "\n=== Search for traverse attribute handling ==="
rg -n 'traverse' crates/vm/src/builtins/tuple.rs | head -5

Repository: RustPython/RustPython

Length of output: 1097


🏁 Script executed:

#!/bin/bash
# Search for Traverse trait more broadly
echo "=== Find Traverse trait definition ==="
fd -e rs -x rg -l 'trait Traverse' {} \;

# Look at the actual imports in slice.rs to understand what's being used
echo -e "\n=== Imports in slice.rs ==="
head -20 crates/vm/src/builtins/slice.rs

# Check if there's a Traverse impl somewhere for PySlice or if it's generated
echo -e "\n=== All Traverse impls in builtins ==="
rg -n 'impl.*Traverse' crates/vm/src/builtins/slice.rs crates/vm/src/builtins/tuple.rs

# Look at other traverse implementations to understand the pattern
echo -e "\n=== Other examples with 'traverse' attribute ==="
rg -B 2 'traverse\)' crates/vm/src/builtins/weakref.rs | head -20

Repository: RustPython/RustPython

Length of output: 1091


🏁 Script executed:

#!/bin/bash
# Look at Traverse trait definition
echo "=== Traverse trait definition ==="
cat crates/vm/src/object/traverse.rs | head -60

# Check if PySlice explicitly implements Traverse anywhere
echo -e "\n=== Explicit Traverse impl for PySlice ==="
rg -n 'impl.*Traverse.*PySlice' crates/vm/src/builtins/slice.rs

# Look at the macro to see what traverse attribute generates
echo -e "\n=== Search for pyclass macro handling of traverse ==="
fd -name '*.rs' -type f crates/pyclass/ | head -5

# Check if there's auto-generation documented
echo -e "\n=== Check if traverse generates Traverse impl automatically ==="
rg -n 'unsafe impl Traverse' crates/vm/src/builtins/ | head -10

Repository: RustPython/RustPython

Length of output: 3825


🏁 Script executed:

#!/bin/bash
# Look for pyclass macro documentation or implementation
echo "=== Search for pyclass macro procedural macro code ==="
find crates -name "*.rs" -path "*pyclass*" | grep -E "(proc|macro)" | head -10

# Check if there are examples where traverse attribute auto-generates clear
echo -e "\n=== Look for generated Traverse impl patterns in builtins ==="
rg -A 8 'unsafe impl Traverse' crates/vm/src/builtins/ | grep -E '(impl Traverse|fn clear)' | head -20

# Specifically check what traverse attribute value PySlice uses vs others
echo -e "\n=== Compare traverse attribute values ==="
rg '#\[pyclass.*traverse' crates/vm/src/builtins/ | head -10

# Check if there's a default/generated Traverse impl somewhere
echo -e "\n=== Look for MaybeTraverse impl on PySlice ==="
rg -A 15 'impl MaybeTraverse' crates/vm/src/builtins/slice.rs

Repository: RustPython/RustPython

Length of output: 2607


🏁 Script executed:

#!/bin/bash
# Check if there's auto-generated Traverse impl or if it relies on MaybeTraverse
echo "=== Check for any Traverse impl for PySlice or if it's missing ==="
rg -n 'Traverse' crates/vm/src/builtins/slice.rs

# Look at how freelist_push is used in other types to see the pattern
echo -e "\n=== Example: Look at PyTuple freelist (even though commented) ==="
rg -B 5 -A 15 'freelist_push' crates/vm/src/builtins/tuple.rs | head -30

# Check how freelist_pop/push interact with Traverse in other payload types
echo -e "\n=== Look at list.rs freelist with Traverse ==="
rg -B 3 -A 15 'freelist_push' crates/vm/src/builtins/list.rs | head -35

# Check if there's any documentation about freelist and Traverse interaction
echo -e "\n=== Search for freelist documentation or comments ==="
rg -B 2 -A 2 'freelist' crates/vm/src/builtins/list.rs | head -20

Repository: RustPython/RustPython

Length of output: 1057


🏁 Script executed:

#!/bin/bash
# Check if there's any generated code or if traverse attribute is just declarative
echo "=== Search for how pyclass macro processes traverse attribute ==="
find crates -name "*.toml" | xargs grep -l "proc-macro\|proc_macro" | head -5

# Look at actual pyclass macro crate
echo -e "\n=== Find pyclass macro crate ==="
find . -type d -name "*pyclass*" | grep -v target

# Check if PySlice should have Traverse but doesn't
echo -e "\n=== Verify PySlice has NO Traverse impl anywhere ==="
rg 'unsafe impl Traverse for PySlice' crates/

# Check if there's a compile-time verification that Traverse is needed
echo -e "\n=== Look at PyPayload trait to understand freelist requirements ==="
rg -B 5 -A 25 'pub trait PyPayload' crates/vm/src/object/

# Check if there are any build/compile errors related to PySlice
echo -e "\n=== Check if PySlice Traverse impl exists in expanded macros ==="
grep -r 'impl.*Traverse.*PySlice' crates/ 2>/dev/null | head -10

Repository: RustPython/RustPython

Length of output: 2411


🏁 Script executed:

#!/bin/bash
# Check the pyclass macro implementation to understand traverse attribute handling
echo "=== Check derive crate for pyclass macro ==="
cat crates/derive/Cargo.toml | grep -A 5 "\[package\]"

# Look at the macro implementation
echo -e "\n=== Search for traverse handling in derive macro ==="
find crates/derive-impl -name "*.rs" -type f | head -10

# Check if there's code that generates Traverse impl
echo -e "\n=== Look for Traverse impl generation ==="
rg -l 'Traverse' crates/derive-impl/ | head -5

# Check whether types with just "traverse" attribute get auto-generated impl
echo -e "\n=== Look at simple types with traverse attribute ==="
rg -B 3 -A 20 '#\[pyclass.*traverse\]\]' crates/vm/src/builtins/iter.rs | head -40

# Check if iter types have Traverse impl
echo -e "\n=== Check if iter.rs types have explicit Traverse impl ==="
rg 'unsafe impl Traverse' crates/vm/src/builtins/iter.rs

Repository: RustPython/RustPython

Length of output: 969


🏁 Script executed:

#!/bin/bash
# Look at the pytraverse macro implementation
echo "=== Check pytraverse.rs which generates Traverse impls ==="
cat crates/derive-impl/src/pytraverse.rs

# Also check pyclass.rs to see how traverse attribute is handled
echo -e "\n=== Check pyclass.rs for traverse attribute handling ==="
rg -n 'traverse' crates/derive-impl/src/pyclass.rs | head -20

Repository: RustPython/RustPython

Length of output: 5653


🏁 Script executed:

#!/bin/bash
# Now check what actually happens when freelist_push is called
# Let's look at where freelist_push is called in the allocation/deallocation flow
echo "=== Search for where freelist_push is called ==="
rg -n 'freelist_push' crates/vm/src/ | head -10

# Check if tp_dealloc or similar calls clear before freelist_push
echo -e "\n=== Look at object deallocation logic ==="
rg -B 10 -A 10 'freelist_push' crates/vm/src/object/payload.rs | head -40

# Verify if the auto-generated Traverse impl includes clear or just traverse
echo -e "\n=== Check if derived Traverse includes clear method ==="
rg -A 20 'impl_pytraverse' crates/derive-impl/src/pytraverse.rs | grep -E '(clear|fn)' | head -10

Repository: RustPython/RustPython

Length of output: 1807


🏁 Script executed:

#!/bin/bash
# The key is the safety comment - let's find where tp_clear is actually called
echo "=== Search for tp_clear implementation ==="
rg -n 'tp_clear' crates/vm/src/object/ | head -20

# Look for where tp_clear is invoked before freelist_push
echo -e "\n=== Search for drop_slow_inner which is mentioned in the safety comment ==="
rg -B 5 -A 15 'drop_slow_inner' crates/vm/src/object/core.rs | head -50

# Check if Traverse::clear is explicitly called anywhere
echo -e "\n=== Check if Traverse::clear is called in deallocation ==="
rg -n 'Traverse::clear\|\.clear\(' crates/vm/src/object/core.rs | head -20

Repository: RustPython/RustPython

Length of output: 3659


🏁 Script executed:

#!/bin/bash
# Let's see the full deallocation flow to understand when clear is called
echo "=== Look at the full default_dealloc function ==="
rg -A 50 'pub\(super\) unsafe fn default_dealloc' crates/vm/src/object/core.rs | head -80

# Also check the traverse_object.rs for tp_clear implementation
echo -e "\n=== Check traverse_object.rs ==="
cat crates/vm/src/object/traverse_object.rs | head -100

Repository: RustPython/RustPython

Length of output: 5659


🏁 Script executed:

#!/bin/bash
# Check what the pyclass macro generates for try_clear
echo "=== Look at pyclass.rs MaybeTraverse generation more carefully ==="
rg -B 5 -A 30 'fn try_clear' crates/derive-impl/src/pyclass.rs

# Also check what the actual generated MaybeTraverse should look like
echo -e "\n=== Check if there's a default try_clear that calls Traverse::clear ==="
rg -n 'try_clear' crates/vm/src/object/core.rs

Repository: RustPython/RustPython

Length of output: 2069


🏁 Script executed:

#!/bin/bash
# Find the try_clear_body generation in pyclass.rs
echo "=== Find try_clear_body generation ==="
rg -B 10 'try_clear_body' crates/derive-impl/src/pyclass.rs | head -40

# Check if there's a template for what try_clear_body should be
echo -e "\n=== Search for all try_clear mentions in pyclass.rs ==="
rg -n 'try_clear' crates/derive-impl/src/pyclass.rs

Repository: RustPython/RustPython

Length of output: 1230


🏁 Script executed:

#!/bin/bash
# Get the actual try_clear_body content
echo "=== Get try_clear_body content ==="
rg -A 3 'let try_clear_body = if has_clear' crates/derive-impl/src/pyclass.rs

Repository: RustPython/RustPython

Length of output: 247


Implement explicit Traverse::clear() for PySlice to extract child references before freelist caching.

PySlice contains PyObjectRef fields (start, stop, step) and has HAS_FREELIST = true. The #[pyclass(traverse)] attribute auto-generates only the traverse() method; the default clear() implementation does nothing. Before a PySlice is pushed to the freelist, tp_clear (called during deallocation) must extract these child references to avoid caching objects with stale references. Add an explicit unsafe impl Traverse for PySlice that implements a clear() method to extract all three fields into the output vector, similar to how PyTuple, PyList, and PyDict handle this.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/slice.rs` around lines 28 - 64, PySlice currently has
HAS_FREELIST = true but relies on the auto-generated traverse() only, so
tp_clear won't extract its PyObjectRef fields before freelist caching; add an
explicit unsafe impl Traverse for PySlice that implements clear(&mut self, out:
&mut Vec<PyObjectRef>) and pushes self.start, self.stop, and self.step (if
present) into out (mirroring PyTuple/PyList/PyDict patterns) so child references
are removed prior to freelist_push; ensure the impl coexists with the existing
traverse() and freelist_* methods (freelist_push, freelist_pop) so tp_clear
behavior is correct before objects are cached.

}

#[pyclass(with(Comparable, Representable, Hashable))]
Expand Down
3 changes: 3 additions & 0 deletions crates/vm/src/builtins/tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ unsafe impl Traverse for PyTuple {
}
}

// No freelist for PyTuple: structseq types (stat_result, struct_time, etc.)
// are static subtypes sharing the same Rust payload, making type-safe reuse
// impractical without a type-pointer comparison at push time.
impl PyPayload for PyTuple {
#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand Down
67 changes: 56 additions & 11 deletions crates/vm/src/object/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,19 @@ pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
// Untrack from GC BEFORE deallocation.
if obj_ref.is_gc_tracked() {
let ptr = unsafe { NonNull::new_unchecked(obj) };
rustpython_common::refcount::try_defer_drop(move || {
// untrack_object only removes the pointer address from a HashSet.
// It does NOT dereference the pointer, so it's safe even after deallocation.
unsafe {
crate::gc_state::gc_state().untrack_object(ptr);
}
});
if T::HAS_FREELIST {
// Freelist types must untrack immediately to avoid race conditions:
// a deferred untrack could remove a re-tracked entry after reuse.
unsafe { crate::gc_state::gc_state().untrack_object(ptr) };
} else {
rustpython_common::refcount::try_defer_drop(move || {
// untrack_object only removes the pointer address from a HashSet.
// It does NOT dereference the pointer, so it's safe even after deallocation.
unsafe {
crate::gc_state::gc_state().untrack_object(ptr);
}
});
}
}

// Extract child references before deallocation to break circular refs (tp_clear)
Expand All @@ -186,8 +192,17 @@ pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
unsafe { clear_fn(obj, &mut edges) };
}

// Deallocate the object memory
drop(unsafe { Box::from_raw(obj as *mut PyInner<T>) });
// Try to store in freelist for reuse; otherwise deallocate.
// Only exact types (not heaptype subclasses) go into the freelist,
// because the pop site assumes the cached typ matches the base type.
let pushed = if T::HAS_FREELIST && obj_ref.class().heaptype_ext.is_none() {
unsafe { T::freelist_push(obj) }
} else {
false
};
if !pushed {
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.
Expand Down Expand Up @@ -807,6 +822,15 @@ impl<T: PyPayload + core::fmt::Debug> PyInner<T> {
}
}

/// Drop a freelist-cached object, properly deallocating the `PyInner<T>`.
///
/// # Safety
/// `ptr` must point to a valid `PyInner<T>` allocation that was stored in a freelist.
#[allow(dead_code)]
pub(crate) unsafe fn drop_freelist_object<T: PyPayload>(ptr: *mut PyObject) {
drop(unsafe { Box::from_raw(ptr as *mut PyInner<T>) });
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Freelist cache lacks a drain path, so cached objects can leak on thread teardown.

Line 829 explicitly marks drop_freelist_object as dead code. With the current TLS storage (Vec<*mut PyObject>), dropping the vector only drops raw pointers, not the pointed PyInner<T> allocations. In thread-churn scenarios, this can leak up to each type’s MAX_FREELIST per thread.

Suggested helper to wire into TLS Drop implementations
-#[allow(dead_code)]
 pub(crate) unsafe fn drop_freelist_object<T: PyPayload>(ptr: *mut PyObject) {
     drop(unsafe { Box::from_raw(ptr as *mut PyInner<T>) });
 }
+
+pub(crate) unsafe fn drain_freelist<T: PyPayload>(list: &mut Vec<*mut PyObject>) {
+    for ptr in list.drain(..) {
+        unsafe { drop_freelist_object::<T>(ptr) };
+    }
+}

Then make each *_FREELIST TLS value own a wrapper with Drop that calls drain_freelist::<Type>().

As per coding guidelines: "Follow Rust best practices for error handling and memory management".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 825 - 832, The freelist currently
stores raw pointers in TLS (Vec<*mut PyObject>) so thread teardown only drops
pointers and leaks the underlying PyInner<T> allocations; implement a drain that
safely converts each raw pointer back into Box<PyInner<T>> and drops them (e.g.,
drain_freelist::<T>() which calls Box::from_raw for each pointer and lets it
drop), remove or repurpose the #[allow(dead_code)] on drop_freelist_object and
use it inside the drain, then replace each *_FREELIST TLS Vec with a small
owning wrapper type (e.g., FreelistTls<T>) that holds the Vec<*mut PyObject> and
implements Drop to call drain_freelist::<T>(); finally, ensure the TLS is
initialized with this wrapper so thread teardown runs the Drop and frees all
cached objects.


/// The `PyObjectRef` is one of the most used types. It is a reference to a
/// python object. A single python object can have multiple references, and
/// this reference counting is accounted for by this type. Use the `.clone()`
Expand Down Expand Up @@ -1720,8 +1744,29 @@ impl<T: PyPayload + crate::object::MaybeTraverse + core::fmt::Debug> PyRef<T> {
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));
let ptr = unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) };

// Try to reuse from freelist (exact type only, no dict, no heaptype)
let ptr = if !has_dict && !is_heaptype {
if let Some(cached) = unsafe { T::freelist_pop() } {
let inner = cached.as_ptr() as *mut PyInner<T>;
unsafe {
core::ptr::write(&mut (*inner).ref_count, RefCount::new());
(*inner).gc_bits.store(0, Ordering::Relaxed);
core::ptr::drop_in_place(&mut (*inner).payload);
core::ptr::write(&mut (*inner).payload, payload);
// typ, vtable, dict(None), weak_list, slots are preserved
}
// Drop the caller's typ since the cached object already holds one
drop(typ);
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
} else {
let inner = Box::into_raw(PyInner::new(payload, typ, dict));
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
}
} else {
let inner = Box::into_raw(PyInner::new(payload, typ, dict));
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
};

// Track object if:
// - HAS_TRAVERSE is true (Rust payload implements Traverse), OR
Expand Down
31 changes: 31 additions & 0 deletions crates/vm/src/object/payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::{
types::PyTypeFlags,
vm::{Context, VirtualMachine},
};
use core::ptr::NonNull;

cfg_if::cfg_if! {
if #[cfg(feature = "threading")] {
Expand Down Expand Up @@ -46,6 +47,36 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static {

fn class(ctx: &Context) -> &'static Py<PyType>;

/// Whether this type has a freelist. Types with freelists require
/// immediate (non-deferred) GC untracking during dealloc to prevent
/// race conditions when the object is reused.
const HAS_FREELIST: bool = false;

/// Maximum number of objects to keep in the freelist.
const MAX_FREELIST: usize = 0;

/// Try to push a dead object onto this type's freelist for reuse.
/// Returns true if the object was stored (caller must NOT free the memory).
///
/// # Safety
/// `obj` must be a valid pointer to a `PyInner<Self>` with refcount 0,
/// after `drop_slow_inner` and `tp_clear` have already run.
#[inline]
unsafe fn freelist_push(_obj: *mut PyObject) -> bool {
false
}

/// Try to pop a pre-allocated object from this type's freelist.
/// The returned pointer still has the old payload; the caller must
/// reinitialize `ref_count`, `gc_bits`, and `payload`.
///
/// # Safety
/// The returned pointer (if Some) points to uninitialized/stale payload.
#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
None
}

#[inline]
fn into_pyobject(self, vm: &VirtualMachine) -> PyObjectRef
where
Expand Down
Loading