Skip to content

perf: use interned name in generic_getattr_opt#8129

Open
sharifhsn wants to merge 1 commit into
RustPython:mainfrom
sharifhsn:perf/getattr-cached-hash
Open

perf: use interned name in generic_getattr_opt#8129
sharifhsn wants to merge 1 commit into
RustPython:mainfrom
sharifhsn:perf/getattr-cached-hash

Conversation

@sharifhsn

@sharifhsn sharifhsn commented Jun 18, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown

Summary

generic_getattr_opt looked up the instance dict by name_str.as_wtf8(), which required recomputing the SipHash from the raw bytes on every attribute access. Now name_str: &Py<PyStr> implements DictKey, which returns the hash PyStr already caches, and adds a pointer-equality fast path for the key compare, which is much faster than using .as_wtf8().

Benchmarks vs main, median: attribute-read loop -17.3%, pystone -3.0%, OOP method loop -7.9%; neutral on non-attribute code.

Assisted-by: Claude:claude-opus-4-8

Summary by CodeRabbit

  • Refactor
    • Optimized attribute lookup performance by improving the efficiency of dictionary lookups during attribute resolution.

@sharifhsn
perf: look up instance __dict__ by the interned name (cached hash) in…
dd4ad34 
… getattr

generic_getattr_opt looked up the instance dict by name_str.as_wtf8() -- the &Wtf8 content, whose DictKey::key_hash recomputes the SipHash from the raw bytes on every attribute access. Look it up by name_str (the &Py<PyStr>) instead: its DictKey returns the hash PyStr already caches, and adds a pointer-equality fast path for the key compare. (generic_setattr already passed the PyStr, so writes were never affected.)

Free -- no memory added; the hash cache already exists. Benchmarks (interleaved A/B vs main, median): attribute-read loop -17.3%, pystone -3.0%, OOP method loop -7.9%; neutral on non-attribute code. Verified: 12 attribute/descriptor/class/dataclass test modules pass; clippy clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Assisted-by: Claude:claude-opus-4
@coderabbitai

coderabbitai Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

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: eb771081-c3dc-4c96-bca1-179ce53a3b98

📥 Commits

Reviewing files that changed from the base of the PR and between fe2a7db and dd4ad34.

📒 Files selected for processing (1)
  • crates/vm/src/protocol/object.rs
📝 Walkthrough

Walkthrough

In PyObject::generic_getattr_opt, the temporary conversion of name_str to &Wtf8 is removed. The instance-dictionary lookup now calls dict.get_item_opt(name_str, vm) with the original Py<PyStr> directly, exploiting its cached hash and pointer-equality fast path. No public APIs are changed.

Changes

Instance dict lookup optimization in generic_getattr_opt

Layer / File(s) Summary
Remove Wtf8 conversion and use cached-hash dict lookup
crates/vm/src/protocol/object.rs		 Removes the &Wtf8 local derived from name_str and changes the instance-dict get_item_opt call to pass name_str (Py<PyStr>) directly; adds comments describing the cached-hash and pointer-equality fast path.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit hops past, no detour to take,
The Wtf8 path—deleted for speed's sake.
The hash is already cached, the pointer is near,
No extra conversion, the lookup is clear!
🐇✨ Fewer steps, same result—hip hip hooray!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'perf: use interned name in generic_getattr_opt' accurately describes the main optimization: using the interned PyStr directly instead of converting to WTF-8 bytes for dictionary lookups, achieving significant performance improvements.
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.

✏️ 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 requested a review from youknowone June 18, 2026 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone Awaiting requested review from youknowone

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

@sharifhsn