Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-103193: Micro-optimise helper functions for inspect.getattr_static#103195
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
AlexWaygood commented Apr 2, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
AlexWaygood commented Apr 2, 2023
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. |
Eclips4 commented Apr 3, 2023
Hello, Alex! |
AlexWaygood commented Apr 3, 2023
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 |
sobolevn left a comment
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_instanceto be
def_check_instance(obj, attr): try: instance_dict=object.__getattribute__(obj, "__dict__") exceptAttributeError: return_sentinelreturndict.get(instance_dict, attr, _sentinel)- Here
typeis called twice:
if (_check_class(type(klass_result), '__get__') isnot_sentineland_check_class(type(klass_result), '__set__') isnot_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
AlexWaygood commented Apr 3, 2023
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? |
sobolevn commented Apr 3, 2023
Yes, I will do it tomorrow 👍 |
picnixz commented Apr 3, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Would it be ok to have |
AlexWaygood commented Apr 3, 2023
I considered it, but felt like it would make it significantly less readable ( |
picnixz commented Apr 3, 2023
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 |
AlexWaygood commented Apr 3, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
That's a decent name. Or maybe I'll make the change — thanks! |
AlexWaygood commented Apr 3, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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) |
picnixz commented Apr 3, 2023
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). |
carljm left a comment
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!
AlexWaygood commented Apr 5, 2023
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_getmroandinspect._shadowed_dictleads 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_staticin a tight loop, that's found in the stdlib.)Benchmark results on a0305c5:
Benchmark results with this PR:
Benchmark script:
inspect.getattr_static#103193