gh-74690: typing: Simplify and optimise _ProtocolMeta.__instancecheck__

[AlexWaygood]

This implements the optimisation suggested by @ilevkivskyi in #74690 (comment). However, I'm not sure if it's the right thing to do. I'm posting it here for discussion, and so that people can have a play around with it.

This dramatically speeds up isinstance() checks for nominal subclasses of runtime-checkable protocols and subclasses registered via ABCMeta.register, i.e. situations like the following:

from typing import Protocol, runtime_checkable

@runtime_checkable
class HasX(Protocol)
    x: int

class Bar(HasX):
    def __init__(self):
        self.x = 42

class Baz: ...
HasX.register(Baz)

isinstance(Bar(), HasX)  # this is ~12x faster
isinstance(Baz(), HasX)  # this is ~2x faster

However, speeding these two up comes at the cost of the performance of isinstance() checks with "structural subclasses". These are all slightly slower:

class Eggs:
    def __init__(self):
        self.x = 42

class Spam:
    x = 42

class Ham:
    @property
    def x(self):
        return 42

isinstance(Eggs(), HasX)
isinstance(Spam(), HasX)
isinstance(Ham(), HasX)

So the question is: is it worth making isinstance() calls against nominal and registered subclasses much, much faster, at the cost of making those^ isinstance() calls a bit slower? I'm not sure it is, as kind of the "whole point" of runtime-checkable protocols is that an object x doesn't have to directly inherit from a protocol X in order it to be considered an instance of X. So I don't know how common it is for people to directly inherit from protocols in their own code.

But, I'm very curious about what other people think.

Here's a benchmarking script for people to play around with. The results of the benchmark script for the structural-subclass cases are a bit noisy on my machine, but consistently a little bit slower than on main:

Benchmark script

import time
from typing import Protocol, runtime_checkable

@runtime_checkable
class HasX(Protocol):
    x: int

class Foo:
    @property
    def x(self) -> int:
        return 42

class Bar:
    x = 42

class Baz:
    def __init__(self):
        self.x = 42

class Egg: ...

class Nominal(HasX):
    def __init__(self):
        self.x = 42

class Registered: ...

HasX.register(Registered)

num_instances = 500_000
foos = [Foo() for _ in range(num_instances)]
bars = [Bar() for _ in range(num_instances)]
bazzes = [Baz() for _ in range(num_instances)]
basket = [Egg() for _ in range(num_instances)]
nominals = [Nominal() for _ in range(num_instances)]
registereds = [Registered() for _ in range(num_instances)]


def bench(objs, title):
    start_time = time.perf_counter()
    for obj in objs:
        isinstance(obj, HasX)
    elapsed = time.perf_counter() - start_time
    print(f"{title}: {elapsed:.2f}")


bench(foos, "Time taken for objects with a property")
bench(bars, "Time taken for objects with a classvar")
bench(bazzes, "Time taken for objects with an instance var")
bench(basket, "Time taken for objects with no var")
bench(nominals, "Time taken for nominal subclass instances")
bench(registereds, "Time taken for registered subclass instances")

• Issue: Performance of typing._ProtocolMeta._get_protocol_attrs and isinstance #74690

[gvanrossum]

I’m still on vacation, and when I get back I will be overwhelmed by catching up, so it may take a while.

[AlexWaygood]

I’m still on vacation, and when I get back I will be overwhelmed by catching up, so it may take a while.

The GitHub machinery automatically requested your review because you're a CODEOWNER. I think there's more than enough eyes on this; feel free to remove your request for review if you're on vacation! :)

[gvanrossum]

Ah. then I’m beginning to get annoyed by CODEOWNERS review requests in general. Maybe I should remove myself. I want to be notified but not requested to review.

[AlexWaygood]

I want to be notified but not requested to review.

There's no good mechanism on GitHub to be automatically subscribed to PRs but not be auto-requested for review, unfortunately :( Though you can always manually subscribe to a PR thread.

You're already subscribed to this thread, so I'll go ahead and remove your request for review on this for now :)

[AlexWaygood]

Hope you're having a great vacation!

[carljm]

This is the sort of tradeoff that is hard to evaluate without checking the performance impact on some realistic code bases using runtime-checkable Protocols. I have no idea how common it is in real code to explicitly subclass or register-as-implementing a Protocol, or why anyone would bother. I guess if we implement this PR, then that would be available as a performance optimization?

It makes intuitive sense to do this, and I would probably lean towards doing it if the impact on performance of the non-explicit case is small; like <1-2%? (Sorry, don't have time right now to actually run benchmarks.)

[AlexWaygood]

I would probably lean towards doing it if the impact on performance of the non-explicit case is small; like <1-2%?

Looks like it would be a performance degradation somewhere in the range of between 5-10% rather than 1-2% for the non-explicit cases :/

I'm hesitant to be any more precise; as I say, the numbers on my machine seem pretty noisy for some reason

[AlexWaygood]

I guess if we implement this PR, then that would be available as a performance optimization?

Yeah, if we do this, then nominal isinstance() checks become very fast compared to all other kinds of isinstance() checks. isinstance() checks via ABCMeta.register are still slower than all other kinds of isinstance() checks with this PR, but significantly faster than they are on main.

(Not quite sure why isinstance() checks using ABCMeta.register are so apparently slow; that seems strange.)

[AlexWaygood]

Looks like it would be a performance degradation somewhere in the range of between 5-10% rather than 1-2% for the non-explicit cases :/

The performance hit for the non-explicit cases has gone down to somewhere in the range of between 3-8% now that #103034 has been merged (because both main and this PR branch are now slower than they were). The performance boost to the explicit cases is now even more pronounced.

[AlexWaygood]

I suddenly realised that, by moving the if super().__instancecheck__() call higher up the function, we could remove a lot of the logic lower down the function while maintaining all invariants. A bunch of the logic lower down the function was purely to provide short-circuiting for subtypes where we wouldn't need to iterate over the result of _get_protocol_attrs. But now we have a catch-all short-circuit at the top of the function, this logic is no longer needed!

This PR now removes that logic and, as a result, all kinds of isinstance() checks are now faster, not just isinstance() checks against nominal and registered subtypes! Plus, the code is simpler than it used to be!

Benchmark results on main:

Time taken for objects with a property: 3.24
Time taken for objects with a classvar: 3.21
Time taken for objects with an instance var: 12.35
Time taken for objects with no var: 15.57
Time taken for nominal subclass instances: 25.28
Time taken for registered subclass instances: 21.54

And with this PR:

Time taken for objects with a property: 2.96
Time taken for objects with a classvar: 2.84
Time taken for objects with an instance var: 9.50
Time taken for objects with no var: 14.04
Time taken for nominal subclass instances: 0.63
Time taken for registered subclass instances: 6.87

[carljm]

all kinds of isinstance() checks are now faster

Well, that does make it easier to evaluate the tradeoff ;)

It's odd to me that the function was written this way in the first place. It seems like someone took some care to ensure that being a true subclass was only sufficient if not a protocol class or if callable-members-only. But it also seems clear that this doesn't change semantics; there's no other return False in the function, so there's no way in which moving super().__instancecheck__(instance) higher up in the function can cause to return True for true subclasses when we wouldn't have before.

[AlexWaygood]

It's odd to me that the function was written this way in the first place. It seems like someone took some care to ensure that being a true subclass was only sufficient if not a protocol class or if callable-members-only. But it also seems clear that this doesn't change semantics; there's no other return False in the function, so there's no way in which moving super().__instancecheck__(instance) higher up in the function can cause to return True for true subclasses when we wouldn't have before.

It's possible that the way it was written used to provide an optimisation. The super().__instancecheck__() call is actually pretty heavy here:

• super().__instancecheck__() delegates to ABCMeta.__instancecheck__(), which does something along these lines:

  class ABCMeta:
      def __instancecheck__(cls, instance):
          return issubclass(type(instance), cls)

• The issubclass call there calls ABCMeta.__subclasscheck__, which does something along these lines:

  class ABCMeta:
      def __subclasscheck__(cls, other):
          subclasshook_result = cls.__subclasshook__(other)
          if subclasshook_result is not NotImplemented:
              return subclasshook_result
          return super().__subclasscheck__(other)

• And the cls.__subclasshook__() call there means that we end up going through this method:

  cpython/Lib/typing.py

  Lines 2087 to 2126 in 47753ec

  	def _proto_hook(other):
  	if not cls.__dict__.get('_is_protocol', False):
  	return NotImplemented
  	# First, perform various sanity checks.
  	if not getattr(cls, '_is_runtime_protocol', False):
  	if _allow_reckless_class_checks():
  	return NotImplemented
  	raise TypeError("Instance and class checks can only be used with"
  	" @runtime_checkable protocols")
  	protocol_attrs = _get_protocol_attrs(cls)
  	if not _is_callable_members_only(cls, protocol_attrs):
  	if _allow_reckless_class_checks():
  	return NotImplemented
  	raise TypeError("Protocols with non-method members"
  	" don't support issubclass()")
  	if not isinstance(other, type):
  	# Same error message as for issubclass(1, int).
  	raise TypeError('issubclass() arg 1 must be a class')
  	# Second, perform the actual structural compatibility check.
  	for attr in protocol_attrs:
  	for base in other.__mro__:
  	# Check if the members appears in the class dictionary...
  	if attr in base.__dict__:
  	if base.__dict__[attr] is None:
  	return NotImplemented
  	break
  	# ...or in annotations, if it is a sub-protocol.
  	annotations = getattr(base, '__annotations__', {})
  	if (isinstance(annotations, collections.abc.Mapping) and
  	attr in annotations and
  	issubclass(other, Generic) and other._is_protocol):
  	break
  	else:
  	return NotImplemented
  	return True

• Because that method is monkey-patched onto Protocol classes to become the new __subclasshook__ method here:

  cpython/Lib/typing.py

  Lines 2128 to 2129 in 47753ec

  	if '__subclasshook__' not in cls.__dict__:
  	cls.__subclasshook__ = _proto_hook

All of which is to say: I definitely wouldn't be surprised if delaying the super().__instancecheck__() call to the last possible moment used to provide an optimisation, even if it no longer does.