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-103193: Use LBYL idioms rather than EAFP in `inspect.getattr_static` by AlexWaygood · Pull Request #103318 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Apr 6, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This PR significantly speeds up inspect.getattr_static() and, as a result, isinstance() checks against runtime-checkable protocols.

Here are benchmark results on main using @sobolevn's benchmark script from #103193 (comment):

type[Foo]                :   89 ±  0 ns
Foo                      :  212 ±  0 ns
type[Bar]                :   90 ±  0 ns
Bar                      :  213 ±  0 ns
WithParentClassX         :  331 ±  0 ns
Baz                      :  325 ±  1 ns
WithParentX              :  441 ±  1 ns
type[Missing]            :  422 ±  1 ns
Missing                  :  328 ±  1 ns
Slotted                  :  267 ±  0 ns
Method                   :  213 ±  0 ns
StMethod                 :  214 ±  0 ns
ClsMethod                :  213 ±  0 ns

And here are the benchmark results with this PR:

type[Foo]                :   75 ±  0 ns
Foo                      :  179 ±  0 ns
type[Bar]                :   75 ±  0 ns
Bar                      :  179 ±  0 ns
WithParentClassX         :  232 ±  1 ns
Baz                      :  216 ±  0 ns
WithParentX              :  265 ±  0 ns
type[Missing]            :  209 ±  0 ns
Missing                  :  218 ±  0 ns
Slotted                  :  214 ±  0 ns
Method                   :  178 ±  0 ns
StMethod                 :  178 ±  0 ns
ClsMethod                :  178 ±  0 ns

A result of this optimisation is that the following isinstance() call becomes around 1.5x faster:

from typing import *

@runtime_checkable
class Foo(Protocol):
    a: int
    b: int
    c: int
    d: int
    e: int
    f: int
    g: int
    h: int
    i: int
    j: int

class Bar:
    def __init__(self):
        for attrname in 'abcdefghij':
            setattr(self, attrname, 42)

isinstance(Bar(), Foo)
  • Issue: Optimise inspect.getattr_static #103193

Sorry, something went wrong.

Uh oh!

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

All reactions
@AlexWaygood
pythongh-103193: Use LBYL idioms rather than EAFP in `inspect.getattr…
972db3a
…_static`
@AlexWaygood AlexWaygood added type-feature A feature request or enhancement performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory 3.12 only security fixes labels Apr 6, 2023
@AlexWaygood
AlexWaygood requested review from carljm and sobolevn April 6, 2023 18:25
@bedevere-bot bedevere-bot mentioned this pull request Apr 6, 2023
Optimise inspect.getattr_static #103193
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Apr 6, 2023
@AlexWaygood AlexWaygood added the skip news label Apr 6, 2023
@AlexWaygood

AlexWaygood commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

Skipping news, since the news entry in #103195 should suffice, I think.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm
carljm approved these changes Apr 6, 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.

It's less idiomatic code, but I think it's worth it for the very significant performance improvement on runtime protocol isinstance checking with many attributes.

I assume you've checked the performance of using .get(...) and it's worse?

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
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 6, 2023
@AlexWaygood

AlexWaygood commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

I assume you've checked the performance of using .get(...) and it's worse?

Hmm I haven't, actually -- not sure how you'd use .get() here in an idiomatic way? Can you show me what you mean?

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood

AlexWaygood commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

Oh I think I see what you mean. Trying it out now.

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood

AlexWaygood commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

Yeah, I tried out doing this instead (diff is relative to my current patch, not to main), and it's slower than what I currently have:

diff --git a/Lib/inspect.py b/Lib/inspect.py
index a317f0ca74..7fcaa13750 100644
--- a/Lib/inspect.py
+++ b/Lib/inspect.py
@@ -1787,8 +1787,10 @@ def _check_instance(obj, attr):

 def _check_class(klass, attr):
     for entry in _static_getmro(klass):
-        if _shadowed_dict(type(entry)) is _sentinel and attr in entry.__dict__:
-            return entry.__dict__[attr]
+        if _shadowed_dict(type(entry)) is _sentinel:
+            ret = entry.__dict__.get(attr, _sentinel)
+            if ret is not _sentinel:
+                return ret
     return _sentinel

 def _is_type(obj):
@@ -1800,13 +1802,13 @@ def _is_type(obj):

 def _shadowed_dict(klass):
     for entry in _static_getmro(klass):
-        dunder_dict = _get_dunder_dict_of_class(entry)
-        if '__dict__' in dunder_dict:
-            class_dict = dunder_dict['__dict__']
-            if not (type(class_dict) is types.GetSetDescriptorType and
-                    class_dict.__name__ == "__dict__" and
-                    class_dict.__objclass__ is entry):
-                return class_dict
+        class_dict = _get_dunder_dict_of_class(entry).get('__dict__', _sentinel)
+        if class_dict is not _sentinel and not (
+            type(class_dict) is types.GetSetDescriptorType
+            and class_dict.__name__ == "__dict__"
+            and class_dict.__objclass__ is entry
+        ):
+            return class_dict
     return _sentinel

 def getattr_static(obj, attr, default=_sentinel):
@@ -1845,11 +1847,10 @@ def getattr_static(obj, attr, default=_sentinel):
     if obj is klass:
         # for types we check the metaclass too
         for entry in _static_getmro(type(klass)):
-            if (
-                _shadowed_dict(type(entry)) is _sentinel
-                and attr in entry.__dict__
-            ):
-                return entry.__dict__[attr]
+            if _shadowed_dict(type(entry)) is _sentinel:
+                ret = entry.__dict__.get(attr, _sentinel)
+                if ret is not _sentinel:
+                    return ret

@carljm is that the kind of thing you were talking about? :)

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

sobolevn
sobolevn approved these changes Apr 6, 2023
View reviewed changes

@sobolevn sobolevn 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.

Good idea!

Sorry, something went wrong.

Uh oh!

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

All reactions
@AlexWaygood

AlexWaygood commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

Thanks both!

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood
AlexWaygood merged commit affedee into python:main Apr 6, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 6, 2023
@AlexWaygood
AlexWaygood deleted the getattr_static-optimisation branch April 6, 2023 19:17
This was referenced Apr 6, 2023
Performance of typing._ProtocolMeta._get_protocol_attrs and isinstance #74690
Closed
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-103193: Use LBYL idioms rather than EAFP in `inspect.getattr…
a4e49a4
…_static` (python#103318)
@AlexWaygood AlexWaygood mentioned this pull request Jul 12, 2023
Reduce overhead to add/remove asyncio readers and writers #106527
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
@sobolevn sobolevn sobolevn approved these changes

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 stdlib Standard Library Python modules in the Lib/ directory 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.

4 participants

@AlexWaygood @carljm @sobolevn @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.