GeezerVison is showing github.com — Reduce number of string clones when compiling python source code by ShaharNaveh · Pull Request #8103 · RustPython/RustPython
Conversation
ShaharNaveh
commented
Jun 14, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Summary by CodeRabbit
- Refactor
- Optimized how source names and paths are passed during compilation and execution to avoid unnecessary string allocations across the runtime, standard library integration, examples, and benchmarks.
- Updated relevant VM APIs and related call sites to accept borrowed
&str for source path/name parameters, plus a small cleanup of related string/Latin-1 cache initialization and test compilation helpers.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Jun 14, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a68f514a-aed0-494f-a450-37233ea846e6
📥 Commits
Reviewing files that changed from the base of the PR and between 0df603a and 215003a.
⛔ Files ignored due to path filters (1)
Lib/test/test_thread.py is excluded by !Lib/**
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 Walkthrough
Walkthrough
VirtualMachine::compile, compile_with_opts, run_string, run_code_string, and import_file change their source_path/file_path parameter from owned String to &str. PyGetSet::new and three Context getset helpers change name from impl Into<String> to &str. All call sites across the codebase are updated to remove .to_owned() and .clone(). context.rs also receives minor idiomatic Rust refactors unrelated to the signature changes.
Changes
source_path & name: String → &str refactor
| Layer / File(s) |
Summary |
Core compile/run API signature changes
crates/vm/src/vm/compile.rs, crates/vm/src/vm/python_run.rs, crates/vm/src/import.rs |
VirtualMachine::compile, compile_with_opts, run_string, run_code_string change source_path from String to &str; import_file changes file_path likewise; internal forwarding calls updated accordingly. |
PyGetSet::new and Context getset helper signature changes
crates/vm/src/builtins/getset.rs, crates/vm/src/vm/context.rs |
PyGetSet::new accepts name: &str with internal into() conversion; Context::new_readonly_getset, new_static_getset, and new_getset change from impl Into<String> to &str, removing the internal conversion. |
VM-internal call-site updates
crates/vm/src/eval.rs, crates/vm/src/stdlib/builtins.rs, crates/vm/src/stdlib/sys.rs, crates/vm/src/vm/interpreter.rs, crates/vm/src/vm/mod.rs |
All VM-internal call sites remove .to_owned() or .clone() when passing source_path or name to the updated APIs. |
External/entry-point call-site updates
crates/capi/src/ceval.rs, crates/wasm/src/vm_class.rs, crates/stdlib/src/_opcode.rs, src/shell.rs, src/lib.rs, examples/*, benches/* |
C API, WASM, stdlib tests, shell, main library, examples, and benchmarks remove .to_owned() / .clone() at their respective vm.compile / vm.run_string call sites. |
context.rs idiomatic cleanups
crates/vm/src/vm/context.rs |
latin1_char_cache uses u8::MIN..=u8::MAX; latin1_singleton_index and new_str condensed to expression form; interned_or_new_str switches to if let; new_exception_type uses unwrap_or_else. |
Cosmetic changes
crates/vm/src/vm/thread.rs |
Import reorder and two blank line insertions; no logic changes. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
- RustPython/RustPython#7740: Both PRs modify
src/lib.rs's run_file implementation, with the main change passing path by reference instead of to_owned().
- RustPython/RustPython#8050: Both PRs modify
crates/vm/src/builtins/getset.rs, adding #[must_use] to PyGetSet::new which this PR updates to accept name: &str.
Suggested reviewers
Poem
🐇 No more to_owned(), no needless heap fuss,
A &str slice passed without any muss!
The compiler now borrows what once was owned,
Across every call site, the pattern is honed.
Fewer allocations, the rabbit hops free—
Just borrow the string and let memory be! 🌿
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name |
Status |
Explanation |
Resolution |
| Docstring Coverage |
⚠️ Warning |
Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%. |
Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name |
Status |
Explanation |
| Description Check |
✅ Passed |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
✅ Passed |
The title clearly and specifically describes the main change: reducing string clones during Python source code compilation, which aligns with all modified files throughout the PR. |
| Linked Issues check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
❤️ Share
Comment @coderabbitai help to get the list of available commands and usage tips.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
📦 Library Dependencies
The following Lib/ modules were modified. Here are their dependencies:
[x] test: cpython/Lib/test/test_thread.py (TODO: 4)
[x] test: cpython/Lib/test/test_thread_local_bytecode.py
[x] test: cpython/Lib/test/test_threadsignals.py
dependencies:
dependent tests: (13 tests)
- [ ] concurrent.futures: test_asyncio test_compileall test_concurrent_futures test_context test_genericalias test_inspect test_struct test_wmi
- [x] asyncio: test_asyncio test_logging test_os test_pdb test_unittest
Legend:
[+] path exists in CPython[x] up-to-date, [ ] outdated
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|