Conversation
📝 WalkthroughWalkthroughAdds per-type, thread-local freelists and integrates them into allocation/deallocation: Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Alloc as PyRef::new_ref
participant Freelist as Thread-local Freelist
participant Heap as Heap Allocation
participant Dealloc as default_dealloc
rect rgba(100,150,200,0.5)
Note over App,Alloc: Allocation flow
App->>Alloc: request new PyRef<T>
Alloc->>Freelist: call T::freelist_pop() (if exact type)
alt cached object available
Freelist-->>Alloc: NonNull<PyObject>
Alloc->>Alloc: reset fields, assign payload, return reused object
else no cached object
Alloc->>Heap: allocate new PyInner<T> (Box)
end
Alloc-->>App: initialized PyRef<T>
end
rect rgba(200,100,100,0.5)
Note over App,Dealloc: Deallocation flow
App->>Dealloc: drop PyObject
Dealloc->>Freelist: call T::freelist_push(obj) if T::HAS_FREELIST and exact type
alt freelist accepted
Freelist->>Freelist: store pointer in thread-local Vec
Freelist-->>Dealloc: return true (cached)
else rejected or full
Dealloc->>Heap: deallocate via Box::from_raw / normal drop
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
b46a203 to
2b6fc93
Compare
Add freelist_push/freelist_pop hooks to PyPayload trait with default no-op implementations. Modify default_dealloc to offer dead objects to the freelist before freeing memory. Modify PyRef::new_ref to try popping from the freelist before allocating. Implement thread-local freelist for PyFloat (max 100 entries). Float objects are ideal candidates: fixed-size payload (f64), no GC tracking, no instance dict, trivial Drop. Benchmark improvement on float-heavy workloads: float_arith: ~12% faster float_list: ~17% faster
Add HAS_FREELIST const to PyPayload trait. For freelist types, GC untracking is done immediately (not deferred) during dealloc to prevent race conditions when the object is reused. Restructure new_ref so freelist-popped objects also get GC tracked, enabling freelist support for GC-tracked types like PyList. Add drop_in_place for old payload before writing new one, required for types with non-trivial Drop (e.g. PyList's RwLock<Vec>). Implement thread-local freelist for PyList (max 80 entries).
Add thread-local freelist for PyDict (max 80 entries). Add heaptype check in default_dealloc to prevent subclass instances from entering the freelist. Subclass objects have a different Python type than the base type, so reusing them would return objects with the wrong __class__. Skip PyTuple freelist: structseq types (stat_result, struct_time) are static subtypes sharing the same Rust payload, making type-safe reuse impractical with heaptype check alone.
2b6fc93 to
a7c6c7e
Compare
a7c6c7e to
e4d4d11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 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.
In `@crates/vm/src/object/payload.rs`:
- Around line 69-76: The freelist contract must match how callers use returned
objects: update the freelist_pop contract (and implementation of freelist reuse)
so it guarantees the returned PyObject payload is valid/initialized (or at least
in a state safe to call drop_in_place), or alternatively change PyRef::new_ref
to avoid unconditionally calling drop_in_place on payloads returned from
freelist_pop; reference the symbols freelist_pop and PyRef::new_ref and ensure
either freelist_pop documents/ensures initialized payloads or PyRef::new_ref
detects freelist-origin objects and skips/drop-safely handles uninitialized
payloads to eliminate the UB.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: abf7359e-93a0-453e-94e1-67512b71c568
📒 Files selected for processing (8)
.cspell.dict/cpython.txtcrates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/slice.rscrates/vm/src/builtins/tuple.rscrates/vm/src/object/core.rscrates/vm/src/object/payload.rs
crates/vm/src/object/core.rs
Outdated
| /// 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>) }); | ||
| } |
There was a problem hiding this comment.
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.
Replace Cell<Vec<*mut PyObject>> with Cell<FreeList<T>> which implements Drop to properly deallocate PyInner<T> when threads exit. Also fix freelist_pop safety doc to match actual contract.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)
1784-1806: Consider clarifying the comment about preserved fields.The comment on line 1794 states "typ, vtable, dict(None), weak_list, slots are preserved" which could be misleading. The
weak_listwas cleared indrop_slow_innerbefore the object was pushed to the freelist, so it's preserved only as an empty structure. The comment could be clearer about this:- // typ, vtable, dict(None), weak_list, slots are preserved + // typ, vtable, dict(None), weak_list (empty), slots are preserved from cached object🤖 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 1784 - 1806, The comment saying "typ, vtable, dict(None), weak_list, slots are preserved" is misleading because drop_slow_inner clears weak_list before pushing to the freelist, so weak_list is preserved only as an empty structure; update the comment near the freelist reuse branch (around T::freelist_pop, PyInner, and Box::into_raw usage) to explicitly state which fields are fully preserved (typ, vtable, slots), which are set to None/cleared (dict is None), and that weak_list is preserved but intentionally emptied by drop_slow_inner.
🤖 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/slice.rs`:
- Around line 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.
---
Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 1784-1806: The comment saying "typ, vtable, dict(None), weak_list,
slots are preserved" is misleading because drop_slow_inner clears weak_list
before pushing to the freelist, so weak_list is preserved only as an empty
structure; update the comment near the freelist reuse branch (around
T::freelist_pop, PyInner, and Box::into_raw usage) to explicitly state which
fields are fully preserved (typ, vtable, slots), which are set to None/cleared
(dict is None), and that weak_list is preserved but intentionally emptied by
drop_slow_inner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5c9293ca-5ac8-4725-958e-c680f6fdc997
📒 Files selected for processing (6)
crates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/slice.rscrates/vm/src/object/core.rscrates/vm/src/object/payload.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/object/payload.rs
| thread_local! { | ||
| static SLICE_FREELIST: Cell<crate::object::FreeList<PySlice>> = const { Cell::new(crate::object::FreeList::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 | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 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.rsRepository: 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 -10Repository: 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 -5Repository: 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 -20Repository: 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 -10Repository: 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.rsRepository: 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 -20Repository: 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 -10Repository: 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.rsRepository: 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 -20Repository: 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 -10Repository: 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 -20Repository: 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 -100Repository: 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.rsRepository: 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.rsRepository: 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.rsRepository: 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.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/builtins/slice.rs (1)
36-45:⚠️ Potential issue | 🔴 Critical
PySlice::clear()leavesstopalive on freelist-hit.Line 40’s assumption is incorrect for the freelist path: if
freelist_pushsucceeds, payload drop is deferred, sostopstays as a strong reference inside cached objects. That can retain arbitrary object graphs and breaks expectedtp_clearbehavior for freelisted objects.Temporary safe mitigation until full `stop` extraction is implemented
impl PyPayload for PySlice { - const MAX_FREELIST: usize = 1; - const HAS_FREELIST: bool = true; + const MAX_FREELIST: usize = 0; + const HAS_FREELIST: bool = false;🤖 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 36 - 45, The clear method for PySlice leaves self.stop alive on freelist-hit; change PySlice::clear (the function handling start/stop/step) to extract stop as well (use self.stop.take()) and push it into out when Some, just like start and step, so the strong reference is removed immediately even if freelist_push defers payload drop; ensure the updated logic handles start, stop, and step symmetrically to avoid retaining object graphs on freelist reuse.
🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)
1785-1807: Collapse duplicated allocation fallback inPyRef::new_ref.The “allocate new
PyInner” block is duplicated in both branches. Extracting cached-value selection first makes the control flow tighter and easier to maintain.Refactor sketch
- 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, 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 { - 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>>()) } - }; + 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); + } + 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>>()) } + };As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate 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 1785 - 1807, The allocation branch is duplicated; refactor PyRef::new_ref by first selecting an inner pointer candidate (call T::freelist_pop() when !has_dict && !is_heaptype to get an Option<NonNull<PyInner<T>>>), then in a single common block handle either reinitializing the cached inner (reset ref_count, gc_bits, drop and write payload, clear dict/weak handling, and drop the incoming typ) when Some(cached) or allocating with Box::into_raw(PyInner::new(payload, typ, dict)) when None, and finally return NonNull::new_unchecked(inner.cast::<Py<T>>()). Keep the exact operations used now (core::ptr::write, drop_in_place, gc_bits.store, drop(typ)) and the same conditions (has_dict/is_heaptype) but remove the duplicated Box::into_raw code paths so the common logic is executed once.
🤖 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/builtins/slice.rs`:
- Around line 36-45: The clear method for PySlice leaves self.stop alive on
freelist-hit; change PySlice::clear (the function handling start/stop/step) to
extract stop as well (use self.stop.take()) and push it into out when Some, just
like start and step, so the strong reference is removed immediately even if
freelist_push defers payload drop; ensure the updated logic handles start, stop,
and step symmetrically to avoid retaining object graphs on freelist reuse.
---
Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 1785-1807: The allocation branch is duplicated; refactor
PyRef::new_ref by first selecting an inner pointer candidate (call
T::freelist_pop() when !has_dict && !is_heaptype to get an
Option<NonNull<PyInner<T>>>), then in a single common block handle either
reinitializing the cached inner (reset ref_count, gc_bits, drop and write
payload, clear dict/weak handling, and drop the incoming typ) when Some(cached)
or allocating with Box::into_raw(PyInner::new(payload, typ, dict)) when None,
and finally return NonNull::new_unchecked(inner.cast::<Py<T>>()). Keep the exact
operations used now (core::ptr::write, drop_in_place, gc_bits.store, drop(typ))
and the same conditions (has_dict/is_heaptype) but remove the duplicated
Box::into_raw code paths so the common logic is executed once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 580bf53d-710b-4561-a4c3-4774712af832
📒 Files selected for processing (2)
crates/vm/src/builtins/slice.rscrates/vm/src/object/core.rs
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/object/core.rs`:
- Around line 1792-1804: The cached-object branch currently drops the incoming
typ and leaves the cached object's typ unchanged, causing stale class identity;
instead, in the cached branch (inside the unsafe block handling inner: *mut
PyInner<T>) replace the stored typ with the incoming typ: drop the old typ
in-place (core::ptr::drop_in_place(&mut (*inner).typ)) and core::ptr::write(&mut
(*inner).typ, typ) (or equivalently move typ into (*inner).typ) before leaving
the unsafe block, then do not call drop(typ) afterwards; this ensures
PyInner<T>::typ is updated when reusing freelist objects while preserving the
existing payload/vtable handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2e7d6088-49dd-4928-9830-e5714a64b5c2
📒 Files selected for processing (1)
crates/vm/src/object/core.rs
| 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>>()) } |
There was a problem hiding this comment.
Reused freelist objects keep the old typ, which can return objects with stale class identity.
In the cached-object branch, the incoming typ is dropped and the cached typ is preserved. Because freelists are keyed by payload type T (not by typ instance), reuse across context lifetimes on the same thread can bind a new object to an old typ.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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>>()) } | |
| 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); | |
| // vtable, slots are preserved; dict is None, weak_list was | |
| // cleared by drop_slow_inner before freelist push | |
| } | |
| unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) } |
🤖 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 1792 - 1804, The cached-object
branch currently drops the incoming typ and leaves the cached object's typ
unchanged, causing stale class identity; instead, in the cached branch (inside
the unsafe block handling inner: *mut PyInner<T>) replace the stored typ with
the incoming typ: drop the old typ in-place (core::ptr::drop_in_place(&mut
(*inner).typ)) and core::ptr::write(&mut (*inner).typ, typ) (or equivalently
move typ into (*inner).typ) before leaving the unsafe block, then do not call
drop(typ) afterwards; this ensures PyInner<T>::typ is updated when reusing
freelist objects while preserving the existing payload/vtable handling.
Summary by CodeRabbit