GeezerVison

Removing request for review from @gvanrossum since he's on vacation :)

Cc. @posita, for interest :)

Although I agree that runtime patching of a Protocol is pretty unlikely in practice, I still find this change uncomfortable, because in case any runtime patching does occur, the new behavior is somewhat unpredictable and hard to specify accurately. Effectively the state of the Protocol will be "locked in" whenever the first isinstance check against it occurs. So patches that take effect before that will be respected, and after that won't be.

I feel like "runtime patching of a Protocol is not ever respected" would be a defensible behavior, but this inconsistent behavior seems like something we shouldn't adopt.

The ideal behavior would probably be "runtime patching of a Protocol is not even possible and errors immediately," (and then we could safely make this optimization) but that's quite difficult or impossible to implement.

There is some similar caching for ABCs. I'm not too familiar with how it works, but it might be worth looking into how that cache deals with monkeypatching. If we ignore monkeypatching there, it might be fine to do the same for Protocols.

I feel like "runtime patching of a Protocol is not ever respected" would be a defensible behavior, but this inconsistent behavior seems like something we shouldn't adopt.

We could just calculate the __protocol_attrs__ and __callable_proto_members_only__ at protocol-class creation time (in Protocol.__init_subclass__ or _ProtocolMeta.__new__) rather than at first-isinstance()-call-time. That would slow down protocol class creation a little, but probably not by much, and protocol class creation is only ever done once. Would you like that behaviour more?

We could just calculate the __protocol_attrs__ and __callable_proto_members_only__ at protocol-class creation time (in __init_subclass__) rather than at first-isinstance()-call-time. That would slow down protocol class creation a little, but probably not by much, and protocol class creation is only ever done once. Would you like that behaviour more?

I would, yeah. It seems like a consistent behavior that we could document, that the state of a runtime-checkable Protocol is locked in when it is defined and runtime patching of the Protocol class has no effect.

It also seems unlikely that someone would bother to decorate a Protocol as runtime-checkable and then never runtime check anything against it, so I'm not sure how often the laziness would offer a benefit?

It's entirely possible I'm being too conservative here, though -- if @JelleZijlstra is right that there is already similar caching in ABCs with similar issues, that would be a strong signal that we don't need to care here.

It also seems unlikely that someone would bother to decorate a Protocol as runtime-checkable and then never runtime check anything against it, so I'm not sure how often the laziness would offer a benefit?

Maybe a library that provides a large number of runtime-checkable protocols as utilities? Doing the calculation of these attributes at class-creation time could plausibly slow down the import of that library, and the user of the library might end up only using the classes for type annotations, never doing any isinstance() checks.

But, doesn't really feel like a big deal.

It's entirely possible I'm being too conservative here, though -- if @JelleZijlstra is right that there is already similar caching in ABCs with similar issues, that would be a strong signal that we don't need to care here.

I also have no idea how the ABC caching works, but will experiment and get back to you!

Ah yes, here's the ABC caching behaviour:

>>> import collections.abc
>>> class Foo:
...     def __iter__(self):
...         while True: yield 42
...
>>> isinstance(Foo(), collections.abc.Iterable)
True
>>> iter(Foo())
<generator object Foo.__iter__ at 0x000001DBE55B7040>
>>> del Foo.__iter__
>>> isinstance(Foo(), collections.abc.Iterable)
True
>>> iter(Foo())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'Foo' object is not iterable

Given that behaviour, it could well be reasonable to slap a cache on the whole _ProtocolMeta.__instancecheck__ call, similar to the way beartype does it... though I still don't much like the idea of such a big behaviour change tbh.

I do actually think it will make the code easier to read and understand, apart from anything else, if we do the calculation at class-creation time, rather than lazily as I currently have it in my PR. So I think I will make that change.

Moving the calls to protocol-class-creation time makes the benchmark even faster (I've updated the numbers in my initial post).

This comes at the cost of making protocol class creation about 40% slower. I think that's actually a reasonable trade-off. Creating the protocol class only needs to be done once, whereas isinstance() checks against the protocol class may need to be done repeatedly. Moreover, a 40% slowdown sounds severe, but I doubt it will actually lead to import times getting slower anywhere. E.g. typing.py defines seven runtime-checkable protocols, but importing typing doesn't seem to get significantly slower (if at all) with this PR.

This change is clearly newsworthy, but I'm adding "skip news" for now -- I'll add a NEWS entry (and possibly a note in "What's new in 3.12") after all the performance-related PRs have been decided on (and possibly merged).

main and this PR branch are both slower after 6d59c9e, so I've updated the benchmark results in the PR description.

I confirmed locally that this still provides a big speedup, even after #103160 being merged in