Add freelists for PyComplex, PyInt, PyRange#7345
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces per-type freelist mechanisms for PyComplex, PyInt, and PyRange objects to enable memory reuse, while improving error handling in existing freelists for dict, float, list, and slice by replacing panicking .with() calls with fallible .try_with() patterns. FreeList infrastructure is enhanced with safer memory deallocation and ergonomic access methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d48faafdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/vm/src/builtins/function.rs
Outdated
| .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.
Drop bound-method payload refs before freelist caching
Pushing PyBoundMethod objects into the freelist here keeps their object and function PyObjectRefs alive until that freelist slot is reused (or thread teardown), because PyBoundMethod does not provide a payload clear step that releases those references before list.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 👍 / 👎.
| .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.
Clear range endpoints before caching range objects
PyRange now enters a freelist, but each cached object still owns start/stop/step PyIntRefs because there is no clear path before list.push(obj). This means dropped ranges can retain large bigint endpoint allocations until the freelist slot is reused (or thread exit), creating avoidable memory retention spikes in workloads that create large ranges.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/function.rs`:
- Around line 1179-1222: PyBoundMethod currently uses the auto-derived Traverse
but lacks a manual Traverse::clear implementation, so freelisted PyBoundMethod
objects retain strong refs to their object and function fields and block GC;
implement a manual impl Traverse for PyBoundMethod that provides a clear(&self)
(or clear(&mut self) following the crate pattern) which detaches/releases the
strong references (set object and function fields to
PyObjectRef::none()/ptr::null or equivalent detach method used by
PyFunction/PyTuple), then ensure freelist_push stores only cleared objects so
freelisted PyBoundMethod will not keep references alive (update the impl for
PyPayload::clear/Traverse for PyBoundMethod to match the existing pattern used
by PyFunction, PyTuple, PyList, PyDict).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2ec1868c-eea8-4060-bf02-28ec5400c8d0
📒 Files selected for processing (8)
crates/vm/src/builtins/complex.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/slice.rs
crates/vm/src/builtins/function.rs
Outdated
| // 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() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 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 traverse (line 1066) but lacks a manual clear() implementation. The default clear() does nothing, so the object and function strong references remain embedded in freelisted bound methods. This blocks garbage collection of referenced objects.
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
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/function.rs` around lines 1179 - 1222, PyBoundMethod
currently uses the auto-derived Traverse but lacks a manual Traverse::clear
implementation, so freelisted PyBoundMethod objects retain strong refs to their
object and function fields and block GC; implement a manual impl Traverse for
PyBoundMethod that provides a clear(&self) (or clear(&mut self) following the
crate pattern) which detaches/releases the strong references (set object and
function fields to PyObjectRef::none()/ptr::null or equivalent detach method
used by PyFunction/PyTuple), then ensure freelist_push stores only cleared
objects so freelisted PyBoundMethod will not keep references alive (update the
impl for PyPayload::clear/Traverse for PyBoundMethod to match the existing
pattern used by PyFunction, PyTuple, PyList, PyDict).
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin freelist-more-types |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/object/core.rs (1)
825-830:⚠️ Potential issue | 🟡 MinorUpdate the
FreeListdoc comment to match the new drop behavior.The comment says
Dropconverts pointers back toBox<PyInner<T>>for proper deallocation, but the implementation now does rawdeallocwithout running destructors. This is now factually incorrect.Proposed doc fix
-/// Wraps a `Vec<*mut PyObject>` and implements `Drop` to convert each -/// raw pointer back into `Box<PyInner<T>>` for proper deallocation. +/// Wraps a `Vec<*mut PyObject>` and implements `Drop` to free raw +/// `PyInner<T>` allocations during thread teardown. Payload destructors are +/// intentionally skipped to avoid TLS-after-destruction access.As per coding guidelines, "Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code".
🤖 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 - 830, Update the FreeList doc comment to reflect the current Drop behavior: replace the statement that Drop "converts each raw pointer back into Box<PyInner<T>> for proper deallocation" with a precise description that Drop performs raw deallocation of the stored pointers without running their Rust destructors (i.e., it calls dealloc on the raw allocation), mention that this is unsafe and intentionally avoids destructor execution, and reference the FreeList<T> type and its Drop implementation so readers understand the semantic change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/object/core.rs`:
- Around line 852-861: The current teardown loop in self.items.drain(..)
directly deallocates the raw PyInner<T> buffer (Layout::new::<PyInner<T>>())
which skips payload destructors and allows unbounded byte/heap leaks for types
with large owned payloads; change it so teardown only raw-deallocates when it is
provably safe and otherwise drops or scrubs payloads: add a marker trait (e.g.,
SafeToTearDown) or a per-type flag and, inside the loop over self.items, for
items where T: SafeToTearDown use ptr::drop_in_place or ptr::read to run
destructor on PyInner<T> (dropping owned fields), and only fall back to
alloc::alloc::dealloc for types not safe to drop; alternatively enforce freelist
policy per type by limiting capacity for non-SafeToTearDown types before
teardown. Ensure references to self.items.drain(..), PyInner<T>, and
Layout::new::<PyInner<T>>() are updated accordingly.
---
Outside diff comments:
In `@crates/vm/src/object/core.rs`:
- Around line 825-830: Update the FreeList doc comment to reflect the current
Drop behavior: replace the statement that Drop "converts each raw pointer back
into Box<PyInner<T>> for proper deallocation" with a precise description that
Drop performs raw deallocation of the stored pointers without running their Rust
destructors (i.e., it calls dealloc on the raw allocation), mention that this is
unsafe and intentionally avoids destructor execution, and reference the
FreeList<T> type and its Drop implementation so readers understand the semantic
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9f925e76-bdd3-4670-94c9-bb20c2b3d0b8
📒 Files selected for processing (1)
crates/vm/src/object/core.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/object/core.rs (1)
852-861:⚠️ Potential issue | 🟠 MajorLeak is only bounded by object count, not by memory bytes.
Line 856-857 says leakage is bounded by
MAX_FREELIST, but this path skipsDropfor payloads, so leaked heap bytes per entry can still be large (e.g., big integer/container internals). Please add a teardown policy that either drops payloads for proven-safe types or avoids raw-dealloc teardown for types with unbounded payload memory.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 852 - 861, The teardown loop in self.items.drain(..) unsafely deallocates PyInner<T> bytes without running payload Drop, so the claimed bound by MAX_FREELIST is incorrect because payloads may own unbounded heap memory; change the teardown to detect types whose payloads may allocate (or implement Drop) and for those either call their destructor (run Drop) or use a safe per-type policy that moves items into a safe drop path (e.g., call a provided safe_free/drop_fn for the type) instead of raw dealloc; locate and modify the loop that deallocates with alloc::alloc::dealloc(ptr as *mut u8, Layout::new::<PyInner<T>>()) and the PyInner<T> handling to branch on a trait/flag (e.g., implements Drop/HasHeapPayload) or whitelist-only raw dealloc for truly fixed-size types to avoid leaking unbounded heap bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/object/core.rs`:
- Around line 852-861: The teardown loop in self.items.drain(..) unsafely
deallocates PyInner<T> bytes without running payload Drop, so the claimed bound
by MAX_FREELIST is incorrect because payloads may own unbounded heap memory;
change the teardown to detect types whose payloads may allocate (or implement
Drop) and for those either call their destructor (run Drop) or use a safe
per-type policy that moves items into a safe drop path (e.g., call a provided
safe_free/drop_fn for the type) instead of raw dealloc; locate and modify the
loop that deallocates with alloc::alloc::dealloc(ptr as *mut u8,
Layout::new::<PyInner<T>>()) and the PyInner<T> handling to branch on a
trait/flag (e.g., implements Drop/HasHeapPayload) or whitelist-only raw dealloc
for truly fixed-size types to avoid leaking unbounded heap bytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c7efdfae-3fc0-436a-88ce-d048138313fd
📒 Files selected for processing (1)
crates/vm/src/object/core.rs
Review responseApplied
Not applied (with reasoning)
— commented by Claude |
8617ce6 to
2499e1c
Compare
- 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
During thread teardown, Box::from_raw triggers cascading destructors that may access already-destroyed thread-local storage (GC state, other freelists). Use raw dealloc instead to free memory without running destructors.
- Set clear=false on PyBoundMethod (tp_clear=NULL in classobject.c) - Update FreeList doc comment to match actual Drop behavior (raw dealloc)
Non-Option PyObjectRef fields retain references in freelist, causing weakref and refcount assertions to fail in test_unittest, test_multiprocessing, and test_socket.
2499e1c to
01b5319
Compare
Summary
try_withinstead ofwithin all freelistpush/pop(including existing float/list/dict/slice) to gracefully handle thread-local access during thread teardownFollow-up to #7337.
Test plan
cargo build --releasecargo clippycleantest_complex,test_int,test_range,test_class,test_descrall pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Performance
Bug Fixes