8000 gh-102433: Use `inspect.getattr_static` in `typing._ProtocolMeta.__instancecheck__` by AlexWaygood · Pull Request #103034 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-102433: Use inspect.getattr_static in typing._ProtocolMeta.__instancecheck__ #103034

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Apr 2, 2023
Merged

gh-102433: Use inspect.getattr_static in typing._ProtocolMeta.__instancecheck__ #103034

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
212fce2
Implementation
AlexWaygood Mar 24, 2023
ef71395
Tests
AlexWaygood Mar 24, 2023
4407d60
docs
AlexWaygood Mar 25, 2023
44eba6b
Wrong issue number
AlexWaygood Mar 25, 2023
9989eea
Merge branch 'main' into runtime-protocols-getattr_static
AlexWaygood Mar 27, 2023
102772c
Merge remote-tracking branch 'origin/main' into runtime-protocols-get…
AlexWaygood Mar 31, 2023
5c15fbc
Fix an edge case
AlexWaygood Mar 31, 2023
b0a9dd1
Merge branch 'runtime-protocols-getattr_static' of https://github.com…
AlexWaygood Mar 31, 2023
0812183
Move tests around, add some more
AlexWaygood Mar 31, 2023
83d7ec2
Merge branch 'main' into runtime-protocols-getattr_static
AlexWaygood Apr 2, 2023
6ba97e7
Simplify and optimise
AlexWaygood Apr 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Implementation
  • Loading branch information
@AlexWaygood
AlexWaygood committed Mar 24, 2023
commit 212fce296424db5ea809d0fcadd31e47dc3b9888
16 changes: 15 additions & 1 deletion Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1994,6 +1994,17 @@ def _allow_reckless_class_checks(depth=3):
}


@functools.cache
def _lazy_load_getattr_static():
# Import getattr_static lazily so as not to slow down the import of typing.py
# Cache the result so we don't slow down _ProtocolMeta.__instancecheck__ unnecessarily
from inspect import getattr_static
Copy link
Member

Choose a reason for hiding this comment

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

Is this really worth it? inspect is likely already imported by a lot of programs (e.g. anything that uses dataclasses or asyncio), so is it really so bad to import it here?

Relatedly, my intuition was that the @cache wouldn't help much because the import is already cached, but in fact the cache makes it 20x faster on my computer: I guess there's still a lot of overhead in going through the import system.

In [5]: %timeit func()
458 ns ± 6.23 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [6]: %timeit func2()
22.7 ns ± 0.0575 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

Copy link
Member Author
@AlexWaygood AlexWaygood Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this really worth it?

I think it is worth it, yes:

  • inspect is a pretty heavy import; the impact of adding an inspect import at the top of typing.py was clearly measurable in microbenchmarks for measuring the import time of typing.
  • I believe importing typing used to be very slow, but that a lot of work was done a few years ago to improve the import time by e.g. reducing the number of metaclasses. I don't want to undermine that work now.
  • We also saw a lot of work done for Python 3.11 to improve startup times for Python in general (via the introduction of e.g. the deepfreeze machinery), so that it could be a more suitable language for small CLI tools, where startup time is very important. I don't want CLI-tool authors who care about performance to have to go to pains to avoid importing typing.

inspect is likely already imported by a lot of programs (e.g. anything that uses dataclasses or asyncio), so is it really so bad to import it here?

I don't think this is a great argument. asyncio and dataclasses might import inspect now, but that doesn't mean that they'll necessarily continue to do so indefinitely. If somebody decides to try to improve import the import time of dataclasses by moving to a lazy import of inspect (for example), we'd be left looking pretty silly by making a decision on the basis that "dataclasses imports inspect, so it's okay for us to do it too". Besides, there's lots of code that doesn't make any use of dataclasses or asyncio.

Relatedly, my intuition was that the @cache wouldn't help much because the import is already cached, but in fact the cache makes it 20x faster on my computer: I guess there's still a lot of overhead in going through the import system.

Yes, I was also surprised at how much the @cache sped things up here :)

return getattr_static


_cleanups.append(_lazy_load_getattr_static.cache_clear)


class _ProtocolMeta(ABCMeta):
# This metaclass is really unfortunate and exists only because of
# the lack of __instancehook__.
Expand All @@ -2013,7 +2024,10 @@ def __instancecheck__(cls, instance):
issubclass(instance.__class__, cls)):
return True
if cls._is_protocol:
if all(hasattr(instance, attr) and
sentinel = object()
getattr_static = _lazy_load_getattr_static()
if all(
(getattr_static(instance, attr, sentinel) is not sentinel) and
# All *methods* can be blocked by setting them to None.
(not callable(getattr(cls, attr, None)) or
getattr(instance, attr) is not None)
Expand Down
0