Align specialization guards and caching with CPython#7341
Align specialization guards and caching with CPython#7341youknowone merged 31 commits intoRustPython:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors adaptive counters into out-of-line bit‑encoded AtomicU16 storage and updates CodeUnits layout/APIs; routes JIT compilation over cleaned bytecode bytes; adds dict hint/get_hint fast‑path helpers; adds PyBoundMethod accessors and JIT guards; tightens tuple index bounds handling and expands instrumentation trace detection. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
d1dc3d6 to
2d7c2f7
Compare
c03189d to
6d096fe
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pickle.py dependencies:
dependent tests: (97 tests)
Legend:
|
- Disable ForIterGen specialization (falls through to generic path) because inline generator frame resumption is needed for correct debugger StopIteration visibility (test_bdb) - Use downcast_ref_if_exact for PyNativeFunction in CALL specialization guards - Add can_specialize_call guard for class __init__ specialization - Remove expectedFailure for test_bad_newobj_args (now passing)
6d096fe to
d464119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/dict_inner.rs`:
- Around line 366-374: Snapshot the entry's key and value while holding the dict
read guard, drop the guard, then perform equality; specifically, in the block
that uses self.read() and inner.entries.get(hint) (and currently calls
key.key_is and key.key_eq on entry.key), clone/copy the entry.key and
entry.value out of the inner guard, release the guard, and then run
key.key_is(&cloned_key) and only if needed call key.key_eq(vm, &cloned_key) to
decide the result and return the cloned value; this avoids invoking key_eq while
holding the read lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1b01ab44-3238-428f-8cd8-393e5623cbb5
⛔ Files ignored due to path filters (1)
Lib/test/test_pickle.pyis excluded by!Lib/**
📒 Files selected for processing (8)
crates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/tuple.rscrates/vm/src/dict_inner.rscrates/vm/src/frame.rscrates/vm/src/stdlib/sys/monitoring.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d464119937
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| builtins: PyObjectRef, | ||
| closure: &[PyCellRef], | ||
| func_obj: Option<PyObjectRef>, |
There was a problem hiding this comment.
Restore Frame API compatibility for callers
This change drops the use_datastack parameter from Frame::new (and removes related frame-local APIs) without updating the existing call sites, so the crate no longer builds: cargo check -p rustpython-vm fails with unresolved methods/arity mismatches in builtins/function.rs, builtins/frame.rs, builtins/super.rs, and vm/mod.rs. As written, the commit is build-blocking and cannot be integrated until the callers are updated or compatibility shims are restored.
Useful? React with 👍 / 👎.
Introduce FastLocalsData enum with Heap and DataStack variants so non-generator/coroutine frames allocate localsplus on the VM datastack instead of the heap. Includes materialize_to_heap for migration when needed (e.g. generator suspension).
968372a to
0f044d6
Compare
Move key_eq call outside the read lock guard to avoid potential deadlock when Python __eq__ re-enters dict mutation paths. Matches the existing pattern in lookup().
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc3fd09611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -2805,7 +2828,7 @@ impl ExecutingFrame<'_> { | |||
| Ok(None) | |||
There was a problem hiding this comment.
Treat ResumeCheck as non-traceable at frame entry
By dispatching Instruction::ResumeCheck through the normal execution path here, functions whose first opcode is RESUME_CHECK now run, but the trace-event filters in run() still only skip Resume/InstrumentedResume (see the matches! checks around line and opcode tracing in crates/vm/src/frame.rs). That causes extra line/opcode callbacks at function entry for RESUME_CHECK code objects (for example, a code object with the first opcode patched from RESUME to RESUME_CHECK emits an extra event on the def line), which breaks tracing parity with the existing RESUME path.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
Bug Fixes
Refactor
Performance
Behavior