Skip to content

Reduce number of string clones when compiling python source code#8103

Merged
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:nits-6
Jun 15, 2026
Merged

Reduce number of string clones when compiling python source code#8103
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:nits-6

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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)
  • src/lib.rs
✅ Files skipped from review due to trivial changes (1)
  • src/lib.rs

📝 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

  • youknowone

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.

@ShaharNaveh ShaharNaveh force-pushed the nits-6 branch 3 times, most recently from d39b8df to e949487 Compare June 14, 2026 12:21
@ShaharNaveh ShaharNaveh marked this pull request as ready for review June 14, 2026 12:43
@github-actions

Copy link
Copy Markdown
Contributor

📦 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

@youknowone youknowone merged commit c0e3066 into RustPython:main Jun 15, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants