Split InterpreterFrame hot/cold fields into FrameColdData by youknowone · Pull Request #8434 · RustPython/RustPython

youknowone · 2026-08-02T17:39:54Z

Summary

Move 10 rarely-used fields from InterpreterFrame into a lazily-allocated FrameColdData struct, reducing per-frame initialization cost.

Part of youknowone#40 (call overhead optimization).

Motivation

InterpreterFrame carries ~200+ bytes of tracing/debugging fields that are only accessed when:

sys.settrace() / sys.setprofile() is active
Frame inspection (f_back, f_locals, f_trace) occurs
GC traversal runs

On every Python call, all these fields were initialized even though the vast majority of calls never touch them.

Changes

These 10 fields are now behind a single UnsafeCell<Option<Box<FrameColdData>>> (8 bytes):

trace, trace_lines, trace_opcodes
temporary_refs, f_locals_hidden_overlay, f_extra_locals
escaped, retained_back
pending_stack_pops, pending_unwind_from_stack

The cold() accessor lazily allocates on first use. Frames that never trigger tracing or debugging pay zero allocation cost. GC traversal skips cold data when it has not been allocated.

Files changed

frame.rs — FrameColdData struct, field migration, cold() accessor, GC traverse update
builtins/frame.rs — f_trace, f_trace_lines, f_trace_opcodes, f_back, clear() accesses
object/ext.rs — swap_to_temporary_refs()
protocol/callable.rs — Trace event handling
vm/mod.rs — Frame cleanup, tracing hooks
builtins/type.rs — __bases__ setter temporary_refs
stdlib/_thread.rs — Cross-thread frame materialization

Testing

All frame/traceback/generator/call/sys/json tests pass.

Summary by CodeRabbit

Performance
- Reduced memory usage by allocating infrequently used interpreter state only when needed.
- Improved efficiency for workloads involving tracing, stack management, temporary references, and frame retention.
Bug Fixes
- Preserved existing behavior for tracing, stack unwinding, cleanup, and frame-chain operations.
- Maintained consistent behavior across supported platforms and frame execution modes.

coderabbitai · 2026-08-02T17:40:15Z

📝 Walkthrough

Walkthrough

InterpreterFrame cold state is consolidated into optional heap-allocated FrameColdData. Frame construction, cleanup, tracing, locals handling, retained back-links, unwinding, garbage collection, and related VM paths now use cold().

Changes

Frame cold-state storage

Layer / File(s)	Summary
Cold storage lifecycle `crates/vm/src/frame.rs`	`FrameColdData` groups cold frame state. `InterpreterFrame` allocates it lazily. Constructors and GC traversal handle optional cold data.
Frame state operations `crates/vm/src/frame.rs`	Cleanup, escape tracking, locals proxies, retained callers, tracing flags, and deferred unwinding use cold storage.
Runtime cold-state access `crates/vm/src/builtins/frame.rs`, `crates/vm/src/builtins/type.rs`, `crates/vm/src/object/ext.rs`, `crates/vm/src/protocol/callable.rs`, `crates/vm/src/stdlib/_thread.rs`, `crates/vm/src/vm/mod.rs`	Frame APIs, temporary-reference retention, tracing, frame materialization, and VM dispatch use `iframe().cold()` or `cold()`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

RustPython/RustPython#7631: Shares frame locals access and cleanup changes in crates/vm/src/frame.rs and crates/vm/src/builtins/frame.rs.
RustPython/RustPython#8354: Refines the same InterpreterFrame cold-state refactor.
RustPython/RustPython#8433: Implements the same FrameColdData split and related frame-state access updates.

Suggested reviewers: copilot, shaharnaveh

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: moving hot and cold fields from InterpreterFrame into FrameColdData.

✨ 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

Pull request overview

This PR optimizes call overhead by moving rarely-used tracing/debug/inspection state out of InterpreterFrame into a lazily-initialized “cold” allocation (FrameColdData), aiming to reduce per-call frame initialization costs while still supporting tracing, frame inspection, and GC traversal when needed.

Changes:

Introduces FrameColdData and routes tracing/inspection-related fields through InterpreterFrame::cold().
Updates tracing hooks and frame APIs (f_trace*, f_back, temporary ref handling, retained backrefs) to use the cold storage.
Adjusts GC traversal to skip cold data when not allocated.

Reviewed changes

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

Show a summary per file

File	Description
crates/vm/src/frame.rs	Adds `FrameColdData`, migrates fields into lazy cold storage, updates traversal and frame APIs accordingly.
crates/vm/src/builtins/frame.rs	Updates Python-visible frame attribute accessors and clear/backref logic to read/write cold fields.
crates/vm/src/object/ext.rs	Routes `swap_to_temporary_refs()` to store old refs in cold `temporary_refs`.
crates/vm/src/protocol/callable.rs	Updates tracing dispatch logic to consult cold `trace_opcodes` / clear cold `trace`.
crates/vm/src/vm/mod.rs	Updates retained-back capture, temporary-ref cleanup, and trace event handling to use cold fields.
crates/vm/src/builtins/type.rs	Updates `__bases__` setter keep-alive path to use cold `temporary_refs`.
crates/vm/src/stdlib/_thread.rs	Updates cross-thread frame materialization backref retention to use cold `retained_back`.

Suppressed comments (1)

crates/vm/src/frame.rs:1582

has_escaped() currently calls cold(), which forces allocation of FrameColdData even for the common case where nothing escaped (e.g. coroutine/generator close checks this routinely). That undermines the stated goal of paying zero allocation cost when tracing/inspection isn’t used.

has_escaped() can treat “cold not allocated yet” as false and avoid allocating/locking in the hot path.

    pub(crate) fn has_escaped(&self) -> bool {
        self.iframe().cold().escaped.load(atomic::Ordering::Acquire)
    }

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

        // Opcode events are only dispatched when f_trace_opcodes is set.
-        if is_opcode_event && !*frame_ref.iframe().trace_opcodes.lock() {
+        if is_opcode_event && !*frame_ref.iframe().cold().trace_opcodes.lock() {
            return Ok(None);
        }


        if self.use_tracing.get()
-            && (frame.iframe().trace.lock().is_some() || !self.is_none(&self.profile_func.borrow()))
+            && (frame.iframe().cold().trace.lock().is_some() || !self.is_none(&self.profile_func.borrow()))
        {


+    pub(crate) fn cold(&self) -> &FrameColdData {
+        let ptr = self.cold.get();
+        unsafe { (*ptr).get_or_insert_with(|| Box::new(FrameColdData::default())) }
+    }


coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 1164-1170: Update InterpreterFrame::cold to replace the
UnsafeCell-based lazy initialization with
std::sync::OnceLock<Box<FrameColdData>>. Initialize the OnceLock through its
thread-safe one-time initialization API and return the stored FrameColdData
reference, preserving default allocation on first use and ensuring concurrent
callers share a single allocation.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 224ac896-8746-45d3-acd9-99e6b4d3580d

📥 Commits

Reviewing files that changed from the base of the PR and between 12f3646 and 896cca5.

📒 Files selected for processing (7)

crates/vm/src/builtins/frame.rs
crates/vm/src/builtins/type.rs
crates/vm/src/frame.rs
crates/vm/src/object/ext.rs
crates/vm/src/protocol/callable.rs
crates/vm/src/stdlib/_thread.rs
crates/vm/src/vm/mod.rs

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/vm/src/frame.rs:1173

InterpreterFrame::cold() performs unsynchronized lazy initialization via UnsafeCell<Option<...>>. Under feature="threading", InterpreterFrame is Send + Sync (see unsafe impl Send/Sync for InterpreterFrame in this file), and FrameObject APIs like frame.clear() explicitly support being called from a different thread. That combination means two threads can race to initialize cold, which is UB (data race) even though the fields inside FrameColdData are mutex/atomic protected.

Consider switching cold to a thread-safe one-time init primitive already used in this repo (e.g. rustpython_common::lock::OnceCell/OnceLock) so initialization is race-free, and adjust the GC traverse path to use cold.get() instead of raw pointer reads.

    /// Access the lazily-allocated cold data, allocating on first use.
    #[inline]
    pub(crate) fn cold(&self) -> &FrameColdData {
        let ptr = self.cold.get();
        unsafe { (*ptr).get_or_insert_with(|| Box::new(FrameColdData::default())) }
    }

Move 10 rarely-used fields (trace, trace_lines, trace_opcodes, temporary_refs, f_locals_hidden_overlay, f_extra_locals, escaped, retained_back, pending_stack_pops, pending_unwind_from_stack) from InterpreterFrame into a lazily-allocated FrameColdData struct. InterpreterFrame now carries a single UnsafeCell<Option<Box<FrameColdData>>> (8 bytes) instead of ~200+ bytes of cold fields. The cold() accessor allocates on first access; frames that never trigger tracing or debugging pay no allocation cost. GC traversal skips cold data when it has not been allocated. Assisted-by: Claude

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

ShaharNaveh

🔥

Copilot AI review requested due to automatic review settings August 2, 2026 17:39

Copilot started reviewing on behalf of youknowone August 2, 2026 17:40 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

coderabbitai Bot reviewed Aug 2, 2026

View reviewed changes

Comment thread crates/vm/src/frame.rs

coderabbitai Bot mentioned this pull request Aug 2, 2026

Flatten Python eval loop with trampoline for reduced call overhead #8431

Open

youknowone force-pushed the frame-cold-split-standalone branch from 896cca5 to e48d496 Compare August 2, 2026 23:50

youknowone requested review from ShaharNaveh and Copilot August 2, 2026 23:50

Copilot started reviewing on behalf of youknowone August 2, 2026 23:50 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 23:55

youknowone force-pushed the frame-cold-split-standalone branch from e48d496 to ff53f27 Compare August 2, 2026 23:55

Copilot started reviewing on behalf of youknowone August 2, 2026 23:55 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

ShaharNaveh approved these changes Aug 3, 2026

View reviewed changes

youknowone merged commit 9dff4d3 into RustPython:main Aug 3, 2026
27 checks passed

youknowone deleted the frame-cold-split-standalone branch August 3, 2026 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split InterpreterFrame hot/cold fields into FrameColdData - #8434

Split InterpreterFrame hot/cold fields into FrameColdData#8434
youknowone merged 1 commit into
RustPython:mainfrom
youknowone:frame-cold-split-standalone

youknowone commented Aug 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

ShaharNaveh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

youknowone commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Changes

Files changed

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

youknowone commented Aug 2, 2026 •

edited

Loading

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading