gh-103193: cache calls to inspect._shadowed_dict in inspect.getattr_static

[AlexWaygood]

EDIT: These performance numbers are out of date with the current PR; read further down the thread for the up-to-date numbers.

This dramatically speeds up calls to inspect.getattr_static.

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

type[Foo]                :   88 ±  0 ns
Foo                      :  158 ±  0 ns
type[Bar]                :   89 ±  0 ns
Bar                      :  158 ±  0 ns
WithParentClassX         :  224 ±  0 ns
Baz                      :  205 ±  0 ns
WithParentX              :  271 ±  1 ns
type[Missing]            :  252 ±  0 ns
Missing                  :  207 ±  0 ns
Slotted                  :  200 ±  1 ns
Method                   :  160 ±  1 ns
StMethod                 :  158 ±  0 ns
ClsMethod                :  158 ±  0 ns

And here are results with this PR:

type[Foo]                :   54 ±  0 ns
Foo                      :   85 ±  0 ns
type[Bar]                :   53 ±  0 ns
Bar                      :   85 ±  0 ns
WithParentClassX         :  102 ±  0 ns
Baz                      :   96 ±  0 ns
WithParentX              :  113 ±  0 ns
type[Missing]            :  110 ±  0 ns
Missing                  :   98 ±  0 ns
Slotted                  :  137 ±  0 ns
Method                   :   85 ±  0 ns
StMethod                 :   85 ±  0 ns
ClsMethod                :   85 ±  0 ns

With this PR, inspect.getattr_static is fast enough that even isinstance() calls like this, with "pathological" runtime-checkable protocols, are faster than they were on Python 3.11:

Pathological protocol

from typing import Protocol, runtime_checkable

@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
    k: int
    l: int
    m: int
    n: int
    o: int
    p: int
    q: int
    r: int
    s: int
    t: int
    u: int
    v: int
    w: int
    x: int
    y: int
    z: int

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

isinstance(Bar(), Foo)

This approach makes me a little nervous. There are two plausible reasons I thought of why adding a cache here might not be a good idea:

1. It could cause references to the klass argument to be held by the cache even after they've been deleted elsewhere, which could be unexpected behaviour for a low-level function like getattr_static
2. Perhaps it's possible that whether or not a __dict__ attribute is shadowed could change at some point during the lifetime of a class.

However, I think we're okay on both points.

For objection (1): _shadowed_dict is only ever called on type objects. The vast majority of classes are defined once in the global namespace and never deleted, so it shouldn't be an issue that the cache is holding strong references to the type objects. (If we do think this is an issue, I also experimented with a version of this PR that uses a weakref.WeakKeyDictionary as a cache. It also sped things up, but not by nearly as much.)

For objection (2): It doesn't seem to be possible, once a class has been created, to change the class's __dict__ attribute, at least from Python code. So for any given class klass, _shadowed_dict(klass) should always return the same result.

@carljm, what do you think?

• Issue: Optimise inspect.getattr_static #103193

[carljm]

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.

[AlexWaygood]

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()

[carljm]

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.

[AlexWaygood]

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)

[AlexWaygood]

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.

[AlexWaygood]

I considered inlining the calls to _static_getmro, but I thought this made it more readable. It also preserves git blame better, and having to pass through this function doesn't seem to have much impact on performance

[AlexWaygood]

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.

[AlexWaygood]

(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.)

[AlexWaygood]

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