-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
gh-103193: Micro-optimise helper functions for inspect.getattr_static
#103195
gh-103193: Micro-optimise helper functions for inspect.getattr_static
#103195
Conversation
I can find one significant bug report open regarding Having studied the bug report, I don't think this patch impacts the bug (or any possible solutions to the bug) either way. |
Hello, Alex! |
There's not really any difference between the two; it's just a matter of taste. Some people consider In this case, the main reason I'm accessing it via |
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.
There are several things I can think of to speed up getattr_static
.
- Change
_check_instance
to be
def _check_instance(obj, attr):
try:
instance_dict = object.__getattribute__(obj, "__dict__")
except AttributeError:
return _sentinel
return dict.get(instance_dict, attr, _sentinel)
- Here
type
is called twice:
if (_check_class(type(klass_result), '__get__') is not _sentinel and
_check_class(type(klass_result), '__set__') is not _sentinel):
Ideally we can also call _check_class
once with several attributes 🤔
Because in this case it will allow us to decrease the amount of _static_mro
calls.
I think that a variable might be a bit faster
Those sound great! My instinct is to tackle them in a separate PR, so that each change can be evaluated and benchmarked independently. Would you like to submit a PR? |
Yes, I will do it tomorrow 👍 |
Would it be ok to have |
I considered it, but felt like it would make it significantly less readable ( |
IMO, since we are already micro-optimizing an internal helper of an internal function, we may not necessarily need to be exact so perhaps we can name it |
That's a decent name. Or maybe I'll make the change — thanks! |
I pushed the change and updated the benchmark results in the PR description (it's maybe a teeny tiny bit faster than it was, but not by much) |
I think the impact will be clearer when we have a complicated inheritance diagram (e.g., large projects with abstract interfaces and/or mixins here and there). |
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.
Very nice!
I tried these out locally, but unfortunately I can't measure any speedup from them :/ It's possible I'm not using the right benchmark to show a speedup, however -- happy to be proven wrong if you can get a speedup somewhere! |
Micro-optimising
inspect._static_getmro
andinspect._shadowed_dict
leads to a significant improvement in the time taken to callisinstance()
on a runtime-checkable protocol. (This is a useful benchmark, as it's a real-world use ofinspect.getattr_static
in a tight loop, that's found in the stdlib.)Benchmark results on a0305c5:
Benchmark results with this PR:
Benchmark script:
inspect.getattr_static
#103193