GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-74690: typing: Don't unnecessarily call `_get_protocol_attrs` twice in `_ProtocolMeta.__instancecheck__` by AlexWaygood · Pull Request #103141 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Mar 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This simple optimisation achieves speedups for all kinds of isinstance() checks against runtime-checkable protocols. For structural subtypes that set attributes in __init__ methods, the speedup is dramatic.

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

Results on 01a49d1 (non-debug build, PGO-optimised):

Time taken for objects with a property: 3.63
Time taken for objects with a classvar: 3.82
Time taken for objects with an instance var: 12.22
Time taken for objects with no var: 13.73
Time taken for nominal subclass instances: 12.46
Time taken for registered subclass instances: 20.62

Results with this PR:

Time taken for objects with a property: 2.10
Time taken for objects with a classvar: 2.07
Time taken for objects with an instance var: 2.06
Time taken for objects with no var: 6.94
Time taken for nominal subclass instances: 7.54
Time taken for registered subclass instances: 16.06

(We may want to wait until #103034 is merged and then re-measure.)

Cc. @leycec and @posita, as people who I know care a lot about Protocol performance :)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner and carljm reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@AlexWaygood
pythongh-74690: typing: Don't unnecessarily call `_get_protocol_attrs…
51ca233 
…` twice in `_ProtocolMeta.__instancecheck__`
@AlexWaygood AlexWaygood added type-feature A feature request or enhancement performance Performance or resource usage topic-typing 3.12 only security fixes labels Mar 30, 2023
@AlexWaygood
AlexWaygood requested a review from carljm March 30, 2023 23:00
@AlexWaygood
AlexWaygood requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners March 30, 2023 23:00
@bedevere-bot bedevere-bot mentioned this pull request Mar 30, 2023
Performance of typing._ProtocolMeta._get_protocol_attrs and isinstance #74690
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Mar 30, 2023
JelleZijlstra
JelleZijlstra reviewed Mar 30, 2023
View reviewed changes
Comment thread Lib/typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Address review
285a41b
@AlexWaygood
AlexWaygood requested a review from JelleZijlstra March 31, 2023 09:33
AlexWaygood
AlexWaygood commented Mar 31, 2023
View reviewed changes
Comment thread Lib/typing.py
return True
if cls._is_protocol:

protocol_attrs = _get_protocol_attrs(cls)

@AlexWaygood AlexWaygood Mar 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could possibly cache the result of this call on the protocol class in a __protocol_attrs__ class attribute. But I think I'd rather look at that in a separate PR so that we can evaluate the impact of it in isolation. I'd also want to check to see if there were any behaviour changes, and compare different methods of caching the result of _get_protocol_attrs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@posita posita Mar 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could possibly cache the result of this call on the protocol class in a __protocol_attrs__ class attribute. ...

If you decide to tackle this, @beartype provides a caching mechanism for __instancecheck__ results that may be worth a look, if only for inspiration. I believe some of the same caveats would apply. (Full disclosure: I am an author. Happy to chat, if helpful.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AlexWaygood AlexWaygood Mar 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I share @ilevkivskyi's reservations in #74690 (comment) about caching the whole call to __instancecheck__ in the way that I believe beartype does it, but I think caching the call to _get_protocol_attrs could get us a large speedup without the same risk of changing the behaviour. (Apologies if I was unclear in my comment above!) Anyway, TBD in a future PR :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
posita reacted with heart emoji
All reactions
  • ❤️ 1 reaction

@carljm carljm Mar 31, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is monkey-patching of the protocol class itself something we need to care about here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AlexWaygood AlexWaygood Mar 31, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is monkey-patching of the protocol class itself something we need to care about here?

Pretty unlikely imo. But yeah, it's that kind of thing that I'd want to think through, which is why I'd probably like to leave that to another PR :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@AlexWaygood

AlexWaygood commented Mar 31, 2023

Copy link
Copy Markdown
Member Author

Some local experimentation (by combining the two branches) indicates that this PR doesn't quite make up for the performance hit from #103034, but does significantly mitigate it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
carljm
carljm approved these changes Mar 31, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change LGTM. Nice find!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Mar 31, 2023
@AlexWaygood

AlexWaygood commented Mar 31, 2023

Copy link
Copy Markdown
Member Author

I discussed with @carljm offline, and we decided it's probably better to skip news for this one. On its own it's a performance improvement, but it'll likely be accompanied by #103034(which hurts performance of runtime-checkable protocols), and possibly one or two other optimisations to this code that I'm eyeing up. So the overall impact on performance for runtime-checkable protocols is pretty uncertain at the moment, which would mean including a news entry at this stage would probably just cause confusion to readers of the changelog.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood added the skip news label Mar 31, 2023
AlexWaygood added 2 commits March 31, 2023 18:06
@AlexWaygood
Delete 2023-03-30-23-51-22.gh-issue-74690.p2c4Co.rst
b6d1c95
@AlexWaygood
Merge branch 'main' into protocol-attrs-collection
0f498a9
ilevkivskyi
ilevkivskyi approved these changes Mar 31, 2023
View reviewed changes
@AlexWaygood
AlexWaygood merged commit 9048d73 into python:main Mar 31, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 31, 2023
@AlexWaygood
AlexWaygood deleted the protocol-attrs-collection branch March 31, 2023 17:37
@hauntsaninja

hauntsaninja commented Mar 31, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Nice find! For what it's worth, I've found users often depend on undocumented aspects of typing.py, so I'd have still included a vague news entry on the lines of "Changed the performance characteristics of _ProtocolMeta.__instancecheck__ by avoiding a redundant call to _get_protocol_attrs" (i.e. make clear something changed but not promise any improvement)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Mar 31, 2023

Copy link
Copy Markdown
Member Author

I've found users often depend on undocumented aspects of typing.py

Speaking from experience? ;)
hauntsaninja reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood mentioned this pull request Mar 31, 2023
gh-74690: Micro-optimise typing._get_protocol_attrs #103152
Merged
This was referenced Apr 2, 2023
gh-102433: Use inspect.getattr_static in typing._ProtocolMeta.__instancecheck__ #103034
Merged
gh-74690: Document changes made to runtime-checkable protocols in 3.12 #103348
Merged
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@AlexWaygood @warsaw
pythongh-74690: typing: Don't unnecessarily call `_get_protocol_attrs…
54698ca 
…` twice in `_ProtocolMeta.__instancecheck__` (python#103141)

Speed up `isinstance()` calls against runtime-checkable protocols
@AlexWaygood AlexWaygood mentioned this pull request Apr 12, 2023
Backport performance improvements to runtime-checkable protocols python/typing_extensions#137
Merged
@AlexWaygood AlexWaygood mentioned this pull request May 24, 2023
[Bug] typing_extensions.Protocol unsupported under typing_extensions ≥ 4.6.0 beartype/beartype#241
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes
@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner
+1 more reviewer
@posita posita posita left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

3.12 only security fixes performance Performance or resource usage skip news topic-typing type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@AlexWaygood @hauntsaninja @carljm @posita @JelleZijlstra @ilevkivskyi @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.