Add per-size tuple freelist (20 buckets × 2000 each)#7361
Merged
youknowone merged 4 commits intoRustPython:mainfrom Mar 9, 2026
Merged
Add per-size tuple freelist (20 buckets × 2000 each)#7361youknowone merged 4 commits intoRustPython:mainfrom
youknowone merged 4 commits intoRustPython:mainfrom
Conversation
Contributor
|
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 revises the freelist API: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant PyRef as PyRef::new_ref
participant Payload as T (payload)
participant Freelist as Thread-local Freelist
participant Alloc as Allocator
Caller->>PyRef: request new PyRef<T>
PyRef->>Payload: construct payload (type info)
PyRef->>Freelist: call T::freelist_pop(&payload)
alt freelist hit
Freelist-->>PyRef: return cached PyObject ptr
PyRef->>PyRef: initialize reused inner (drop caller typ if needed)
else freelist miss
PyRef->>Alloc: allocate PyInner<T> (pyinner_layout)
Alloc-->>PyRef: return new PyObject
end
PyRef-->>Caller: return initialized PyRef<T>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/tuple.rs`:
- Around line 113-118: The freelist_push implementation in unsafe fn
freelist_push currently reads the tuple size from py_tuple.elements.len(), but
default_dealloc calls tp_clear (which calls PyTuple::clear) before push so
elements are already cleared and len becomes 0; change the code so the tuple
length is captured before clearing (or modify the call site to pass the
pre-clear size into freelist_push), then use that captured size for the bucket
selection and the TUPLE_MAXSAVESIZE check; ensure you still skip caching when
the original size was 0 or >TUPLE_MAXSAVESIZE and keep the rest of freelist_push
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 65df5403-da34-438f-9eec-53a4d53aae14
📒 Files selected for processing (10)
crates/vm/src/builtins/complex.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/slice.rscrates/vm/src/builtins/tuple.rscrates/vm/src/object/core.rscrates/vm/src/object/payload.rs
youknowone
force-pushed
the
freelist-more-types
branch
from
ea854ba to
33f54e0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/builtins/tuple.rs (1)
110-130:⚠️ Potential issue | 🔴 CriticalPer-size tuple freelist is effectively bypassed at push time.
At Line 112, bucket selection uses
py_tuple.elements.len(), butdefault_dealloc(incore.rs:191-194) callstp_clearbeforefreelist_push. ThePyTuple::clearimplementation (lines 52-55) doescore::mem::take(&mut self.elements), which empties the elements. Whenfreelist_pushruns,len == 0, so lines 113-114 returnfalseand tuples are never cached in size buckets.To fix this, the tuple length must be captured before
tp_clearruns. One approach is to store the original size in thePyTuplepayload (e.g., anoriginal_len: usizefield set at construction) or pass the pre-clear size intofreelist_pushvia a different mechanism.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/tuple.rs` around lines 110 - 130, freelist_push currently reads the tuple length from py_tuple.elements, but PyTuple::clear (called by default_dealloc) empties elements before freelist_push runs so len is always 0; fix by capturing the tuple size before clearing and passing it to freelist_push (or storing it in the PyTuple payload set at construction) so bucket selection uses the pre-clear size. Update freelist_push signature (or add original_len field) and all call sites (notably default_dealloc / tp_clear path) to supply the original length, and keep the existing bounds check against TupleFreeList::MAXSAVESIZE and TUPLE_FREELIST bucket logic.
🤖 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/tuple.rs`:
- Around line 110-130: freelist_push currently reads the tuple length from
py_tuple.elements, but PyTuple::clear (called by default_dealloc) empties
elements before freelist_push runs so len is always 0; fix by capturing the
tuple size before clearing and passing it to freelist_push (or storing it in the
PyTuple payload set at construction) so bucket selection uses the pre-clear
size. Update freelist_push signature (or add original_len field) and all call
sites (notably default_dealloc / tp_clear path) to supply the original length,
and keep the existing bounds check against TupleFreeList::MAXSAVESIZE and
TUPLE_FREELIST bucket logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1463f3a8-1b7c-461f-9c0c-57ad8b07ed56
📒 Files selected for processing (10)
crates/vm/src/builtins/complex.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/slice.rscrates/vm/src/builtins/tuple.rscrates/vm/src/object/core.rscrates/vm/src/object/payload.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/range.rs
- crates/vm/src/builtins/dict.rs
youknowone
force-pushed
the
freelist-more-types
branch
3 times, most recently
from
6be1e32 to
8a56dd5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/builtins/tuple.rs (1)
62-63: StoreNonNull<PyObject>in the buckets.That makes the non-null invariant explicit and removes the unchecked conversion on Line 148.
♻️ Suggested cleanup
struct TupleFreeList { - buckets: [Vec<*mut PyObject>; Self::MAXSAVESIZE], + buckets: [Vec<NonNull<PyObject>>; Self::MAXSAVESIZE], } ... let bucket = &mut list.buckets[len - 1]; let stored = if bucket.len() < Self::MAX_FREELIST { - bucket.push(obj); + bucket.push(NonNull::new_unchecked(obj)); true } else { false }; ... - let result = list.buckets[len - 1] - .pop() - .map(|p| unsafe { NonNull::new_unchecked(p) }); + let result = list.buckets[len - 1].pop();for ptr in bucket.drain(..) { unsafe { - alloc::alloc::dealloc(ptr as *mut u8, layout); + alloc::alloc::dealloc(ptr.as_ptr() as *mut u8, layout); } }Also applies to: 125-127, 146-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/tuple.rs` around lines 62 - 63, Change the buckets field from Vec<*mut PyObject> to Vec<NonNull<PyObject>> (keeping the array type [Vec<...>; Self::MAXSAVESIZE]) and update all places that push or construct entries (e.g., where raw pointers are inserted) to create NonNull values via NonNull::new(ptr).expect("non-null pointer") or otherwise handle the Option instead of storing raw *mut; likewise replace the unchecked conversion at its usage site with NonNull::as_ptr() (or .get()) to obtain a *mut when needed and adjust any initializations/clears to work with NonNull<PyObject> instead of *mut.
🤖 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 2091-2119: Currently code calls T::freelist_pop(&payload) whenever
!has_dict && !is_heaptype, but that lets shared-Rust-payload subtypes (e.g.
structseq) drain base-type freelists and then get deallocated on type mismatch;
fix by only probing the freelist when the requested PyType exactly matches the
freelist bucket or by passing the requested typ into the freelist pop so it can
decline non-exact matches. Concretely, update the condition around the cached
lookup to require an exact-type match (use the existing typ variable) or change
T::freelist_pop signature to T::freelist_pop(&payload, &typ) and have it return
None if the bucket’s stored typ != typ; then remove the mismatch deallocation
path (or guard it) so freelist entries for other PyTypes are never popped.
---
Nitpick comments:
In `@crates/vm/src/builtins/tuple.rs`:
- Around line 62-63: Change the buckets field from Vec<*mut PyObject> to
Vec<NonNull<PyObject>> (keeping the array type [Vec<...>; Self::MAXSAVESIZE])
and update all places that push or construct entries (e.g., where raw pointers
are inserted) to create NonNull values via NonNull::new(ptr).expect("non-null
pointer") or otherwise handle the Option instead of storing raw *mut; likewise
replace the unchecked conversion at its usage site with NonNull::as_ptr() (or
.get()) to obtain a *mut when needed and adjust any initializations/clears to
work with NonNull<PyObject> instead of *mut.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0a5e6be4-676c-4497-8b86-e2f0b1a131e7
📒 Files selected for processing (10)
crates/vm/src/builtins/complex.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/slice.rscrates/vm/src/builtins/tuple.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/builtins/float.rs
youknowone
force-pushed
the
freelist-more-types
branch
2 times, most recently
from
417dde4 to
ad7d629
Compare
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin freelist-more-types |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/asyncio dependencies:
dependent tests: (7 tests)
[x] test: cpython/Lib/test/test_fork1.py dependencies: dependent tests: (no tests depend on fork1) [x] lib: cpython/Lib/os.py dependencies:
dependent tests: (169 tests)
[x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (148 tests)
Legend:
|
youknowone
force-pushed
the
freelist-more-types
branch
4 times, most recently
from
b7513c1 to
bbc1d7f
Compare
Implement PyTuple freelist matching tuples[PyTuple_MAXSAVESIZE]: - TupleFreeList with 20 per-size buckets (sizes 1..=20, 2000 capacity each) - freelist_push uses pre-clear size hint for correct bucket selection - freelist_pop takes &Self payload to select bucket by size - Type guard in new_ref handles structseq types sharing PyTuple vtable - Add pyinner_layout<T>() helper for custom freelist Drop impls - Update freelist_pop/push signatures across all freelist types
Move exact-type filtering from freelist_pop implementations to the single call-site in new_ref. This prevents structseq and other subtypes from popping tuple freelist entries entirely, rather than popping and then deallocating on type mismatch. Add Context::try_genesis() that returns None during bootstrap to avoid deadlock when genesis() is called during Context initialization.
Remove typ parameter from freelist_push trait signature. The exact type check is now done once at the call-site alongside the heaptype check, simplifying all freelist_push implementations.
By calling freelist_push before tp_clear, the payload is still intact and can be read directly (e.g. tuple element count for bucket selection). This eliminates freelist_hint and the hint parameter entirely.
youknowone
force-pushed
the
freelist-more-types
branch
from
bbc1d7f to
c32f31b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TupleFreeListwith 20 per-size buckets matching CPython'stuples[PyTuple_MAXSAVESIZE](sizes 1..=20, 2000 capacity each)freelist_pushreads element count from the object and stores inbuckets[len-1]freelist_poptakes&Selfpayload reference to select the correct size bucketpyinner_layout<T>()helper for custom freelistDropimplsfreelist_poptrait signature to accept&Selfacross all freelist typesCPython alignment
PyTuple_MAXSAVESIZETUPLE_MAXSAVESIZE = 20Py_tuple_MAXFREELISTTUPLE_MAXFREELIST = 2000tuples[20]arraybuckets: [Vec; 20]Py_IS_TYPE(op, &PyTuple_Type)HAS_FREELIST=falsefor structseq) +heaptype_ext.is_none()Py_SIZE(op) - 1elements.len() - 1len == 0checkTest plan
cargo build --releasecargo clippy --releasetest_tuple— passtest_structseq— pass (structseq types correctly excluded from freelist)test_os,test_time,test_stat— pass (structseq-heavy tests)test_sys— pass🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Refactor