Avoid per-tail-call owner allocations - #8472
Conversation
📝 WalkthroughWalkthroughThe PR adds a RustPython-only tail-call benchmark and changes VM tail-call lifetime management from a reference vector to one pending callee owner transferred through suspended trampoline frames. ChangesTail-call ownership and benchmark coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Does this PR follow our AI Policy? If not, we may close your PR. |
ac3be2e to
f197de0
Compare
Assisted-by: Codex:gpt-5.6-sol
Assisted-by: Codex:gpt-5.6-sol
f197de0 to
b97e119
Compare
|
@fanninpm The AI policy also includes:
having |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/vm/mod.rs (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Cellinstead ofUnsafeCellforpending_tailcall_owner.
Cell<Option<PyObjectRef>>supports.set()and.take()directly, becauseOption<T>implementsDefaultregardless of whetherTdoes. This gives the same single-threaded interior-mutability guarantee as the currentUnsafeCell, without an unsafe raw-pointer dereference inset_pending_tailcall_ownerandtake_pending_tailcall_owner.The comment on line 116 justifies the choice by stating the VM is per-thread and the field is accessed only on the owning thread. That reasoning applies equally to
Cell, so it does not explain whyUnsafeCellis required here.♻️ Proposed refactor to remove the unsafe raw-pointer access
- pending_tailcall_owner: core::cell::UnsafeCell<Option<PyObjectRef>>, + pending_tailcall_owner: Cell<Option<PyObjectRef>>,pub(crate) fn set_pending_tailcall_owner(&self, owner: PyObjectRef) { - let slot = unsafe { &mut *self.pending_tailcall_owner.get() }; - debug_assert!(slot.is_none(), "pending TailCall owner was not consumed"); - *slot = Some(owner); + let previous = self.pending_tailcall_owner.replace(Some(owner)); + debug_assert!(previous.is_none(), "pending TailCall owner was not consumed"); } fn take_pending_tailcall_owner(&self) -> PyObjectRef { - unsafe { &mut *self.pending_tailcall_owner.get() } - .take() - .expect("TailCall without pending owner") + self.pending_tailcall_owner + .take() + .expect("TailCall without pending owner") }Also applies to: 1409-1424
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/vm/mod.rs` around lines 113 - 117, Replace the UnsafeCell-based pending_tailcall_owner field with Cell<Option<PyObjectRef>> and update its initialization and accessors, especially set_pending_tailcall_owner and take_pending_tailcall_owner, to use Cell::set and Cell::take directly. Remove the unsafe raw-pointer dereferences while preserving the existing ownership and single-threaded behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/vm/src/vm/mod.rs`:
- Around line 113-117: Replace the UnsafeCell-based pending_tailcall_owner field
with Cell<Option<PyObjectRef>> and update its initialization and accessors,
especially set_pending_tailcall_owner and take_pending_tailcall_owner, to use
Cell::set and Cell::take directly. Remove the unsafe raw-pointer dereferences
while preserving the existing ownership and single-threaded behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ddf1bc6f-fe93-4dc1-87df-aeecedaa5c21
📒 Files selected for processing (4)
benches/tailcall_baseline.pycrates/vm/src/frame.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/thread.rs
Summary
Vec<PyObjectRef>with a singleOption<PyObjectRef>Motivation
The specialized exact-call handlers retained callable ownership by allocating a new vector for every tail call. The trampoline only needs one owner object: the exact function itself. Keeping that owner in a single slot removes the per-call vector allocation while preserving ownership across return and unwind paths.
For bound methods, the function owns the executable code and
selfis already retained in fast locals, so the temporary bound-method wrapper can be released immediately.Performance
Release-mode benchmark medians from 14 interleaved before/after samples:
The inline control changed by +0.99%.
Summary by CodeRabbit
Performance
Benchmarking