Optimize list.remove() lock usage - #8451
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change updates Python script invocation to use ChangesPython 3 invocation updates
List removal handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/vm/src/builtins/list.rs`:
- Around line 375-377: Update the removal logic around the mutable list borrow
so the PyObjectRef returned by elements.remove(index) is stored and kept alive
until after the borrow guard is released, then explicitly drop it. Add a
regression test covering an item whose __del__ accesses the list during
deallocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a8da830-9d9e-4a7e-88b5-2f6bf20599d2
📒 Files selected for processing (3)
.pre-commit-config.yamlcrates/vm/src/builtins/list.rsscripts/check_redundant_patches.py
8278e0b to
fb2941a
Compare
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
TYSM!
this LGTM overall:)
- can you please check our AI policy (I've added it to your PR)
- is there a reason why you've changed
python -> python3in the ci and the script?
|
FTR: this PR was done by a proprietary tool of mine which does leverage AI to make decisions, in this case DeepSeek V4 Flash was used and for the python -> python3, MacOS removed python2.7 support a while back and it ended up breaking the precommit for me since it couldn't find it . |
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Thank you for contributing!
Please also add AI disclosure in your commit message, not in PR comment.
| // defer delete out of borrow | ||
| let is_inside_range = index < self.borrow_vec().len(); | ||
| Ok(is_inside_range.then(|| self.borrow_vec_mut().remove(index))) | ||
| let removed = { |
There was a problem hiding this comment.
I am not sure how creating removed helps here. what does it hold and why do we need removed variable?
There was a problem hiding this comment.
I believe the main change was to call .borrow_vec_mut() once instead of twice
There was a problem hiding this comment.
yes, in the summary: Reduce list.remove() from two lock acquisitions to one
Assisted-by: WarpForgeAgent:DeepSeek-v4-flash
fb2941a to
d917aca
Compare
| // defer delete out of borrow | ||
| let is_inside_range = index < self.borrow_vec().len(); | ||
| Ok(is_inside_range.then(|| self.borrow_vec_mut().remove(index))) | ||
| let removed = { |
3 participants
Summary
list.remove()from two lock acquisitions to one.python3.Performance
PyList::removenow checks bounds and removes the item under a single mutable lock, avoiding an extra read-lock acquisition.Testing
prek run --all-filespasses.rustpython-capitests currently fail with an unrelated SIGSEGV in PyO3's CPython-specificPyList_SET_ITEM.Summary by CodeRabbit
Summary by CodeRabbit