-
-
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-74690: typing: Don't unnecessarily call _get_protocol_attrs
twice in _ProtocolMeta.__instancecheck__
#103141
gh-74690: typing: Don't unnecessarily call _get_protocol_attrs
twice in _ProtocolMeta.__instancecheck__
#103141
Conversation
…` twice in `_ProtocolMeta.__instancecheck__`
return True | ||
if cls._is_protocol: | ||
|
||
protocol_attrs = _get_protocol_attrs(cls) |
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.
We could possibly cache the result of this call on the protocol class in a __protocol_attrs__
class attribute. But I think I'd rather look at that in a separate PR so that we can evaluate the impact of it in isolation. I'd also want to check to see if there were any behaviour changes, and compare different methods of caching the result of _get_protocol_attrs
.
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.
We could possibly cache the result of this call on the protocol class in a
__protocol_attrs__
class attribute. ...
If you decide to tackle this, @beartype provides a caching mechanism for __instancecheck__
results that may be worth a look, if only for inspiration. I believe some of the same caveats would apply. (Full disclosure: I am an author. Happy to chat, if helpful.)
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 share @ilevkivskyi's reservations in #74690 (comment) about caching the whole call to __instancecheck__
in the way that I believe beartype does it, but I think caching the call to _get_protocol_attrs
could get us a large speedup without the same risk of changing the behaviour. (Apologies if I was unclear in my comment above!) Anyway, TBD in a future PR :)
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.
Is monkey-patching of the protocol class itself something we need to care about here?
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.
Is monkey-patching of the protocol class itself something we need to care about here?
Pretty unlikely imo. But yeah, it's that kind of thing that I'd want to think through, which is why I'd probably like to leave that to another PR :)
Some local experimentation (by combining the two branches) indicates that this PR doesn't quite make up for the performance hit from #103034, but does significantly mitigate it. |
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.
This change LGTM. Nice find!
I discussed with @carljm offline, and we decided it's probably better to skip news for this one. On its own it's a performance improvement, but it'll likely be accompanied by #103034(which hurts performance of runtime-checkable protocols), and possibly one or two other optimisations to this code that I'm eyeing up. So the overall impact on performance for runtime-checkable protocols is pretty uncertain at the moment, which would mean including a news entry at this stage would probably just cause confusion to readers of the changelog. |
Nice find! For what it's worth, I've found users often depend on undocumented aspects of typing.py, so I'd have still included a vague news entry on the lines of "Changed the performance characteristics of |
|
…` twice in `_ProtocolMeta.__instancecheck__` (python#103141) Speed up `isinstance()` calls against runtime-checkable protocols
This simple optimisation achieves speedups for all kinds of
isinstance()
checks against runtime-checkable protocols. For structural subtypes that set attributes in__init__
methods, the speedup is dramatic.Benchmark script
Results on 01a49d1 (non-debug build, PGO-optimised):
Results with this PR:
(We may want to wait until #103034 is merged and then re-measure.)
Cc. @leycec and @posita, as people who I know care a lot about
Protocol
performance :)