Skip to content

Reduce Python→Python call overhead with LightFrame#8354

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

Reduce Python→Python call overhead with LightFrame#8354
youknowone wants to merge 2 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: c3593fc2-ab1f-40ed-9259-67b3687835fd

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
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 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 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);

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