-
Notifications
You must be signed in to change notification settings - Fork 783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make use of intern! macro for attribute names used internally #2273
Conversation
d3cf58d
to
8ddcc77
Compare
0958a78
to
89577a2
Compare
8ddcc77
to
580cb7b
Compare
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.
Sneaky idea that should be nice for performance! It makes me a little uneasy if we know we might have to regress this for PEP 489 combatibility later, however it's still a nice win for the meanwhile.
Related idea: add a comment to |
580cb7b
to
2c95b3a
Compare
Add a comment to the methods of |
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.
I think an example would not go amiss.
05f0123
to
f02a060
Compare
Added a single example to each of the four cases. |
Thanks a lot! |
Since we do use several attribute names internally, I looked into whether some of them could be candidates for interning. Personally, I do not think this is obvious as while identifiers like
__name__
will most likely already be part of Python's string interner dictionary, they do not seem to be used in a hot path of PyO3.I therefore split this into separate commits to solicit feedback on whether we want to include any of them:
__qualname__
used inPyType::name
. Here, I am not sure how often production code using PyO3 actually calls this.__all__
and__name__
used in the implementation ofPyModule
. This seems to be limited to initialization, but the strings are almost surely interned already by other code.getattr
calls emitted by theFromPyObject
derive macro. I think this has the largest possible benefit as this could end up being used in every Python-to-Rust call. But I am also not sure whether enabling this unconditionally is a good idea. (Since the names are given via code and not generated dynamically, this should be ok?)