GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Split InterpreterFrame hot/cold fields into FrameColdData by youknowone · Pull Request #8434 · RustPython/RustPython

Conversation

youknowone commented Aug 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot AI review requested due to automatic review settings

August 2, 2026 17:39

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

📝 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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Copilot started reviewing on behalf of youknowone

August 2, 2026 17:40

View session

Copilot AI reviewed

Aug 2, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/protocol/callable.rs

Comment on lines 235 to 238

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

Comment thread

crates/vm/src/vm/mod.rs

Comment on lines 2013 to 2015

		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()))
		{

Comment thread

crates/vm/src/frame.rs

Comment on lines +1167 to +1170

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

coderabbitai Bot reviewed

Aug 2, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/frame.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot mentioned this pull request

Aug 2, 2026

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

Merged

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 left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

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 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())) }
    }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Split InterpreterFrame hot/cold fields into FrameColdData

ff53f27

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

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

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 7 out of 7 changed files in this pull request and generated no new comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ShaharNaveh approved these changes

Aug 3, 2026

View reviewed changes

ShaharNaveh left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🔥

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit 9dff4d3 into RustPython:main

Aug 3, 2026

27 checks passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the frame-cold-split-standalone branch

August 3, 2026 07:00

This was referenced Aug 5, 2026

Perf: instance attribute access still ~7-11x slower than CPython (specialized LOAD_ATTR does a full dict lookup) youknowone/RustPython#41

Open

Perf: small int/float arithmetic allocates heap objects; specialized BinaryOp*Int skips the i64 fast path youknowone/RustPython#43

Open

Perf: GC tracking takes a global lock per allocation; refcounts always atomic — free-threading-friendly mitigations youknowone/RustPython#44

Open

youknowone added a commit that referenced this pull request

Sep 16, 2026


          Split InterpreterFrame hot/cold fields into FrameColdData (#8434)

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

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

ShaharNaveh

ShaharNaveh approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

youknowone commented Aug 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.