gh-85160: improve performance of singledispatchmethod #106448

eendebakpt · 2023-07-05T09:28:38Z

This PR implements the idea from #85160 to improve performance of the singledispatchmethod by caching the generated dispatch method.

(updated) benchmark script:

import timeit
class Test:
    @singledispatchmethod
    def normal(self, item, arg):
        print('general')
    
    @normal.register
    def _(self, item:int, arg):
        return item + arg

    @singledispatchmethod
    @staticmethod
    def static(item, arg):
        print('general')
    
    @static.register
    @staticmethod
    def _(item:int, arg):
        return item + arg

class SlotClass():
    __slots__ = ("a", "b")
    
    def __init__(self):
        self.a = "zero"
        self.b = "one"
        
    @singledispatchmethod
    def normal(self, item, arg):
        print('general')
    
    @normal.register
    def _(self, item:int, arg):
        return item + arg
  
print('normal ', timeit.timeit('t.normal(1, 1)', globals={'t': Test()}))
print('static method', timeit.timeit('Test.static(1, 1)', globals={'t': Test(), 'Test': Test}))
print('static method instance', timeit.timeit('t.static(1, 1)', globals={'t': Test()}))
print('slotted class ', timeit.timeit('s.normal(1, 1)', globals={'s': SlotClass()}))

Output on main:

normal  2.5062581849997514
static method 2.7215049250007723
static method instance 2.922215309999956
slotted class  2.8028633869998885

This PR:

normal  0.9856528150003214
static method 0.9481567359998735
static method instance 0.9450106439999217
slotted class  0.9822057020001012

PR #23213 (alternative implementation)

normal  0.79438615700019
static method 3.4307698050006366
static method instance 0.7644647329998406
slotted class  3.4285360860003493

Issue: singledispatchmethod significantly slower than singledispatch #85160

AlexWaygood

Thanks! It looks like this doesn't differ significantly from #23213, which has already been open for a while -- is that correct?

eendebakpt · 2023-07-05T14:33:44Z

Thanks! It looks like this doesn't differ significantly from #23213, which has already been open for a while -- is that correct?

You are right, I somehow missed that one. I will compare the two PRs. If this one does not have any benefits I will close this one.

eendebakpt · 2023-07-05T18:45:18Z

Thanks! It looks like this doesn't differ significantly from #23213, which has already been open for a while -- is that correct?

@AlexWaygood The two implementations are similar, but there are some differences so perhaps one is preferable over the other.

Lib/functools.py

CaselIT · 2023-07-05T21:26:17Z

Looking at the different implementation performance, if it does not become too complex,
maybe _dispatch_method = None could be set on the singledispatchmethod class, then __get__ tries to assign __dict__ and fallsback on the _dispatch_method logic? May not be worth the extra complexity though

Lib/functools.py

Misc/NEWS.d/next/Library/2023-07-05-11-28-57.gh-issue-85160.t925jm.rst

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

corona10 · 2023-07-21T14:39:21Z

@eendebakpt Well, I will take a look at this PR by this weekend.

AlexWaygood · 2023-07-21T14:40:05Z

8000

(I haven't had time yet, but I'd also like to take a look! I'll try to do so this weekend :)

AlexWaygood

I'm a little concerned that this would cache singledispatchmethods across different instances of the same class. That would mean that singledispatchmethods would have pretty different semantics to most other methods in Python.

Here's what you get with most methods in Python:

>>> class Klass:
...     def method(self): pass
...
>>> Klass().method is Klass().method
False

And here's the behaviour on main with respect to singledispatchmethod:

>>> from functools import singledispatchmethod
>>> class Foo:
...     @singledispatchmethod
...     def neg(self, arg): raise NotImplementedError
... 
>>> Foo().neg is Foo().neg
False
>>> Foo().neg.bar = 'bar'
>>> Foo().neg.bar
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'function' object has no attribute 'bar'

Here's the behaviour you get with this PR branch:

>>> from functools import singledispatchmethod
>>> class Foo:
...     @singledispatchmethod
...     def neg(self, arg): raise NotImplementedError
...
>>> Foo().neg is Foo().neg
True
>>> Foo().neg.bar = 'bar'
>>> Foo().neg.bar
'bar'

Does this matter? Not sure. But in general, I feel like it's unfortunate to have the behaviour of singledispatchmethods differ from "normal" methods, and I'd imagine that's why this class was originally written the way that it is

AlexWaygood · 2023-07-21T18:18:00Z

Note that I don't see the same behaviour change with #23213 (as it's using a per-instance cache, rather than caching the same singledispatchmethod for all instances of Foo), so I generally prefer that approach. (But I like that you added a test for this PR, and the author of that PR hasn't responded to your review suggestion from a few weeks ago.)

eendebakpt · 2023-07-21T22:29:47Z

Note that I don't see the same behaviour change with #23213 (as it's using a per-instance cache, rather than caching the same singledispatchmethod for all instances of Foo), so I generally prefer that approach. (But I like that you added a test for this PR, and the author of that PR hasn't responded to your review suggestion from a few weeks ago.)

If we decide the approach from this PR is not the right one (for the reasons mentioned above) and there is no response from the other author I can modify this PR and add another test for the behavior change.

The behavior from current main also differs a bit from "normal" methods, for example

from functools import singledispatch

class Foo:
    def bar(self):
        pass
    
    @singledispatchmethod
    def neg(self, arg): raise NotImplementedError
     
f = Foo()
print(f.bar is f.bar) # prints False
print(f.bar == f.bar) # prints True
print(f.neg is f.neg) # prints False on main, on this PR that changes to True
print(f.neg == f.neg) # prints False on main, which differs from the "normal" method bar
print(f.neg.register == f.neg.register) # prints True

F=Foo()
f.neg.bar=1 # works as expected
f.neg.bar # raises an error! that is really strange. for both PRs one can set f.neg.bar

In a certain sense with this PR the behavior is more like normal methods than current main, since we can now set f.neg.bar=1 and use than later. But the behaviour might be misleading since it is not per instance but per class.

A per object cache (like #85160) definitely feels like a safer option. I quickly checked whether a lru_cache on the __get__ might be an option to improve performance. The performance is good, but I think the behavior will be even stranger.

corona10 · 2023-07-22T13:34:08Z

@eendebakpt

I have the same concern with @AlexWaygood
(Thank you Alex, for catching the side-effect, I was finding unexpected side-effects, and you pointed it out)

Well, IMO, the different result of the is operator doesn't look big issue for me. It is just for implementation detail.
(I think that is same as we don't recommend to compare int with is operator)
But assigning behavior is a big concern because this is quite a common operation and the user can meet unexpected behavior that the user didn't meet at older versions.

corona10 · 2023-07-22T13:37:25Z

@eendebakpt
Would you like to re-submit the patch based on #23213 with a co-authored commit of the origin author?
cc @AlexWaygood

eendebakpt · 2023-07-23T19:41:36Z

Closing in favor of #107148

generate dispatch method only once

5277335

eendebakpt requested a review from rhettinger as a code owner July 5, 2023 09:28

bedevere-bot added the awaiting review label Jul 5, 2023

bedevere-bot mentioned this pull request Jul 5, 2023

singledispatchmethod significantly slower than singledispatch #85160

Closed

eendebakpt and others added 2 commits July 5, 2023 13:27

fix whitespace

5eb3d3b

📜🤖 Added by blurb_it.

ad5bd22

AlexWaygood reviewed Jul 5, 2023

View reviewed changes

eendebakpt added 2 commits July 5, 2023 20:20

cache dispath method on singledispatch instance

abd1628

add test for slotted classes

4e068d4

eendebakpt mentioned this pull request Jul 5, 2023

gh-85160: Optimized singledispatchmethod access (noticeable improvement). #23213

Closed

CaselIT reviewed Jul 5, 2023

View reviewed changes

Lib/functools.py Show resolved Hide resolved

add missing statement

802f3d2

rhettinger removed their request for review July 7, 2023 09:07

corona10 reviewed Jul 8, 2023

View reviewed changes

Lib/functools.py Outdated Show resolved Hide resolved

corona10 reviewed Jul 8, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-07-05-11-28-57.gh-issue-85160.t925jm.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

72c8b0f

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

eendebakpt requested a review from corona10 July 20, 2023 09:59

corona10 self-assigned this Jul 21, 2023

AlexWaygood self-requested a review July 21, 2023 15:01

AlexWaygood reviewed Jul 21, 2023

View reviewed changes

eendebakpt mentioned this pull request Jul 23, 2023

gh-85160: improve performance of singledispatchmethod #107148

Merged

eendebakpt closed this Jul 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-85160: improve performance of singledispatchmethod #106448

gh-85160: improve performance of singledispatchmethod #106448

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gh-85160: improve performance of singledispatchmethod #106448

gh-85160: improve performance of singledispatchmethod #106448

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!