GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Avoid per-tail-call owner allocations by moreal · Pull Request #8472 · RustPython/RustPython Conversation
|
| Case | Before | After | Change |
|---|---|---|---|
| Exact function | 189.210 ns | 165.974 ns | -12.28% |
| Exact bound method | 177.925 ns | 173.704 ns | -2.37% |
| Shallow steady trampoline | 320.042 ns | 288.682 ns | -9.80% |
| Deep steady trampoline | 17,756.400 ns | 15,564.300 ns | -12.35% |
| Shallow fresh activation | 318.714 ns | 289.905 ns | -9.04% |
| Deep fresh activation | 17,813.900 ns | 15,471.650 ns | -13.15% |
The inline control changed by +0.99%.
Performance
Benchmarking
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
📝 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Does this PR follow our AI Policy? If not, we may close your PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
ac3be2e to
f197de0
Compare
@fanninpm I'm sorry to miss leaving |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Assisted-by: Codex:gpt-5.6-sol
Assisted-by: Codex:gpt-5.6-sol
f197de0 to
b97e119
Compare
There was an error while loading. Please reload this page.
|
@fanninpm The AI policy also includes:
having |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ddf1bc6f-fe93-4dc1-87df-aeecedaa5c21
benches/tailcall_baseline.pycrates/vm/src/frame.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/thread.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This pull request is replaced by fd27902#diff-fe30ecf7c6b462e46e0874a55a05ea0edba83cefad5ac7a86e7137fa8f86ba74L863 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.