Remove cells_frees duplicate storage from Frame#7351

youknowone

Cell/free variable objects were stored in both a separate Box<[PyCellRef]> (cells_frees field) and in the localsplus fastlocals array. Remove the redundant cells_frees field and access cell objects directly through localsplus, eliminating one Box allocation and N clone operations per frame creation.

Summary by CodeRabbit

Refactor
- Optimized VM frame variable storage mechanism by consolidating cell and free variable management for improved internal efficiency and code maintainability.

Cell/free variable objects were stored in both a separate `Box<[PyCellRef]>` (cells_frees field) and in the localsplus fastlocals array. Remove the redundant cells_frees field and access cell objects directly through localsplus, eliminating one Box allocation and N clone operations per frame creation.

coderabbitai

📝 Walkthrough

Walkthrough

Frame's cell and free variable storage refactored from a separate cells_frees buffer to inline placement within LocalsPlus.fastlocals array. Frame initialization, accessors, and instruction implementations updated accordingly to use new ExecutingFrame.cell_ref() helper method.

Changes

Cohort / File(s)	Summary
Cell/Free Variable Storage Refactoring `crates/vm/src/frame.rs`	Removed `cells_frees` field from Frame struct; cell and free variables now stored directly in `LocalsPlus.fastlocals` at positions after `nlocals`. Introduced `ExecutingFrame.cell_ref()` method for accessing cells. Updated Frame initialization, `set_cell_contents()`, `get_cell_contents()`, and Traverse implementation. Updated instruction implementations (DeleteDeref, LoadDeref, StoreDeref, etc.) to use `cell_ref()` instead of direct `cells_frees` access.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

LoadClosure as pseudo op #6789: Directly modifies cell/free variable storage in Frame by moving it into fastlocals and removing cells_frees field—core code-level intersection with this PR.
Clear frame locals and stack on generator close + Add dir_fd support for rmdir, remove/unlink, scandir #7222: Updates Frame cell storage and access patterns in crates/vm/src/frame.rs with overlapping changes to per-frame cell variable handling.
Optimize fast_locals and atomic ordering #7289: Modifies Frame's fastlocals and cell storage paths with related refactoring of cell access mechanisms.

Poem

🐰 A rabbit hops through fastlocals with glee,
Cell variables inline where they belong to be,
No separate buffer to hop around,
Storage unified—efficiency found! 🥕✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: removing the redundant cells_frees field from Frame that was duplicating cell/free variable storage.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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 and usage tips.}

github-actions

Code has been automatically formatted

The code in this PR has been formatted using:

cargo fmt --all
Please pull the latest changes before pushing again:

git pull origin worktree-frame-datastack

chatgpt-codex-connector

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 743-744: Rename the misleading method clear_stack_and_cells() to
clear_stack() and update its docstring to state it only clears the value stack
(delegating cell/free clearing to clear_locals_and_stack()); inside the function
keep calling stack_clear() as before, and update all callsites and references
(including any uses alongside clear_locals_and_stack()) to the new name to
maintain behavior; ensure public API/export names are adjusted and tests/uses
compile.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 97256480-8c7b-4237-bb71-6049292ef84a

📥 Commits

Reviewing files that changed from the base of the PR and between 5c29074 and 577f474.

📒 Files selected for processing (1)

crates/vm/src/frame.rs

Auto-format: cargo fmt --all

577f474

youknowone marked this pull request as ready for review March 5, 2026 00:14

coderabbitai Bot reviewed Mar 5, 2026

View reviewed changes

youknowone mentioned this pull request Mar 5, 2026

Extract InterpreterFrame from Frame with Deref wrapper #7353

Merged

3 tasks

youknowone closed this in #7353 Mar 5, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cells_frees duplicate storage from Frame#7351