Skip to content
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: Simplify and optimise _ProtocolMeta.__instancecheck__ #103159

Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 31, 2023

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

@gvanrossum
Copy link
Member

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

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 31, 2023

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
Copy link
Member

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
Copy link
Member Author

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 AlexWaygood removed the request for review from gvanrossum March 31, 2023 23:54
@AlexWaygood
Copy link
Member Author

Hope you're having a great vacation!

@carljm
Copy link
Member

carljm commented Apr 1, 2023

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
Copy link
Member Author

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
Copy link
Member Author

AlexWaygood commented Apr 1, 2023

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
Copy link
Member Author

AlexWaygood commented Apr 2, 2023

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
Copy link
Member Author

AlexWaygood commented Apr 3, 2023

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

@AlexWaygood AlexWaygood changed the title gh-74690: typing: Optimise nominal subtypes in _ProtocolMeta.__instancecheck__ gh-74690: typing: Simplify and optimise _ProtocolMeta.__instancecheck__ Apr 3, 2023
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AlexWaygood merged commit 47753ec into python:main Apr 5, 2023
@AlexWaygood AlexWaygood deleted the runtime-protocols-nominal-optimisation branch April 5, 2023 09:07
@bedevere-bot

This comment was marked as off-topic.

@AlexWaygood
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes performance Performance or resource usage skip news stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants