-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add freelists for PyComplex, PyInt, PyRange #7345
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
Changes from 1 commit
d3cc53a
3d4951d
7b25894
b962909
7b614ae
01b5319
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- PyComplex(100), PyInt(100), PyBoundMethod(20), PyRange(6) - Use try_with instead of with in all freelist push/pop to prevent panic on thread-local access during thread teardown
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ use crate::{ | |
| Callable, Comparable, Constructor, GetAttr, GetDescriptor, PyComparisonOp, Representable, | ||
| }, | ||
| }; | ||
| use core::cell::Cell; | ||
| use core::ptr::NonNull; | ||
| use core::sync::atomic::{AtomicU32, Ordering::Relaxed}; | ||
| use itertools::Itertools; | ||
| #[cfg(feature = "jit")] | ||
|
|
@@ -1205,11 +1207,49 @@ impl PyBoundMethod { | |
| } | ||
| } | ||
|
|
||
| // spell-checker:ignore MAXFREELIST | ||
| thread_local! { | ||
| static BOUND_METHOD_FREELIST: Cell<crate::object::FreeList<PyBoundMethod>> = const { Cell::new(crate::object::FreeList::new()) }; | ||
| } | ||
|
|
||
| impl PyPayload for PyBoundMethod { | ||
| const MAX_FREELIST: usize = 20; | ||
| const HAS_FREELIST: bool = true; | ||
|
|
||
| #[inline] | ||
| fn class(ctx: &Context) -> &'static Py<PyType> { | ||
| ctx.types.bound_method_type | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn freelist_push(obj: *mut PyObject) -> bool { | ||
| BOUND_METHOD_FREELIST | ||
| .try_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 | ||
| }) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn freelist_pop() -> Option<NonNull<PyObject>> { | ||
| BOUND_METHOD_FREELIST | ||
| .try_with(|fl| { | ||
| let mut list = fl.take(); | ||
| let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) }); | ||
| fl.set(list); | ||
| result | ||
| }) | ||
| .ok() | ||
| .flatten() | ||
| } | ||
| } | ||
|
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PyBoundMethod freelist + traverse declarations =="
rg -n "name = \"method\", traverse|unsafe impl Traverse for PyBoundMethod|BOUND_METHOD_FREELIST" \
crates/vm/src/builtins/function.rs -C4
echo
echo "== Traverse clear implementations for object refs =="
rg -n "impl .*Traverse for PyObjectRef|impl .*Traverse for Option<PyObjectRef>|fn clear\\(" \
crates/vm/src -g '*.rs' -C4
echo
echo "== Dealloc/freelist flow in object core =="
rg -n "tp_clear|freelist_push|drop\\(edges\\)|freelist_pop|drop_in_place" \
crates/vm/src/object/core.rs -C5Repository: RustPython/RustPython Length of output: 16552 Implement explicit Traverse::clear for PyBoundMethod to detach refs before freelist storage. PyBoundMethod uses auto The fix is straightforward: implement manual Traverse with an explicit clear() that detaches both fields, matching the pattern already established for PyFunction and other ref-bearing types (PyTuple, PyList, PyDict, etc.). Proposed fix-#[pyclass(module = false, name = "method", traverse)]
+#[pyclass(module = false, name = "method", traverse = "manual")]
#[derive(Debug)]
pub struct PyBoundMethod {
object: PyObjectRef,
function: PyObjectRef,
}
+
+unsafe impl Traverse for PyBoundMethod {
+ fn traverse(&self, traverse_fn: &mut TraverseFn<'_>) {
+ self.object.traverse(traverse_fn);
+ self.function.traverse(traverse_fn);
+ }
+
+ fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
+ out.push(core::mem::replace(
+ &mut self.object,
+ Context::genesis().none.to_owned().into(),
+ ));
+ out.push(core::mem::replace(
+ &mut self.function,
+ Context::genesis().none.to_owned().into(),
+ ));
+ }
+}🤖 Prompt for AI Agents |
||
|
|
||
| impl Representable for PyBoundMethod { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,9 @@ use crate::{ | |
| Representable, SelfIter, | ||
| }, | ||
| }; | ||
| use core::cell::Cell; | ||
| use core::cmp::max; | ||
| use core::ptr::NonNull; | ||
| use crossbeam_utils::atomic::AtomicCell; | ||
| use malachite_bigint::{BigInt, Sign}; | ||
| use num_integer::Integer; | ||
|
|
@@ -67,11 +69,49 @@ pub struct PyRange { | |
| pub step: PyIntRef, | ||
| } | ||
|
|
||
| // spell-checker:ignore MAXFREELIST | ||
| thread_local! { | ||
| static RANGE_FREELIST: Cell<crate::object::FreeList<PyRange>> = const { Cell::new(crate::object::FreeList::new()) }; | ||
| } | ||
|
|
||
| impl PyPayload for PyRange { | ||
| const MAX_FREELIST: usize = 6; | ||
| const HAS_FREELIST: bool = true; | ||
|
|
||
| #[inline] | ||
| fn class(ctx: &Context) -> &'static Py<PyType> { | ||
| ctx.types.range_type | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn freelist_push(obj: *mut PyObject) -> bool { | ||
| RANGE_FREELIST | ||
| .try_with(|fl| { | ||
| let mut list = fl.take(); | ||
| let stored = if list.len() < Self::MAX_FREELIST { | ||
| list.push(obj); | ||
|
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.
Useful? React with 👍 / 👎. |
||
| true | ||
| } else { | ||
| false | ||
| }; | ||
| fl.set(list); | ||
| stored | ||
| }) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn freelist_pop() -> Option<NonNull<PyObject>> { | ||
| RANGE_FREELIST | ||
| .try_with(|fl| { | ||
| let mut list = fl.take(); | ||
| let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) }); | ||
| fl.set(list); | ||
| result | ||
| }) | ||
| .ok() | ||
| .flatten() | ||
| } | ||
| } | ||
|
|
||
| impl PyRange { | ||
|
|
||
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.
Pushing
PyBoundMethodobjects into the freelist here keeps theirobjectandfunctionPyObjectRefs alive until that freelist slot is reused (or thread teardown), becausePyBoundMethoddoes not provide a payload clear step that releases those references beforelist.push(obj). In practice, creating and dropping bound methods can now delay destruction/finalizers of arbitrary user objects and retain resources unexpectedly when the freelist is not churned.Useful? React with 👍 / 👎.