Skip to content

Reduce Python→Python call overhead with LightFrame#8354

Draft
youknowone wants to merge 4 commits into
RustPython:mainfrom
youknowone:light-frame-call-overhead
Draft

Reduce Python→Python call overhead with LightFrame#8354
youknowone wants to merge 4 commits into
RustPython:mainfrom
youknowone:light-frame-call-overhead

Conversation

@youknowone

Copy link
Copy Markdown
Member

Summary

Reduces specialized Python→Python call overhead from ~92ns to ~55ns (~40% reduction) and fib(28) from ~152ms to ~110ms (~28% reduction).

Phase 1: LightFrame (commit 1)

For specialized exact-args call paths (CallPyExactArgs, CallBoundMethodExactArgs, LoadAttrMethodWithValues, etc.), allocate a lightweight LightFrame header on the DataStack instead of a full Frame PyObject. The frame is materialized lazily only when observed (sys._getframe, traceback, tracing).

What we skip:

  • Frame::new — no InterpreterFrame with Mutex fields, no trace state
  • into_ref — no PyObject shell allocation, no freelist, no refcount
  • with_frame — no TLS CURRENT_FRAME swap, no owner swap, no scopeguard, no dispatch_traced_frame
  • release_datastack_frame — no strong_count check, no Frame::clear

What we keep:

  • DataStack push/pop (bump allocator)
  • Localsplus zero-init + args copy
  • Recursion depth check
  • Closure cell copy

The interleaved heavy/light frame walk correctly handles cases where heavy frames are pushed above light frames (e.g. native code calling back into Python from within a light frame).

Phase 2: with_frame streamlining (commit 2)

Optimizes the remaining heavy path (generators, exec/eval, tracing-active calls):

  • Inline recursion check (skip with_recursion closure)
  • Amortize C stack overflow check to every 64th depth
  • Skip dispatch_traced_frame when use_tracing is false

Known limitations

  • test_getframemodulename: frame identity assertion fails because materialized light frames are not linked into the heavy chain, so f.f_back and sys._getframe(i) may return different PyObject identities for the same logical frame.

Benchmark results (Apple M-series, release build)

Metric Before After Improvement
Call overhead (f(x) loop) ~92 ns ~55 ns 40%
fib(28) ~152 ms ~110 ms 28%

Addresses youknowone#40

🤖 Generated with Claude Code

Allocate a lightweight LightFrame header on the DataStack instead of a
full Frame PyObject for specialized exact-args call paths. The frame is
materialized lazily only when observed (sys._getframe, traceback, tracing).

Key changes:
- LightFrame struct with borrowed pointers and no refcount overhead
- FrameSource enum (Heavy/Light) replacing the object field on ExecutingFrame
- CURRENT_LIGHT_FRAME TLS for the light frame chain
- Interleaved heavy/light frame walking in frame_at_offset_vm and
  current_thread_frame_vm
- invoke_light_slots on PyFunction for the fast call path

Call overhead reduced from ~92ns to ~58ns (~37% reduction).
fib(28) improved from ~152ms to ~111ms (~27% reduction).

Assisted-by: Claude
- Inline check_recursive_call instead of wrapping in with_recursion closure
- Amortize C stack overflow check to every 64th call depth
- Skip dispatch_traced_frame when use_tracing is false (hot path)
- Add recursion_depth_increment/decrement helpers

Assisted-by: Claude
Copilot AI review requested due to automatic review settings July 23, 2026 15:30
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: daf4a189-99b0-4b14-9d6a-c12b2275cbe3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a “LightFrame” fast-path to reduce Python→Python call overhead by avoiding allocation/materialization of full Frame PyObjects for specialized exact-args call paths, and streamlines the remaining heavy with_frame path to reduce tracing/recursion-check overhead. It also updates sys._getframe and VM frame-walk helpers to account for (and lazily materialize) light frames when the call stack is observed.

Changes:

  • Add a stack-allocated LightFrame header (with lazy materialization to Frame) plus unified heavy/light stack walking APIs for _getframe-style introspection.
  • Route specialized exact-args call sites through PyFunction::invoke_light_slots() to run bytecode using a light frame on the DataStack.
  • Optimize VirtualMachine::with_frame by inlining recursion checks, amortizing C-stack checks, and skipping traced-frame dispatch when tracing is off.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
crates/vm/src/vm/thread.rs Adds TLS pointer for the current light-frame chain (CURRENT_LIGHT_FRAME) plus get/set helpers.
crates/vm/src/vm/mod.rs Streamlines with_frame recursion/C-stack checks and avoids traced dispatch when tracing is disabled; switches current_frame() to unified heavy/light lookup.
crates/vm/src/stdlib/sys.rs Updates sys._getframe/related lookups to use VM-aware frame walking that can materialize light frames.
crates/vm/src/frame.rs Introduces LightFrame, lazy materialization, and unified heavy/light stack walking helpers; extends ExecutingFrame to abstract heavy vs light sources.
crates/vm/src/builtins/function.rs Adds invoke_light_slots fast path that allocates and runs a LightFrame on the DataStack for unobserved calls.
Comments suppressed due to low confidence (1)

crates/vm/src/vm/mod.rs:1759

  • After adding an early recursion_depth guard, this later decrement will double-decrement the depth on normal return/panic. Remove it (or replace with a comment) and let the earlier guard handle balancing.
            self.recursion_depth.update(|d| d - 1);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/vm/src/frame.rs Outdated
Comment on lines +211 to +223
// Set up the `previous` frame pointer for f_back:
// If the previous light frame exists, materialize it and point there.
// Otherwise, point to the top heavy frame.
let prev_light = (*light).previous_light;
let prev_frame_ptr = if !prev_light.is_null() {
let prev_materialized = materialize_light_frame(prev_light as *mut _, vm);
let ptr = &*prev_materialized as *const Py<Frame> as *mut Frame;
core::mem::forget(prev_materialized);
ptr
} else {
crate::vm::thread::get_current_frame() as *mut Frame
};
iframe.previous.store(prev_frame_ptr, atomic::Ordering::Relaxed);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== LightFrame and thread-frame symbol references =='
rg -n -C 4 \
  -e '\bLightFrame\b' \
  -e '\bprevious_light\b' \
  -e '\bsaved_current_frame\b' \
  -e '\bget_current_light_frame\s*\(' \
  -e '\bset_current_light_frame\s*\(' \
  -e '\bget_current_frame\s*\(' \
  -e '\bset_current_frame\s*\(' \
  crates

printf '%s\n' '== Candidate source files =='
fd -t f -e rs . crates | sort

Length of output: 54707


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== Light-frame creation, execution, and cleanup =='
sed -n '780,955p' crates/vm/src/builtins/function.rs

printf '%s\n' '== Heavy-frame push/pop scopes =='
sed -n '1700,1825p' crates/vm/src/vm/mod.rs

printf '%s\n' '== f_back implementation and materialization call sites =='
rg -n -C 5 \
  -e '\bfn f_back\b' \
  -e '\.f_back\s*\(' \
  -e '\bmaterialize_light_frame_pub\s*\(' \
  -e '\bmaterialize_light_frame\s*\(' \
  crates/vm/src -g '*.rs'

Length of output: 20624


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '700,775p' crates/vm/src/builtins/frame.rs

Length of output: 3354


@fanninpm, confirmed — the finding is valid.

A concrete failing stack is:

H0 → L1 → H1 → L2

where L1 is a light frame, H1 is a heavy Python frame entered through a native callback, and L2 is a subsequent light call.

  • L1.saved_current_frame == H0
  • L2.previous_light == L1
  • L2.saved_current_frame == H1

The current code materializes L2 with f_back = L1, skipping its immediate caller H1. Also, for a light frame without a previous_light, using get_current_frame() at materialization time can select a heavy frame that was pushed after that light frame began, rather than its caller.

f_back follows the stored previous pointer directly, so it cannot repair this later.

The materialization logic should use the entry-time boundary:

  • link to previous_light only when it has the same saved_current_frame;
  • otherwise link to this light frame’s saved_current_frame, rather than the current heavy-frame TLS value.

One caveat: this fixes the immediate incorrect predecessor. Preserving the entire H1 → L1 relationship for a full f_back walk across native callbacks would need additional chain handling, because heavy-frame push currently links H1.previous only to the prior heavy frame (H0).


🧬 Code Graph Analysis Results

Relevant snippets

crates/vm/src/exceptions.rs

  • Lines 27-34 (GC traversal for exceptions)
unsafe impl Traverse for PyBaseException {
    fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
        self.traceback.traverse(tracer_fn);
        self.cause.traverse(tracer_fn);
        self.context.traverse(tracer_fn);
        self.args.traverse(tracer_fn);
    }
}
  • Lines 420-423 (Exception constructor enum)
pub enum ExceptionCtor {
    Class(PyTypeRef),
    Instance(PyBaseExceptionRef),
}
  • Lines 425-443 (converting an object into ExceptionCtor)
impl TryFromObject for ExceptionCtor {
    fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
        obj.downcast::<PyType>()
            .and_then(|cls| {
                if cls.fast_issubclass(vm.ctx.exceptions.base_exception_type) {
                    Ok(Self::Class(cls))
                } else {
                    Err(cls.into())
                }
            })
            .or_else(|obj| obj.downcast::<PyBaseException>().map(Self::Instance))
            .map_err(|obj| {
                vm.new_type_error(format!(
                    "exceptions must be classes or instances deriving from BaseException, not {}",
                    obj.class().name()
                ))
            })
    }
}
  • Lines 445-480 (instantiating exception instances/values)
impl ExceptionCtor {
    pub fn instantiate(self, vm: &VirtualMachine) -> PyResult<PyBaseExceptionRef> {
        match self {
            Self::Class(cls) => vm.invoke_exception(&cls, vec![]),
            Self::Instance(exc) => Ok(exc),
        }
    }

    pub fn instantiate_value(
        self,
        value: PyObjectRef,
        vm: &VirtualMachine,
    ) -> PyResult<PyBaseExceptionRef> {
        let exc_inst = value.clone().downcast::<PyBaseException>().ok();
        match (self, exc_inst) {
            // both are instances; which would we choose?
            (Self::Instance(_exc_a), Some(_exc_b)) => {
                Err(vm.new_type_error("instance exception may not have a separate value"))
            }
            // if the "type" is an instance and the value isn't, use the "type"
            (Self::Instance(exc), None) => Ok(exc),
            // if the value is an instance of the type, use the instance value
            (Self::Class(cls), Some(exc)) if exc.fast_isinstance(&cls) => Ok(exc),
            // otherwise; construct an exception of the type using the value as args
            (Self::Class(cls), _) => {
                let args = match_class!(match value {
                    PyNone => vec![],
                    tup @ PyTuple => tup.to_vec(),
                    exc @ PyBaseException => exc.args().to_vec(),
                    obj => vec![obj],
                });
                vm.invoke_exception(&cls, args)
            }
        }
    }
}
  • Lines 1645-1667 (exception payload fields)
pub struct PyBaseException {
        pub(super) traceback: PyRwLock<Option<PyTracebackRef>>,
        pub(super) cause: PyRwLock<Option<PyRef<Self>>>,
        pub(super) context: PyRwLock<Option<PyRef<Self>>>,
        pub(super) suppress_context: AtomicCell<bool>,
        pub(super) args: PyRwLock<PyTupleRef>,
    }

impl PyBaseException {
    pub fn get_arg(&self, idx: usize) -> Option<PyObjectRef> {
        self.args.read().get(idx).cloned()
    }
}
  • Lines 3043-3106 (ExceptionGroup matching used by bytecode CHECK_EG_MATCH)
pub(crate) fn exception_group_match(
    exc_value: &PyObjectRef,
    match_type: &PyObjectRef,
    vm: &VirtualMachine,
) -> PyResult<(PyObjectRef, PyObjectRef)> {
    // Implements _PyEval_ExceptionGroupMatch

    // If exc_value is None, return (None, None)
    if vm.is_none(exc_value) {
        return Ok((vm.ctx.none(), vm.ctx.none()));
    }

    // Validate match_type and reject ExceptionGroup/BaseExceptionGroup
    check_except_star_type_valid(match_type, vm)?;

    // Check if exc_value matches match_type
    if exc_value.is_instance(match_type, vm)? {
        // Full match of exc itself
        let is_eg = exc_value.fast_isinstance(vm.ctx.exceptions.base_exception_group);
        let matched = if is_eg {
            exc_value.clone()
        } else {
            // Naked exception - wrap it in ExceptionGroup
            let excs = vm.ctx.new_tuple(vec![exc_value.clone()]);
            let eg_type: PyObjectRef = crate::exception_group::exception_group().to_owned().into();
            let wrapped = eg_type.call((vm.ctx.new_str(""), excs), vm)?;
            // Copy traceback from original exception
            if let Ok(exc) = exc_value.clone().downcast::<types::PyBaseException>()
                && let Some(tb) = exc.__traceback__()
                && let Ok(wrapped_exc) = wrapped.clone().downcast::<types::PyBaseException>()
            {
                let _ = wrapped_exc.set___traceback__(tb.into(), vm);
            }
            wrapped
        };
        return Ok((vm.ctx.none(), matched));
    }

    // Check for partial match if it's an exception group
    if exc_value.fast_isinstance(vm.ctx.exceptions.base_exception_group) {
        let pair = vm.call_method(exc_value, "split", (match_type.clone(),))?;
        if !pair.class().is(vm.ctx.types.tuple_type) {
            return Err(vm.new_type_error(format!(
                "{}.split must return a tuple, not {}",
                exc_value.class().name(),
                pair.class().name()
            )));
        }
        let pair_tuple: PyTupleRef = pair.try_into_value(vm)?;
        if pair_tuple.len() < 2 {
            return Err(vm.new_type_error(format!(
                "{}.split must return a 2-tuple, got tuple of size {}",
                exc_value.class().name(),
                pair_tuple.len()
            )));
        }
        let matched = pair_tuple[0].clone();
        let rest = pair_tuple[1].clone();
        return Ok((rest, matched));
    }

    // No match
    Ok((exc_value.clone(), vm.ctx.none()))
}
  • Lines 3110-3175 (prep_reraise_star used by PREP_RERAISE_STAR)
pub fn prep_reraise_star(orig: PyObjectRef, excs: PyObjectRef, vm: &VirtualMachine) -> PyResult {
    use crate::builtins::PyList;

    let excs_list = excs
        .downcast::<PyList>()
        .map_err(|_| vm.new_type_error("expected list for prep_reraise_star"))?;

    let excs_vec: Vec<PyObjectRef> = excs_list.borrow_vec().to_vec();

    // If no exceptions to process, return None
    if excs_vec.is_empty() {
        return Ok(vm.ctx.none());
    }

    // Special case: naked exception (not an ExceptionGroup)
    // Only one except* clause could have executed, so there's at most one exception to raise
    if !orig.fast_isinstance(vm.ctx.exceptions.base_exception_group) {
        // Find first non-None exception
        let first = excs_vec.into_iter().find(|e| !vm.is_none(e));
        return Ok(first.unwrap_or_else(|| vm.ctx.none()));
    }

    // Split excs into raised (new) and reraised (from original) by comparing metadata
    let mut raised: Vec<PyObjectRef> = Vec::new();
    let mut reraised: Vec<PyObjectRef> = Vec::new();

    for exc in excs_vec {
        if vm.is_none(&exc) {
            continue;
        }
        // Check if this exception came from the original group
        if is_exception_from_orig(&exc, &orig, vm) {
            reraised.push(exc);
        } else {
            raised.push(exc);
        }
    }

    // If no exceptions to reraise, return None
    if raised.is_empty() && reraised.is_empty() {
        return Ok(vm.ctx.none());
    }

    // Project reraised exceptions onto original structure to preserve nesting
    let reraised_eg = exception_group_projection(&orig, &reraised, vm)?;

    // If no new raised exceptions, just return the reraised projection
    if raised.is_empty() {
        return Ok(reraised_eg);
    }

    // Combine raised with reraised_eg
    if !vm.is_none(&reraised_eg) {
        raised.push(reraised_eg);
    }

    // If only one exception, return it directly
    if raised.len() == 1 {
        return Ok(raised.into_iter().next().unwrap());
    }

    // Create new ExceptionGroup for multiple exceptions
    let excs_tuple = vm.ctx.new_tuple(raised);
    let eg_type: PyObjectRef = crate::exception_group::exception_group().to_owned().into();
    eg_type.call((vm.ctx.new_str(""), excs_tuple), vm)
}

crates/vm/src/vm/mod.rs

  • Lines 780-939 (thread datastack allocation used by frame localsplus)
impl VirtualMachine {
    /// Bump-allocate `size` bytes from the thread data stack.
    ///
    /// # Safety
    /// The returned pointer must be freed by calling `datastack_pop` in LIFO order.
    #[inline(always)]
    pub(crate) fn datastack_push(&self, size: usize) -> *mut u8 {
        unsafe { (*self.datastack.get()).push(size) }
    }

    /// Check whether the thread data stack currently has room for `size` bytes.
    #[inline(always)]
    pub(crate) fn datastack_has_space(&self, size: usize) -> bool {
        unsafe { (*self.datastack.get()).has_space(size) }
    }

    /// Pop a previous data stack allocation.
    ///
    /// # Safety
    /// `base` must be a pointer returned by `datastack_push` on this VM,
    /// and all allocations made after it must already have been popped.
    #[inline(always)]
    pub(crate) unsafe fn datastack_pop(&self, base: *mut u8) {
        unsafe { (*self.datastack.get()).pop(base) }
    }

    /// Temporarily detach the current thread (ATTACHED → DETACHED) while
    /// running `f`, then re-attach afterwards.  Allows `stop_the_world` to
    /// park this thread during blocking syscalls.
    #[inline]
    pub fn allow_threads<R>(&self, f: impl FnOnce() -> R) -> R {
        thread::allow_threads(self, f)
    }
}

Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +925 to +930
if !materialized_ptr.is_null() {
// Frame was materialized (observed by traceback/sys._getframe).
// Drop the leaked ref from materialization.
let materialized_ref: FrameRef = (&*materialized_ptr).to_owned();
drop(materialized_ref);
}
Comment on lines +899 to +903
// Run the bytecode
let lasti_ref: &rustpython_common::atomic::PyAtomic<u32> =
&*(&(*light).lasti as *const core::sync::atomic::AtomicU32
as *const rustpython_common::atomic::PyAtomic<u32>);
let code_ref = (*(*light).code).to_owned();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai can this be confirmed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- PyAtomic definitions and cfg gates ---'
rg -n -C 8 'PyAtomic|type .*Atomic|struct .*Atomic' crates --glob '*.rs' | head -n 280

printf '%s\n' '--- LightFrame declaration and lasti uses ---'
rg -n -C 12 '\bstruct\s+LightFrame\b|\blasti\b' crates/vm/src --glob '*.rs' | head -n 360

printf '%s\n' '--- workspace threading feature declarations ---'
rg -n -C 4 '^\s*threading\s*=|feature.*threading|\bthreading\b' Cargo.toml crates --glob 'Cargo.toml' | head -n 240

Length of output: 46644


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- candidate atomic and frame files ---'
fd -t f -e rs 'atomic|frame' crates/common crates/vm | sort

printf '%s\n' '--- exact PyAtomic definition ---'
rg -n -C 24 'pub\s+(type|use)\s+PyAtomic\b|PyAtomic\s*[=<]' crates/common --glob '*.rs'

printf '%s\n' '--- exact LightFrame definition and runner signature ---'
rg -n -C 28 '\bstruct\s+LightFrame\b|fn\s+run_light_frame\b|impl\s+LightFrame\b' crates/vm/src/frame.rs

printf '%s\n' '--- package feature wiring relevant to wasm and common ---'
sed -n '1,80p' crates/wasm/Cargo.toml
sed -n '1,55p' crates/common/Cargo.toml
sed -n '1,45p' crates/vm/Cargo.toml

Length of output: 20133


@fanninpm, confirmed.

PyAtomic<u32> is feature-dependent in crates/common/src/atomic.rs:

  • with threading: AtomicU32
  • without threading: core::cell::Cell<u32>

But LightFrame::lasti is unconditionally declared as core::sync::atomic::AtomicU32 (in crates/vm/src/frame.rs, Line 72), then cast to &PyAtomic<u32> in the new call path. The wasm package explicitly builds the VM without the threading feature, so that cast becomes effectively:

&AtomicU32 -> &Cell<u32>

That is unsound. A matching size/alignment today would not make it valid: the types have no guaranteed layout equivalence, and Cell operations perform interior mutation through a reference that does not actually point to a Cell.

The appropriate fix is to make LightFrame::lasti a PyAtomic<u32>, initialize it with Radium::new(0), and pass &(*light).lasti directly—removing the cast entirely.

One wording refinement: the unsafe raw-pointer cast likely bypasses a normal compile-time type mismatch, so I can confirm the non-threading unsoundness rather than specifically a guaranteed build failure.

Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +863 to +867
// Recursion depth check
if vm.current_recursion_depth() >= vm.recursion_limit.get() {
// Clean up the light frame TLS before erroring
crate::vm::thread::set_current_light_frame(prev_light);
// Drop values we moved into localsplus

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai can this be confirmed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '=== Native-stack guard call sites and recursion helpers ==='
rg -n -C 5 '\bcheck_c_stack_overflow\b|\bwith_recursion\b|\bwith_frame\b' crates/vm --glob '*.rs'

printf '%s\n' '=== Light-frame runner definitions/call sites ==='
rg -n -C 5 '\brun_light_frame\s*\(' crates/vm --glob '*.rs'

printf '%s\n' '=== Light-frame invocation and recursion-depth operations ==='
rg -n -C 4 '\binvoke_light_slots\s*\(|\brecursion_depth_(increment|decrement)\s*\(' crates/vm --glob '*.rs'

Length of output: 34152


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '=== All tracked definitions/references related to light frames ==='
git grep -n -E 'run_light_frame|LightFrame|check_c_stack_overflow' -- '*.rs' || true

printf '%s\n' '=== Relevant VirtualMachine frame-helper implementation ==='
sed -n '1690,1775p' crates/vm/src/vm/mod.rs

printf '%s\n' '=== Complete light-frame recursion-check and execution region ==='
sed -n '850,930p' crates/vm/src/builtins/function.rs

printf '%s\n' '=== Source-file locations that may supply included/generated frame code ==='
rg -n -C 2 'include!|mod .*frame|pub.*frame' crates/vm/src --glob '*.rs'

Length of output: 32072


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '=== run_light_frame implementation ==='
sed -n '2325,2415p' crates/vm/src/frame.rs

printf '%s\n' '=== Calls made by run_light_frame (for recursion/frame-helper delegation) ==='
sed -n '2334,2415p' crates/vm/src/frame.rs | rg -n -C 2 '\b(with_frame|with_recursion|check_recursive_call|run_frame|run|execute)\b' || true

Length of output: 3377


@fanninpm, confirmed.

PyFunction::invoke_light_slots checks only recursion_depth >= recursion_limit before incrementing the depth. It then calls run_light_frame, which directly executes ExecutingFrame::run and does not delegate to with_frame, with_recursion, or any other C-stack guard.

In contrast, VirtualMachine::with_frame performs the amortized native-stack probe:

let depth = self.recursion_depth.get();
if depth & 63 == 0 && self.check_c_stack_overflow() {
    return Err(self.new_recursion_error(String::new()));
}

Therefore the light-frame fast path bypasses the native-stack protection. With an increased sys.setrecursionlimit(), sufficiently deep recursive light-frame calls may exhaust the native stack rather than reliably raising RecursionError.

The appropriate fix is to add the same amortized check to invoke_light_slots before recursion_depth_increment() (exposing a crate-visible VM helper if needed).

Comment thread crates/vm/src/vm/mod.rs
Comment on lines +1727 to +1728
self.recursion_depth.update(|d| d + 1);

- Fix materialize_light_frame previous pointer: use payload pointer
  (&**frame) instead of Py<Frame> pointer, matching the chain convention
- Fix predecessor ref management: use retained_back instead of
  mem::forget to keep materialized predecessors alive
- Fix materialized ref reclaim: use FrameRef::from_raw to drop the
  leaked keeper ref without double-incrementing
- Use PyAtomic<u32> for LightFrame.lasti instead of AtomicU32, removing
  the unsound cast on non-threading targets
- Add amortized C stack overflow check to light frame path
- Add scopeguard to light frame execution for panic safety
- Remove unnecessary Send/Sync impls from LightFrame
- Make check_c_stack_overflow pub(crate) for light frame path access

Assisted-by: Claude
Copilot AI review requested due to automatic review settings July 23, 2026 16:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/vm/src/vm/mod.rs:1732

  • self.recursion_depth.update(|d| d + 1) happens before any scopeguard::defer! is installed. On some cfgs, the intervening operations (notably push_thread_frame(...).lock().push(...) on non-unix threading builds) can panic (e.g. allocation failure), which would leak the incremented recursion depth and potentially break subsequent recursion checks. To keep invariants panic-safe, consider either (a) installing a recursion-depth-only defer immediately after increment, or (b) moving the increment down so it occurs after the fallible setup and immediately before the existing scopeguard::defer! block.
        self.recursion_depth.update(|d| d + 1);

        // Publish the frame for sys._current_frames() and faulthandler.
        #[cfg(all(not(unix), feature = "threading"))]
        crate::vm::thread::push_thread_frame(FramePtr(NonNull::from(&*frame)));
        // Link frame into the signal-safe frame chain.

Comment on lines +803 to +811
// Generator/coroutine code and tracing must use the heavy path
if code.flags.intersects(
bytecode::CodeFlags::GENERATOR
| bytecode::CodeFlags::COROUTINE
| bytecode::CodeFlags::ASYNC_GENERATOR,
) || vm.use_tracing.get()
{
return self.invoke_exact_args_slots(args, vm);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Relevant symbols ==='
rg -n -C 5 \
  -e 'struct FrameLocals\b' \
  -e 'impl FrameLocals\b' \
  -e 'fn lazy\b' \
  -e 'pub.*fn new\(' \
  -e '\binvoke_light_slots\s*\(' \
  crates/vm/src

printf '%s\n' '=== Frame module outline ==='
ast-grep outline crates/vm/src/frame.rs --items all --type class,function,method --match 'Frame|FrameLocals|LightFrame' || true

printf '%s\n' '=== Function module call contexts ==='
rg -n -C 12 '\binvoke_light_slots\s*\(' crates/vm/src/builtins/function.rs crates/vm/src

Length of output: 70868


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Frame locals and Frame::new ==='
sed -n '897,970p' crates/vm/src/frame.rs
sed -n '1220,1305p' crates/vm/src/frame.rs

printf '%s\n' '=== Exact-call specialization dispatch and guard setup ==='
sed -n '5340,5490p' crates/vm/src/frame.rs
rg -n -C 8 \
  -e 'CallPythonExactArgs' \
  -e 'CallBoundMethodExactArgs' \
  -e 'can_specialize_call\s*\(' \
  -e 'is_optimized_for_call_specialization\s*\(' \
  crates/vm/src/frame.rs

printf '%s\n' '=== Code flag mutation/validation surfaces ==='
rg -n -C 8 \
  -e 'CodeFlags' \
  -e 'co_flags' \
  -e 'replace\(' \
  -e 'PyCodeNew' \
  crates/vm/src/builtins/code.rs crates/vm/src

printf '%s\n' '=== Tests using code.replace or FunctionType ==='
rg -n -C 5 \
  -e '\.replace\s*\(' \
  -e 'FunctionType\s*\(' \
  -e 'types\.CodeType' \
  -e 'co_flags' \
  Lib test crates \
  -g '*.py' -g '*.rs' || true

Length of output: 192102


@fanninpm, confirmed — the finding is valid.

FrameLocals::lazy() is explicitly the CO_NEWLOCALS representation. In contrast, the normal Frame::new path uses the function’s globals as the locals mapping when CO_NEWLOCALS is absent:

None if code.flags.contains(CodeFlags::NEWLOCALS) => FrameLocals::lazy(),
None => FrameLocals::with_locals(ArgMapping::from_dict_exact(scope.globals.clone())),

invoke_light_slots always selects FrameLocals::lazy(), while its current fallback only covers generator/coroutine code and tracing. The call-specialization eligibility also requires CO_OPTIMIZED but not CO_NEWLOCALS.

This state is reachable because RustPython’s code.replace(co_flags=...) and code-object constructor accept supplied flags without enforcing CO_OPTIMIZED => CO_NEWLOCALS. Consequently, a function with CO_OPTIMIZED but without CO_NEWLOCALS can be specialized and then observe an empty lazy locals dictionary instead of its globals mapping.

The fix should require CO_NEWLOCALS for the light-frame path and fall back to invoke_exact_args_slots otherwise. Adding that condition to can_specialize_call avoids creating the specialization; keeping a defensive check in invoke_light_slots would also protect current and future callers.

Comment thread crates/vm/src/vm/mod.rs Outdated
Comment on lines +1732 to +1737
// Link frame into the signal-safe frame chain.
let old_frame = crate::vm::thread::set_current_frame((&**frame) as *const Frame);
frame.previous.store(
old_frame as *mut Frame,
core::sync::atomic::Ordering::Relaxed,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

fd -t f 'frame\.rs|mod\.rs|thread\.rs' crates/vm/src | sort

ast-grep outline crates/vm/src/builtins/frame.rs --items all --view expanded
ast-grep outline crates/vm/src/frame.rs --items all --view expanded
ast-grep outline crates/vm/src/vm/thread.rs --items all --view expanded

rg -n -C 5 -P '\b(frame_at_offset_vm|find_owned_chain_frame|retained_back|f_back|with_frame|resume_gen_frame|Light)\b' \
  crates/vm/src/builtins/frame.rs crates/vm/src/frame.rs crates/vm/src/vm crates/vm/src \
  || true

Length of output: 79142


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- crates/vm/src/frame.rs: materialization and unified walk ---'
sed -n '145,405p' crates/vm/src/frame.rs

echo '--- crates/vm/src/builtins/frame.rs: f_back ---'
sed -n '720,755p' crates/vm/src/builtins/frame.rs

echo '--- crates/vm/src/builtins/function.rs: light-frame invocation ---'
sed -n '840,965p' crates/vm/src/builtins/function.rs

echo '--- all light-frame state access and heavy-frame entry call sites ---'
rg -n -C 4 -P '\b(get_current_light_frame|set_current_light_frame|saved_current_frame|run_frame\s*\(|with_frame\s*\(|ensure_heavy\s*\()' \
  crates/vm/src --glob '*.rs'

Length of output: 34501


@fanninpm, confirmed — the concern is valid.

frame_at_offset_vm explicitly models this case: it walks “heavy frames pushed by native callbacks from within a light frame” before returning the active light frame. So the unified stack can be:

heavy callback frame → light caller → earlier heavy frame

But VirtualMachine::with_frame sets the callback frame’s previous pointer from set_current_frame, which is only the prior heavy frame. Py<Frame>::f_back follows only that pointer through find_owned_chain_frame / retained_back; neither path consults or materializes CURRENT_LIGHT_FRAME.

Therefore, when a heavy frame is entered while a light frame is active, sys._getframe() can report the light frame as its logical caller, while heavy_frame.f_back skips it. That breaks the expected adjacent-frame invariant.

The fix needs to materialize/retain the active light frame as the heavy callback frame’s back-link at the heavy-frame entry boundary, or the affected CPython test needs an explicit temporary expected-failure marker if this remains an acknowledged limitation.

@youknowone

Copy link
Copy Markdown
Member Author

@fanninpm Thank you so much, it is unexpectedly very helpful 😂

- Fix cspell: 'amortised' → 'amortized'
- Sync lasti, prev_line, and fastlocals when re-observing a materialized
  light frame, fixing stale f_locals/f_lineno in test_inspect and others
- In with_frame, materialize any active light frame as the heavy frame's
  predecessor and store it in retained_back, so f_back and inspect.stack
  see the correct interleaved order (H_new → L_mat → H_old)
- Guard invoke_light_slots on NEWLOCALS | OPTIMIZED code flags

Assisted-by: Claude
Copilot AI review requested due to automatic review settings July 23, 2026 17:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/vm/src/vm/mod.rs:1727

  • with_frame() increments recursion_depth before installing the scopeguard::defer! that decrements it. A panic between the increment and the defer installation (e.g., during materialize_light_frame_pub / Frame::new allocations) would leak recursion_depth, potentially causing spurious RecursionError later in the thread.
        self.recursion_depth.update(|d| d + 1);

Comment thread crates/vm/src/frame.rs
Comment on lines 301 to +302
/// The current thread's topmost frame object, if any.
/// If light frames are active, they are on top of the heavy chain.
Comment thread crates/vm/src/frame.rs
Comment on lines +278 to +282
/// Public wrapper for `materialize_light_frame`.
///
/// # Safety
/// `light` must point to a valid LightFrame whose borrowed pointers are still alive.
pub unsafe fn materialize_light_frame_pub(light: *mut LightFrame, vm: &VirtualMachine) -> FrameRef {
@youknowone

Copy link
Copy Markdown
Member Author

the implementation direction is wrong. reworking

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.

3 participants