-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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.
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
Conversation
There was a problem hiding this 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?
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. |
@AlexWaygood The two implementations are similar, but there are some differences so perhaps one is preferable over the other. |
Looking at the different implementation performance, if it does not become too complex, |
Misc/NEWS.d/next/Library/2023-07-05-11-28-57.gh-issue-85160.t925jm.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@eendebakpt Well, I will take a look at this PR by this weekend. |
(I haven't had time yet, but I'd also like to take a look! I'll try to do so this weekend :) |
There was a problem hiding this comment.
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 singledispatchmethod
s across different instances of the same class. That would mean that singledispatchmethod
s 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 singledispatchmethod
s differ from "normal" methods, and I'd imagine that's why this class was originally written the way that it is
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 |
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
In a certain sense with this PR the behavior is more like normal methods than current main, since we can now set A per object cache (like #85160) definitely feels like a safer option. I quickly checked whether a |
I have the same concern with @AlexWaygood Well, IMO, the different result of the |
@eendebakpt |
Closing in favor of #107148 |
This PR implements the idea from #85160 to improve performance of the singledispatchmethod by caching the generated dispatch method.
(updated) benchmark script:
Output on main:
This PR:
PR #23213 (alternative implementation)