X Tutup
Skip to content

Reduce per-instruction overhead in interpreter loop #7315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Mar 3, 2026
Merged

Reduce per-instruction overhead in interpreter loop #7315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6788774
Reduce per-instruction overhead in interpreter loop
youknowone Mar 2, 2026
b078243
Unmark test_pdb_set_trace EXPECTED_FAILURE
youknowone Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Lib/test/test_doctest/test_doctest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2063,7 +2063,7 @@ def test_pdb_set_trace():
... 'continue', # stop debugging
... ''])

>>> try: runner.run(test) # TODO: RUSTPYTHON # doctest: +EXPECTED_FAILURE
>>> try: runner.run(test)
... finally: sys.stdin = real_stdin
> <doctest foo-bar@baz[2]>(1)<module>()
-> import pdb; pdb.set_trace()
Expand Down Expand Up @@ -2091,7 +2091,7 @@ def test_pdb_set_trace():
... 'continue', # stop debugging
... ''])

>>> try: # TODO: RUSTPYTHON # doctest: +EXPECTED_FAILURE
>>> try:
... runner.run(test)
... finally:
... sys.stdin = real_stdin
Expand Down Expand Up @@ -2209,7 +2209,7 @@ def test_pdb_set_trace_nested():
... 'continue', # stop debugging
... ''])

>>> try: # TODO: RUSTPYTHON # doctest: +EXPECTED_FAILURE
>>> try:
... runner.run(test)
... finally:
... sys.stdin = real_stdin
Expand Down
74 changes: 37 additions & 37 deletions crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,36 +750,43 @@ impl ExecutingFrame<'_> {
let bytecode::CodeUnit { op, arg } = instructions[idx];
let arg = arg_state.extend(arg);
let mut do_extend_arg = false;
let caches = op.cache_entries();

if !matches!(
op,
Instruction::Resume { .. }
| Instruction::ExtendedArg
| Instruction::InstrumentedLine
) && let Some((loc, _)) = self.code.locations.get(idx)
{
self.state.prev_line = loc.line.get() as u32;
}

// Fire 'opcode' trace event for sys.settrace when f_trace_opcodes
// is set. Skip RESUME and ExtendedArg (matching CPython's exclusion
// of these in _Py_call_instrumentation_instruction).
if vm.use_tracing.get()
&& !vm.is_none(&self.object.trace.lock())
&& *self.object.trace_opcodes.lock()
&& !matches!(
// Update prev_line only when tracing or monitoring is active.
// When neither is enabled, prev_line is stale but unused.
if vm.use_tracing.get() {
if !matches!(
op,
Instruction::Resume { .. }
| Instruction::InstrumentedResume
| Instruction::ExtendedArg
)
{
vm.trace_event(crate::protocol::TraceEvent::Opcode, None)?;
| Instruction::InstrumentedLine
) && let Some((loc, _)) = self.code.locations.get(idx)
Comment on lines +755 to +763
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Exclude InstrumentedResume when updating prev_line.

Line 758 excludes Resume but not InstrumentedResume. Under tracing + monitoring instrumentation, this can pre-set prev_line on resume and suppress the first real Line trace event.

🔧 Proposed fix 
-            // Update prev_line only when tracing or monitoring is active.
-            // When neither is enabled, prev_line is stale but unused.
+            // Update prev_line only when tracing is active.
+            // Keep RESUME variants excluded so the first real instruction
+            // can still trigger a line event.
             if vm.use_tracing.get() {
                 if !matches!(
                     op,
                     Instruction::Resume { .. }
+                        | Instruction::InstrumentedResume
                         | Instruction::ExtendedArg
                         | Instruction::InstrumentedLine
                 ) && let Some((loc, _)) = self.code.locations.get(idx)
                 {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Update prev_line only when tracing or monitoring is active.
// When neither is enabled, prev_line is stale but unused.
if vm.use_tracing.get() {
if !matches!(
op,
Instruction::Resume { .. }
| Instruction::InstrumentedResume
| Instruction::ExtendedArg
)
{
vm.trace_event(crate::protocol::TraceEvent::Opcode, None)?;
| Instruction::InstrumentedLine
) && let Some((loc, _)) = self.code.locations.get(idx)
// Update prev_line only when tracing is active.
// Keep RESUME variants excluded so the first real instruction
// can still trigger a line event.
if vm.use_tracing.get() {
if !matches!(
op,
Instruction::Resume { .. }
| Instruction::InstrumentedResume
| Instruction::ExtendedArg
| Instruction::InstrumentedLine
) && let Some((loc, _)) = self.code.locations.get(idx)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 755 - 763, The prev_line update logic in
frame.rs currently skips Instruction::Resume but not
Instruction::InstrumentedResume, which allows InstrumentedResume to pre-set
prev_line and suppress the first real Line trace; update the conditional inside
the vm.use_tracing.get() branch (the match against op) to also exclude
Instruction::InstrumentedResume alongside Resume, ExtendedArg, and
InstrumentedLine so prev_line is not updated for instrumented resume operations
(look for the match that checks Instruction::Resume { .. } |
Instruction::ExtendedArg | Instruction::InstrumentedLine and add
Instruction::InstrumentedResume { .. } to that set).

{
self.state.prev_line = loc.line.get() as u32;
}

// Fire 'opcode' trace event for sys.settrace when f_trace_opcodes
// is set. Skip RESUME and ExtendedArg (matching CPython's exclusion
// of these in _Py_call_instrumentation_instruction).
if !vm.is_none(&self.object.trace.lock())
&& *self.object.trace_opcodes.lock()
&& !matches!(
op,
Instruction::Resume { .. }
| Instruction::InstrumentedResume
| Instruction::ExtendedArg
)
{
vm.trace_event(crate::protocol::TraceEvent::Opcode, None)?;
}
}

let lasti_before = self.lasti();
let result = self.execute_instruction(op, arg, &mut do_extend_arg, vm);
self.skip_caches_if_fallthrough(op, lasti_before);
// Skip inline cache entries if instruction fell through (no jump).
if caches > 0 && self.lasti() == lasti_before {
self.update_lasti(|i| *i += caches as u32);
}
match result {
Ok(None) => {}
Ok(Some(value)) => {
Expand Down Expand Up @@ -3409,7 +3416,10 @@ impl ExecutingFrame<'_> {
let mut do_extend_arg = false;
self.execute_instruction(original_op, arg, &mut do_extend_arg, vm)
};
self.skip_caches_if_fallthrough(original_op, lasti_before_dispatch);
let orig_caches = original_op.to_base().unwrap_or(original_op).cache_entries();
if orig_caches > 0 && self.lasti() == lasti_before_dispatch {
self.update_lasti(|i| *i += orig_caches as u32);
}
result
}
Instruction::InstrumentedInstruction => {
Expand Down Expand Up @@ -3441,7 +3451,10 @@ impl ExecutingFrame<'_> {
let mut do_extend_arg = false;
self.execute_instruction(original_op, arg, &mut do_extend_arg, vm)
};
self.skip_caches_if_fallthrough(original_op, lasti_before_dispatch);
let orig_caches = original_op.to_base().unwrap_or(original_op).cache_entries();
if orig_caches > 0 && self.lasti() == lasti_before_dispatch {
self.update_lasti(|i| *i += orig_caches as u32);
}
result
}
_ => {
Expand Down Expand Up @@ -4106,19 +4119,6 @@ impl ExecutingFrame<'_> {
self.update_lasti(|i| *i = target);
}

/// Skip past CACHE code units after an instruction, but only if the
/// instruction did not modify lasti (i.e., it did not jump).
#[inline]
fn skip_caches_if_fallthrough(&mut self, op: Instruction, lasti_before: u32) {
if self.lasti() == lasti_before {
let base = op.to_base().unwrap_or(op);
let caches = base.cache_entries();
if caches > 0 {
self.update_lasti(|i| *i += caches as u32);
}
}
}

#[inline]
fn pop_jump_if_relative(
&mut self,
Expand Down
Loading
X Tutup