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: Improve performance when testing against protocols with many attributes. by randolf-scholz · Pull Request #112157 · python/cpython

Conversation

@randolf-scholz

@randolf-scholz randolf-scholz commented Nov 16, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This adds a very simple check for detecting true negatives:

# quick true negative check for protocols with many attributes.
if not hasattr(instance, '__getattr__'):
    if not (cls.__protocol_attrs__ <= set(dir(instance))):
        return False

Running some benchmarks with pyperf against the example provided here: #74690 (comment) give the following speedups:

Benchmark baseline baseline_repro test
one_int 34.9 us not significant 12.8 us: 2.73x faster
two_frac 505 ns 501 ns: 1.01x faster 502 ns: 1.01x faster
three_float 39.2 us not significant 10.7 us: 3.67x faster
Geometric mean (ref) 1.05x slower 2.16x faster

one_int and three_float are negative examples, and we see a 2.7x-3.7x speedup, but no measurable slowdown for the true positive two_frac.

Are there any other benchmarks to test against?

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

All reactions
@randolf-scholz
added fast check in _ProtocolMeta.__instancecheck__
9234769
@randolf-scholz
randolf-scholz requested review from AlexWaygood, JelleZijlstra and gvanrossum as code owners November 16, 2023 13:31
@ghost

ghost commented Nov 16, 2023 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot added the awaiting review label Nov 16, 2023
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot mentioned this pull request Nov 16, 2023
Performance of typing._ProtocolMeta._get_protocol_attrs and isinstance #74690
Closed
AlexWaygood
AlexWaygood reviewed Nov 16, 2023
View reviewed changes
Comment thread Lib/typing.py

# quick true negative check for protocols with many attributes.
if not hasattr(instance, '__getattr__'):
if not (cls.__protocol_attrs__ <= set(dir(instance))):

@AlexWaygood AlexWaygood Nov 16, 2023 •
edited
Loading

Uh oh!

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

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.

What about classes that define custom __dir__() methods? What about classes that define custom __getattribute__ methods? I think this approach is very risky

Sorry, something went wrong.

Uh oh!

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

All reactions

@randolf-scholz randolf-scholz Nov 16, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

Well, is there any C-implemented alternative to dir, that returns some iterable of the attribute names? I saw inspect.getmembers_static, but it is mostly plain python and way too slow. If custom __dir__ is a worry, one could maybe use object.__dir__(instance).

dir itself is even inefficient since it sorts the names, here we really only need something that iterates over the names.

Sorry, something went wrong.

Uh oh!

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

All reactions

@randolf-scholz randolf-scholz Nov 16, 2023

Copy link
Copy Markdown
Contributor 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.

In regard to the custom __getattribute__ stuff, I think the current code already suffers from that, no? The docs say about getattr_static:

Note: this function may not be able to retrieve all attributes that getattr can fetch (like dynamically created attributes) and may find attributes that getattr can’t (like descriptors that raise AttributeError).

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Nov 16, 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 there any C-implemented alternative to dir, that returns some iterable of the attribute names?

Not that I know of

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Nov 16, 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.

In regard to the custom __getattribute__ stuff, I think the current code already suffers from that, no?

Ah indeed; good point

Sorry, something went wrong.

Uh oh!

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

All reactions

@randolf-scholz randolf-scholz Nov 16, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

So really the issue here seems that

  1. getattr_static is just really slow (I re-ran the test, replacing it with plain getattr and got 11x speedup)
    • getattr can be slow if it triggers some computation.
  2. isinstance(x, proto) only checks statically known attributes and can give incorrect results if certain attributes are defined dynamically.
    • since Protocols are about static typing, this seems ok to do.
  3. There is nothing in the data-model for classes to provide something like a __hasattr__ to communicate which attributes are set dynamically.

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Nov 16, 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.

Your analysis is correct.

  • getattr_static is just really slow (I re-ran the test, replacing it with plain getattr and got 11x speedup)

    • getattr can be slow if it triggers some computation.
  • isinstance(x, proto) only checks statically known attributes and can give incorrect results if certain attributes are defined dynamically.

Note that we changed these two bullet points in Python 3.12 after long discussions in python/typing#1363 and #102433. Previously, isinstance() checks didn't use inspect.getattr_static, and did resolve dynamically defined attributes. But it was decided that that behaviour was bad, so we decided to accept the performance penalty in order to have more correct and less unexpected behaviour. (And through various other performance optimisations to compensate for that, isinstance() checks against runtime-checkable protocols are still overall much faster in Python 3.12 than in Python 3.11, despite the slowdown from switching to inspect.getattr_static.)

Sorry, something went wrong.

Uh oh!

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

All reactions
@randolf-scholz

randolf-scholz commented Nov 16, 2023

Copy link
Copy Markdown
Contributor Author

Wait for dir_static (#55979) and see if it still improves performance.

All reactions

Sorry, something went wrong.

Uh oh!

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

@randolf-scholz randolf-scholz closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

2 participants

@randolf-scholz @AlexWaygood
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.