-
Notifications
You must be signed in to change notification settings - Fork 1.4k
freelist optimization #7337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
freelist optimization #7337
Changes from 4 commits
4c030e9
ad49638
c706c9e
e4d4d11
c0b33a6
5b78ad5
2266d94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,7 @@ finalizers | |
| firsttraceable | ||
| flowgraph | ||
| formatfloat | ||
| freelist | ||
| freevar | ||
| freevars | ||
| fromlist | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
@@ -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. | ||
|
|
@@ -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>) }); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Freelist cache lacks a drain path, so cached objects can leak on thread teardown. Line 829 explicitly marks 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 As per coding guidelines: "Follow Rust best practices for error handling and memory management". 🤖 Prompt for AI Agents |
||
|
|
||
| /// 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()` | ||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 673
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 556
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1097
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1091
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 3825
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 2607
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1057
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 2411
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 969
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 5653
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1807
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 3659
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 5659
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 2069
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1230
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 247
Implement explicit
Traverse::clear()forPySliceto extract child references before freelist caching.PySlicecontainsPyObjectReffields (start,stop,step) and hasHAS_FREELIST = true. The#[pyclass(traverse)]attribute auto-generates only thetraverse()method; the defaultclear()implementation does nothing. Before aPySliceis pushed to the freelist,tp_clear(called during deallocation) must extract these child references to avoid caching objects with stale references. Add an explicitunsafe impl Traverse for PySlicethat implements aclear()method to extract all three fields into the output vector, similar to howPyTuple,PyList, andPyDicthandle this.🤖 Prompt for AI Agents