Align specialization guards and caching with CPython - #7341
Conversation
Sorry, something went wrong.
|
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: f7913c33-3baf-4654-95d5-df8bc8685b4a 📥 CommitsReviewing files that changed from the base of the PR and between 968372a and cc3fd09. 📒 Files selected for processing (2)
📝 Walkthrough WalkthroughRefactors 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pickle.py dependencies:
dependent tests: (97 tests)
Legend:
|
Sorry, something went wrong.
- 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)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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
📥 CommitsReviewing files that changed from the base of the PR and between 68ad332 and d464119.
⛔ Files ignored due to path filters (1)- Lib/test/test_pickle.py is excluded by !Lib/**
- crates/compiler-core/src/bytecode.rs
- crates/jit/src/instructions.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/builtins/function.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/dict_inner.rs
- crates/vm/src/frame.rs
- crates/vm/src/stdlib/sys/monitoring.rs
Sorry, something went wrong.
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 GitHubYour 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".
Sorry, something went wrong.
| 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 👍 / 👎.
Sorry, something went wrong.
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).
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 GitHubYour 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".
Sorry, something went wrong.
| @@ -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 👍 / 👎.
Sorry, something went wrong.
* vm: complete specialized opcode dispatch paths * vm: cache LOAD_GLOBAL with dict entry hints * vm: align adaptive specialization counters with CPython backoff * vm: apply cooldown counter on specialization success paths * vm: retain LOAD_GLOBAL specializations on misses * vm: keep attr and call specializations on guard misses * vm: retain store-attr and store-subscr specializations on misses * vm: retain specialization opcodes on generic fallback paths * vm: align jump-backward specialization defaults with CPython * vm: retain exact-args call specializations on misses * vm: retain SEND_GEN specialization on non-coroutine sends * vm: specialize list.append calls like CPython CALL_LIST_APPEND * vm: set cooldown on LOAD_ATTR_CLASS specialization * vm: specialize bound method object CALL paths * vm: specialize CALL_KW for bound method objects * vm: use current-state function version for CALL_KW specialization * vm: align CALL/CALL_KW pyfunction specialization with CPython * vm: drop call-site identity caches in generic CALL specializations * vm: align builtin type call specializations with CPython guards * vm: align builtin CALL guards with CPython self_or_null semantics * vm: require exact list in CALL_LIST_APPEND fast path * vm: align CALL builtin/class specialization flow with CPython * vm: tighten len/isinstance CALL specializations to builtin guards * vm: gate CALL_BUILTIN_CLASS on type vectorcall like CPython * vm: run non-py CALL specializations via direct vectorcall * vm: align class-call specialization branching with CPython * Fix CI: disable ForIterGen, tighten CALL guards - 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)
Summary by CodeRabbit
Bug Fixes
Refactor
Performance
Behavior