8000 gh-85160: improve performance of singledispatchmethod by eendebakpt · Pull Request #106448 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-85160: improve performance of singledispatchmethod #106448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

gh-85160: improve performance of singledispatchmethod #106448

wants to merge 7 commits into from

Conversation

eendebakpt
Copy link
Contributor
@eendebakpt eendebakpt commented Jul 5, 2023 •
edited
Loading

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

@eendebakpt eendebakpt requested a review from rhettinger as a code owner July 5, 2023 09:28
@bedevere-bot bedevere-bot mentioned this pull request Jul 5, 2023
Closed
AlexWaygood
AlexWaygood reviewed Jul 5, 2023
View reviewed changes
Copy link
Member
@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eendebakpt
Copy link
Contributor Author

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 eendebakpt mentioned this pull request Jul 5, 2023
Closed
@eendebakpt
Copy link
Contributor Author

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.

CaselIT
CaselIT reviewed Jul 5, 2023
View reviewed changes
@CaselIT
Copy link
CaselIT commented Jul 5, 2023

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

@rhettinger rhettinger removed their request for review July 7, 2023 09:07
corona10
corona10 reviewed Jul 8, 2023
View reviewed changes
@eendebakpt @corona10
Apply suggestions from code review
72c8b0f 
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@eendebakpt eendebakpt requested a review from corona10 July 20, 2023 09:59
@corona10
Copy link
Member

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

@corona10 corona10 self-assigned this Jul 21, 2023
@AlexWaygood
Copy link
Member
AlexWaygood commented Jul 21, 2023 •
edited
Loading

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

@AlexWaygood AlexWaygood self-requested a review July 21, 2023 15:01
AlexWaygood
AlexWaygood reviewed Jul 21, 2023
View reviewed changes
Copy link
Member
@AlexWaygood AlexWaygood left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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 8000 a few weeks ago.)

@eendebakpt
Copy link
Contributor Author

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
Copy link
Member
corona10 commented Jul 22, 2023 •
edited
Loading

@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
Copy link
Member
8000 corona10 commented Jul 22, 2023

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

@eendebakpt eendebakpt mentioned this pull request Jul 23, 2023
Merged
@eendebakpt
Copy link
Contributor Author

Closing in favor of #107148

@eendebakpt eendebakpt closed this Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaselIT CaselIT CaselIT left review comments

@AlexWaygood AlexWaygood AlexWaygood left review comments

@corona10 corona10 Awaiting requested review from corona10

Assignees

@corona10 corona10

Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eendebakpt @CaselIT @corona10 @AlexWaygood @bedevere-bot
0