GeezerVison

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

GeezerVison is showing github.com — Specialize module attribute loads by 1ndahous3 · Pull Request #8864 · RustPython/RustPython

Conversation

1ndahous3 commented Sep 27, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

This PR follows our AI policy

Summary

LOAD_ATTR on a module (math.pi, os.path, random.random()) never specialized. specialize_load_attr returned early for types with their own getattro, including modules, before reaching LoadAttrModule.

Run the exact-module branch before the default-getattro check.
Capture the module dictionary's keys version and entry index under one read lock, then read directly by index instead of repeating the type's MRO lookup and dictionary lookup. Keep the full 32-bit keys version for long-running interpreters.
Specialize only when the dictionary has exact string keys, has no __getattr__, and contains the requested name without a competing type attribute. This preserves type-level descriptors such as __class__ and __dict__, custom key equality, and generic lookup after dictionary changes.
Publish module cache guards atomically and validate the cached attribute name, preserving lookup results when threads specialize the same instruction concurrently.

Follow-up to #7386.

Performance

Windows x64 release build, medians of five runs with 500,000 operations each:

Operation	Before	After	Speedup
`math.pi`	79.26 ns	59.68 ns	1.33×
`random.random()`	180.36 ns	155.88 ns	1.16×
`time.monotonic()`	146.99 ns	119.58 ns	1.23×

AI assistance

Written with Claude Code (claude-opus-5-5) and Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

Performance
- Optimized repeated module attribute lookups with caching for eligible module dictionaries.
- Cache entries are checked against the current dictionary layout and attribute name to avoid returning stale values.
Bug Fixes
- Preserved correct behavior for module attributes when dictionaries change, custom attribute handling is present, or attributes are missing.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Specialize module attribute loads

dfe4bb9

Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Codex:GPT-6

chatgpt-codex-connector Bot commented Sep 27, 2026

Copy link

Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

All reactions

Sorry, something went wrong.

Uh oh!

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

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: a93b4032-65a9-4df8-b4ca-44f70b7373d5

📥 Commits

Reviewing files that changed from the base of the PR and between f871707 and dfe4bb9.

📒 Files selected for processing (4)

crates/vm/src/builtins/dict.rs
crates/vm/src/dict_inner.rs
crates/vm/src/frame.rs
extra_tests/snippets/vm_specialization.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

Module attribute loads now use guarded dictionary-cache entries for exact modules. The change adds cache helpers, updates specialization and lookup behavior, and adds tests for cache guards and attribute changes.

Changes

Module Attribute Cache

Layer / File(s)	Summary
Dictionary cache helpers `crates/vm/src/dict_inner.rs`, `crates/vm/src/builtins/dict.rs`	The dictionary helpers expose a cache only for exact-string keys when `__getattr__` is absent. Cached reads validate the keys version, entry index, and requested name. Tests cover key layouts, invalid cache pairs, non-string keys, `__getattr__`, and non-interned exact strings.
Module attribute specialization `crates/vm/src/frame.rs`, `extra_tests/snippets/vm_specialization.py`	Specialization records the module type version, dictionary keys version, and entry index for cacheable attributes. `LoadAttrModule` uses the guarded cache and falls back to slow lookup on a miss. Tests cover attribute updates and deletion, missing attributes, dictionary replacement, rebinding, special attributes, and custom dictionary keys.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Frame
  participant Module
  participant Dict
  Frame->>Module: Check module type version and read dictionary
  Frame->>Dict: Look up cached attribute by keys version and index
  Dict-->>Frame: Return cached value or cache miss
  Frame->>Frame: Use slow attribute lookup after cache miss

Merge Risk: ⚪ Minimal · up to dfe4b

No confirmed merge-blocking issue remains; this change is ready for normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to dfe4b

The new fast path has guards that appear to preserve normal module attribute lookup, including a fallback when its assumptions no longer hold. No concrete security bypass was established, though concurrent cache publication remains an area of uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The directly affected security boundary is attribute lookup for Python code executing in the VM, including modules whose dictionaries change at runtime; the inspected change does not establish a new cross-service authority path.

Trust Boundaries and Controls

observed — A cached hit requires a matching type version and exact module, then a matching dictionary version, live indexed entry, and attribute name. Otherwise execution uses generic lookup.

Resilience and Maintainability Implications

observed — Specializers write the module cache fields separately after an initial opcode check. The reader validates the resulting fields before using them, but the inspected code does not publish the three fields as one indivisible unit.

Hardening Proposals

proposed — Add a focused concurrent-specialization check that interleaves cache-field publication and confirms mixed payloads either return the requested attribute or fall back to generic lookup.

🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	❓ Inconclusive	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and concisely describes the main change: specializing module attribute loads through a module-dictionary cache.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: 1 too large.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

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.

codspeed Bot commented Sep 27, 2026

Copy link

Copy Markdown

Contributor

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks¹

_{Comparing 1ndahous3:module_attr_specialization (dfe4bb9) with main (f871707)}

Footnotes

4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

All reactions

Sorry, something went wrong.

Uh oh!

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

youknowone approved these changes

Sep 28, 2026

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

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 d132713 into RustPython:main

Sep 28, 2026

22 checks passed

Uh oh!

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

1ndahous3 deleted the module_attr_specialization branch

September 28, 2026 16:09

Reviewers

youknowone

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

2 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

1ndahous3 commented Sep 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Performance

AI assistance

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Sep 27, 2026

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Security Architecture Review

❌ Failed checks (1 inconclusive)

Uh oh!

codspeed Bot commented Sep 27, 2026

Merging this PR will not alter performance

Footnotes

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1ndahous3 commented Sep 27, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

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

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading

Uh oh!

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