Skip to content
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.

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

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

Closed
wants to merge 2 commits into from

Conversation

mental32
Copy link
Sponsor Contributor

@mental32 mental32 commented Nov 10, 2020

I benchmarked using the following code:

    from functools import singledispatch, singledispatchmethod
    import timeit

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

    @singledispatch
    def go(item, arg):
        print('general')

    @go.register
    def _(item:int, arg):
        return item + arg

    print('m', timeit.timeit('t.go(1, 1)', globals={'t': Test()}))
    print('s', timeit.timeit('go(1, 1)', globals={'go': go}))

And got the following output:

mental@felix ~/D/o/cpython (master)> ./python /tmp/fdsm.py
m 4.794188453000061
s 0.9868643390000216
mental@felix ~/D/o/cpython (master)> git checkout 40988
Switched to branch '40988'
mental@felix ~/D/o/cpython (40988) [1]> ./python /tmp/fdsm.py
m 1.3791182540007867
s 0.9882650030003788

I'm still curious if access could be further optimized clueless when it comes to ideas 😅

https://bugs.python.org/issue40988

@mental32 mental32 changed the title bpo-40988: Optimized singledispatchmethod access (3x ish improvement). bpo-40988: Optimized singledispatchmethod access (noticeable improvement). Nov 10, 2020
@CaselIT
Copy link

CaselIT commented Nov 10, 2020

Thanks for taking this up!

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@CaselIT
Copy link

CaselIT commented Dec 16, 2020

Comment to avoid closure

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 16, 2021
@mental32
Copy link
Sponsor Contributor Author

@rhettinger sorry for bothering you but do you have enough bandwidth to review this or can you defer to someone else?

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jan 17, 2021
@CaselIT
Copy link

CaselIT commented Sep 17, 2021

Not sure if there is anything we should do to allow this to be reviewed

@rhettinger rhettinger removed their request for review May 3, 2022 06:14
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

On my slow system more modest, but an improvement:
11.24
2.27
to
5.83
3.54
I not that this is built on 3.10.0a2+ so there maybe further speed improvements when 3.11/3.12 is released. So a worthwhile change.

@TeamSpen210
Copy link

Since this is something that’ll have observable effects on objects, should this be documented? If you’re introspecting your objects, or are say concerned about memory usage it might be surprising behaviour.

@AlexWaygood AlexWaygood changed the title bpo-40988: Optimized singledispatchmethod access (noticeable improvement). gh-85160: Optimized singledispatchmethod access (noticeable improvement). Jul 5, 2023
@eendebakpt
Copy link
Contributor

@mental32 @AlexWaygood By accident I created an independent implementation of the singledispatchmethod improvement. In #106448 I added a performance comparison of this PR and the other one. I also added a test for the case of a class with slots (commit 4e068d4). Feel free to pull that one into this branch.

Both branches give a performance improvement over current main. Hopefully one of them gets accepted.

Comment on lines +957 to +963
if self.attrname is not None:
attrname = self.attrname

try:
obj.__dict__[attrname] = _method
except AttributeError:
pass # not all objects have __dict__ (e.g. class defines slots)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.attrname is not None:
attrname = self.attrname
try:
obj.__dict__[attrname] = _method
except AttributeError:
pass # not all objects have __dict__ (e.g. class defines slots)
if self.attrname is not None:
try:
obj.__dict__[self.attrname] = _method
except AttributeError:
pass # not all objects have __dict__ (e.g. class defines slots)

@corona10 corona10 self-assigned this Jul 21, 2023
@mental32
Copy link
Sponsor Contributor Author

mental32 commented Jul 26, 2023

@eendebakpt thank you for continuing with this!

for any changes or merging that you would like to see done is it okay for me to say "do whatever you need to do :)" and defer to you? (including closing this PR if the other one is sufficient in perf improvements) I don't think I have the time to pay attention to this is all

@AlexWaygood
Copy link
Member

Closing in favour of #107148 (in which you have been listed as a co-author :)

@AlexWaygood AlexWaygood closed this Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.