-
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 all 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,52 @@ impl<T: PyPayload + core::fmt::Debug> PyInner<T> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Thread-local freelist storage that properly deallocates cached objects | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// on thread teardown. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Wraps a `Vec<*mut PyObject>` and implements `Drop` to convert each | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// raw pointer back into `Box<PyInner<T>>` for proper deallocation. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub(crate) struct FreeList<T: PyPayload> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| items: Vec<*mut PyObject>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _marker: core::marker::PhantomData<T>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<T: PyPayload> FreeList<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub(crate) const fn new() -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| items: Vec::new(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _marker: core::marker::PhantomData, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<T: PyPayload> Default for FreeList<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn default() -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self::new() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<T: PyPayload> Drop for FreeList<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn drop(&mut self) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for ptr in self.items.drain(..) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| drop(unsafe { Box::from_raw(ptr as *mut PyInner<T>) }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<T: PyPayload> core::ops::Deref for FreeList<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type Target = Vec<*mut PyObject>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn deref(&self) -> &Self::Target { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &self.items | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<T: PyPayload> core::ops::DerefMut for FreeList<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &mut self.items | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// 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 +1781,31 @@ 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 cached = if !has_dict && !is_heaptype { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsafe { T::freelist_pop() } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let ptr = if let Some(cached) = cached { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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, slots are preserved; dict is None, weak_list was | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // cleared by drop_slow_inner before freelist push | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Drop the caller's typ since the cached object already holds one | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| drop(typ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1792
to
+1804
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. Reused freelist objects keep the old In the cached-object branch, the incoming Suggested fix let ptr = if let Some(cached) = cached {
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).typ);
+ core::ptr::write(&mut (*inner).typ, PyAtomicRef::from(typ));
core::ptr::drop_in_place(&mut (*inner).payload);
core::ptr::write(&mut (*inner).payload, payload);
- // typ, vtable, slots are preserved; dict is None, weak_list was
+ // vtable, slots are preserved; dict is None, weak_list was
// cleared by drop_slow_inner before freelist push
}
- // Drop the caller's typ since the cached object already holds one
- drop(typ);
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
} else {As per coding guidelines: "Follow Rust best practices for error handling and memory management". 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } 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