GeezerVison

So for any given class klass, _shadowed_dict(klass) should always return the same result.

The __dict__ can't change, but the __mro__ can :/

>>> class A: pass
...
>>> class B: pass
...
>>> class C(A): pass
...
>>> C.__mro__
(<class '__main__.C'>, <class '__main__.A'>, <class 'object'>)
>>> C.__bases__ = (B,)
>>> C.__mro__
(<class '__main__.C'>, <class '__main__.B'>, <class 'object'>)

So I don't think this is safe.

The __dict__ can't change, but the __mro__ can :/

Well, good that we're discovering holes in getattr_static test coverage :)

What about something like this, where _shadowed_dict is only cached for the lifetime of a single getattr_static call? It's slower than this patch currently, as it turns out that the call to .cache_clear() is pretty expensive. But it's still noticeably faster than main:

# add @lru_cache() to _shadowed_dict, like in this PR currently

def getattr_static(obj, attr, default=_sentinel):
    try:
        # existing getattr_static implementation here
    finally:
        _shadowed_dict.cache_clear()

How does the performance comparison look if you make _shadowed_dict take the mro tuple rather than the class? I.e. call sites would turn into _shadowed_dict(_static_getmro(klass)).

I think if you did that, then it would be safe to put an LRU cache on _shadowed_dict.

I think it should be a bounded cache, though, not unbounded.

The pathological scenario would be someone creating lots of transient classes inside a function, and calling getattr_static on instances of them. It seems prettu unlikely that someone is doing all of that, and expecting the transient classes to be GCed. But I think we at least should ensure the memory use in such a case is bounded.

I think it should be a bounded cache, though, not unbounded.

@functools.lru_cache() creates a cache with size 128 by default, so my PR already proposes a bounded cache ;)

>>> functools.lru_cache(lambda: 1).cache_info()
CacheInfo(hits=0, misses=0, maxsize=128, currsize=0)

How does the performance comparison look if you make _shadowed_dict take the mro tuple rather than the class? I.e. call sites would turn into _shadowed_dict(_static_getmro(klass)).

It's not as fast as this PR, but it's a lot faster than main. I'll switch to that approach.

Here's the new performance numbers using the getattr_static benchmark:

type[Foo]                :   61 ±  0 ns
Foo                      :  100 ±  0 ns
type[Bar]                :   61 ±  0 ns
Bar                      :  100 ±  0 ns
WithParentClassX         :  129 ±  0 ns
Baz                      :  118 ±  0 ns
WithParentX              :  146 ±  0 ns
type[Missing]            :  148 ±  0 ns
Missing                  :  126 ±  1 ns
Slotted                  :  162 ±  2 ns
Method                   :  105 ±  0 ns
StMethod                 :  106 ±  0 ns
ClsMethod                :  100 ±  0 ns

The same benchmark on main:

type[Foo]                :   90 ±  0 ns
Foo                      :  163 ±  0 ns
type[Bar]                :   90 ±  0 ns
Bar                      :  162 ±  0 ns
WithParentClassX         :  229 ±  0 ns
Baz                      :  209 ±  0 ns
WithParentX              :  276 ±  0 ns
type[Missing]            :  258 ±  0 ns
Missing                  :  213 ±  0 ns
Slotted                  :  203 ±  0 ns
Method                   :  162 ±  0 ns
StMethod                 :  163 ±  0 ns
ClsMethod                :  163 ±  0 ns

With this PR now, isinstance() checks against runtime-checkable protocols are faster than they were on 3.11 as long as the protocol has 13 members or fewer. On main, they are only faster than on 3.11 with 6 members or fewer.

On main, the results of the typing_runtime_protocols pyperformance benchmark are 262 us +- 11 us. With this PR, they are 178 us +- 9 us.

(Skipping news because, if this patch is correct, it shouldn't change behaviour at all other than improving performance. And the news entry in https://github.com/python/cpython/pull/103195/files should already cover the performance boost.)

Thanks @carljm :D

I listed you as a co-author on the commit; I don't think I would have thought of using the mro tuple as a cache key on my own :)