gh-103193: Use LBYL idioms rather than EAFP in `inspect.getattr_static` by AlexWaygood · Pull Request #103318 · python/cpython · GitHub
Skip to content

gh-103193: Use LBYL idioms rather than EAFP in inspect.getattr_static#103318

Merged
AlexWaygood merged 1 commit intopython:mainfrom
AlexWaygood:getattr_static-optimisation
Apr 6, 2023
Merged

gh-103193: Use LBYL idioms rather than EAFP in inspect.getattr_static#103318
AlexWaygood merged 1 commit intopython:mainfrom
AlexWaygood:getattr_static-optimisation

Conversation

@AlexWaygood
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood commented Apr 6, 2023 •
edited
Loading

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)

@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
Closed
@AlexWaygood
Copy link
Copy Markdown
Member Author

AlexWaygood commented Apr 6, 2023

carljm
carljm approved these changes Apr 6, 2023
View reviewed changes
Copy link
Copy Markdown
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.

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?

@AlexWaygood
Copy link
Copy Markdown
Member Author

AlexWaygood commented Apr 6, 2023

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?

@AlexWaygood
Copy link
Copy Markdown
Member Author

AlexWaygood commented Apr 6, 2023

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

@AlexWaygood
Copy link
Copy Markdown
Member Author

AlexWaygood commented Apr 6, 2023

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? :)

sobolevn
sobolevn approved these changes Apr 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Good idea!

@AlexWaygood
Copy link
Copy Markdown
Member Author

AlexWaygood commented Apr 6, 2023

@AlexWaygood AlexWaygood merged commit affedee into python:main Apr 6, 2023
@AlexWaygood AlexWaygood deleted the getattr_static-optimisation branch April 6, 2023 19:17
This was referenced Apr 6, 2023
Closed
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
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

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.

4 participants

@AlexWaygood @carljm @sobolevn @bedevere-bot