Skip to content

Simplify cache_entries match statement#7613

Open
ShaharNaveh wants to merge 2 commits intoRustPython:mainfrom
ShaharNaveh:simplify-cache-match
Open

Simplify cache_entries match statement#7613
ShaharNaveh wants to merge 2 commits intoRustPython:mainfrom
ShaharNaveh:simplify-cache-match

Conversation

@ShaharNaveh
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh commented Apr 15, 2026 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Refactor
    • Simplified the compiler's instruction cache estimation, reducing internal complexity and improving maintainability.
    • Adjusted cache-count assignments for several instruction types; this may slightly alter cache accounting and optimization behavior in generated bytecode.
    • No public APIs or external interfaces were changed.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 15, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 8515d206-d986-472c-9241-83ad4446199c

📥 Commits

Reviewing files that changed from the base of the PR and between d929f81 and 44ddb01.

📒 Files selected for processing (1)
  • crates/compiler-core/src/bytecode/instruction.rs
📝 Walkthrough

Walkthrough

Instruction::cache_entries now calls deoptimize() and maps selected deoptimized opcodes to fixed cache counts; unmapped deoptimized results return 0. This replaces the previous exhaustive per-variant match and changes handling for instrumented/specialized opcodes.

Changes

Cohort / File(s) Summary
Cache sizing logic
crates/compiler-core/src/bytecode/instruction.rs		 Rewrote cache_entries() to compute counts from deoptimize() results rather than an exhaustive match. Maps specific deoptimized/base variants to fixed counts (LoadAttr→9, BinaryOp→5, LoadGlobal/StoreAttr→4, Call/CallKw/ToBool→3, several control/data-flow variants→1) and returns 0 for other deoptimized outcomes; alters handling for instrumented/specialized opcodes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through bytecode, ears all alert,

I deopted branches and trimmed all the dirt,
Cache slots aligned in tidy little rows,
Fewer matches now — that's how efficiency grows,
A rabbit's soft refactor, neatly in prose.

🚥 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 describes the main change: refactoring the cache_entries match statement to a simpler implementation using deoptimization-based mapping.
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

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.

@ShaharNaveh ShaharNaveh force-pushed the simplify-cache-match branch 3 times, most recently from a0921b8 to 032213d Compare April 15, 2026 17:57
coderabbitai[bot]
coderabbitai bot reviewed Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a 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 the current code and only fix it if needed.

Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 700-702: Update the doc comment in
crates/compiler-core/src/bytecode/instruction.rs that currently reads
"Instrumented and specelized opcodes have the same cache entries as their base."
— correct the typo by replacing "specelized" with "specialized" in the comment
above the opcode definitions (the comment describing instrumented and
specialized opcodes).
🪄 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

Run ID: ad4c7fd5-b818-4fef-9f68-3e70dcad1bbb

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee1faf and 032213d.

📒 Files selected for processing (1)
  • crates/compiler-core/src/bytecode/instruction.rs

Comment thread crates/compiler-core/src/bytecode/instruction.rs
ShaharNaveh
ShaharNaveh commented Apr 15, 2026
View reviewed changes
Comment thread crates/compiler-core/src/bytecode/instruction.rs Outdated
@ShaharNaveh ShaharNaveh force-pushed the simplify-cache-match branch from d929f81 to 44ddb01 Compare April 15, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ShaharNaveh