GeezerVison

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

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! :)

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.

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 :)

Hope you're having a great vacation!

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.)

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

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.)

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.

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

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.