gh-127750: Fix and optimize functools.singledispatchmethod()

[serhiy-storchaka]

Remove broken singledispatchmethod caching introduced in gh-85160. Achieve the same performance using different optimization.

• Issue: Regression in Django with singledispatchmethod on models #127750

[eendebakpt]

This is giving some problems in my interactive console. A minimal reproducer:

from functools import singledispatchmethod, singledispatch
from IPython.lib.pretty import pretty

class A:
    def __init__(self, value):
        self.value = value
        
    @singledispatchmethod
    def dp(self, x):
        return id(self)
    
a=obj=A(4)

pretty(a.dp) # fails

_singledispatchmethod_get.__module__ # this is now a property, not a string

[serhiy-storchaka]

Yes, there is a problem with type attributes which we want to define for instances (__name__, __qualname__, __doc__). If we define a property, it conflicts with the type attribute.

[serhiy-storchaka]

This can be resolved by defining __getattribute__ instead of __getattr__. But this adds such large overhead, that it outweigh the optimization gain.

So I added setting just two instance attributes __module__ and __doc__ in the constructor. This adds some overhead, but not so large as in the original code. I hope that more satisfying solution will be found in future, but this is a complex issue.

[eendebakpt]

This fixes the bugs and the performance is good. But there are some more corner cases I am worried about. For example:

from functools import *

class A:
    
    @singledispatchmethod
    def dp(self, x):
        return x
a = A()

print(repr(a.dp))
print(str(a.dp))

Results in

<functools._singledispatchmethod_get object at 0x0000014DCB8A3B60>
<functools._singledispatchmethod_get object at 0x0000014DCB8A4E10>

On main (and on 3.12) it is:

<function A.dp at 0x00000182896CA200>
<function A.dp at 0x00000182896CA200>

[eendebakpt]

Note: this set is functools.WRAPPER_ASSIGNMENTS minus __doc__ and __module__ which have special handling. Fine to leave it this way

[serhiy-storchaka]

Plus __isabstractmethod__.

[serhiy-storchaka]

Yes, I aware of this. Actually, I wrote also code and tests for __repr__, but have not included it in this PR, because the format of the repr may need a separate discussion. The current repr is an implementation artifact, and is not necessary the best.

[AA-Turner]

@serhiy-storchaka I think this has caused a regression in Sphinx tests (sphinx-doc/sphinx#13360), though the onus may be on Sphinx to fix this -- noting for visibility though.

A

[serhiy-storchaka]

There is a bug here. It affects also pydoc,

[serhiy-storchaka]

I have found the cause, but it is late here, I'll fix this tomorrow.

[AA-Turner]

Thanks!

A

[dg-pb]

So caching was abandoned all together?

With this, the call of __get__ is extremely slow as it constructs new instance every time. 420 ns every time for __get__ only + __call__ of instance is slower than the one of FunctionType.

Or am I missing something?

[eendebakpt]

Yes, we abandoned caching because of several issues with caching (we tried several options, they all have some issues).

However, with this PR performance the performance is almost as good (or sometimes better) than the caching approaches. The performance issue was only partly creating a new instance, but mostly getting attributes when updating attributes on the newly created instance.

Do you have any examples where performance is still an issue?

[dg-pb]

I find it a bit hard to believe that performance of this is good.

I have no personal interest in this, but just thinking in general. Don't think this sort of application is very common, but let's say an example:

from functools import singledispatchmethod, singledispatch


@singledispatch
def flatten_js(obj, parent_key=None):
    yield obj if parent_key is None else (parent_key, obj)

@flatten_js.register
def _(obj: dict, parent_key=None):
    for k, v in obj.items():
        new_key = (k,) if parent_key is None else parent_key + (k,)
        yield from flatten_js(v, new_key)

@flatten_js.register
def _(obj: list, parent_key=None):
    for k, v in enumerate(obj):
        new_key = (k,) if parent_key is None else parent_key + (k,)
        yield from flatten_js(v, new_key)

class A:
    @singledispatchmethod
    def flatten_js(self, obj, parent_key=None):
        yield obj if parent_key is None else (parent_key, obj)

    @flatten_js.register
    def _(self, obj: dict, parent_key=None):
        for k, v in obj.items():
            new_key = (k,) if parent_key is None else parent_key + (k,)
            yield from self.flatten_js(v, new_key)

    @flatten_js.register
    def _(self, obj: list, parent_key=None):
        for k, v in enumerate(obj):
            new_key = (k,) if parent_key is None else parent_key + (k,)
            yield from self.flatten_js(v, new_key)

a = A()

# ---
./python.exe -m timeit -s $S "list(a.flatten_js({'a': [1, 2, 3, [4]]}))"    # 10µs
./python.exe -m timeit -s $S "list(flatten_js({'a': [1, 2, 3, [4]]}))"      #  5µs

So for applications of low complexity such as this, half of the time spent is an overhead of constructing _singledispatchmethod_get instances and calling its __call__.

[serhiy-storchaka]

Yes, singledispatchmethod has an overhead. And caching does not solve it. If you have other solution, please open a new issue.

[dg-pb]

And caching does not solve it.

With this:

class singledispatchmethod(functools.singledispatchmethod):
    def __set_name__(self, obj, name):
        self.attrname = name

    def __get__(self, obj, cls=None):
        cache = obj.__dict__
        try:
            return cache[self.attrname]
        except:
            def method(*args, **kwargs):
                return self.dispatcher.dispatch(args[0].__class__).__get__(obj, cls)(*args, **kwargs)
            cache[self.attrname] = method
            return method

./python.exe -m timeit -s $S "list(a.flatten_js({'a': [1, 2, 3, [4]]}))"    # 7 µs

So caching, at least for this case, results in 60% lower overhead.

[dg-pb]

If you have other solution, please open a new issue.

In terms of performance, caching as per #128648 (comment) had benefits - I have nothing else.

If anyone wants needs performance benefit of caching and the approach of cached_property is acceptable, _singledispatchmethod_get caching can always be implemented. For my example, its time is 7.2 µs, to sum up:

singledispatch                   -  5µs
singledispatchmethod             - 10µs (this PR)
cached FuntionType               -  7µs
cached _singledispatchmethod_get -  7.2 µs