Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygoodAlexWaygood commented Apr 6, 2023

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:

fromtypingimport*@runtime_checkableclassFoo(Protocol): a: intb: intc: intd: inte: intf: intg: inth: inti: intj: intclassBar: def__init__(self): forattrnamein'abcdefghij': setattr(self, attrname, 42) isinstance(Bar(), Foo)

@AlexWaygoodAlexWaygood 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
Copy link
MemberAuthor

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

Copy link
Member

@carljmcarljm 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
MemberAuthor

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
MemberAuthor

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

@AlexWaygood
Copy link
MemberAuthor

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

Copy link
Member

@sobolevnsobolevn 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
MemberAuthor

Thanks both!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.12only security fixesperformancePerformance or resource usageskip newsstdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@AlexWaygood@carljm@sobolevn@bedevere-bot