Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-127750: Fix and optimize functools.singledispatchmethod()#130008
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-127750: Fix and optimize functools.singledispatchmethod()#130008
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Remove broken singledispatchmethod caching introduced inpythongh-85160.Achieve the same performance using different optimization.
Lib/functools.py Outdated
| @property | ||
| def__isabstractmethod__(self): | ||
| returngetattr(self.func,'__isabstractmethod__',False) | ||
| def__module__(self): |
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.
This is giving some problems in my interactive console. A minimal reproducer:
from functools import singledispatchmethod, singledispatchfrom IPython.lib.pretty import prettyclass 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 stringThere 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.
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.
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.
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 left a comment
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.
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 xa = 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>| ifnamenotin {'__name__','__qualname__','__isabstractmethod__', | ||
| '__annotations__','__type_params__'}: |
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.
Note: this set isfunctools.WRAPPER_ASSIGNMENTS minus__doc__ and__module__ which have special handling. Fine to leave it this way
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.
Plus__isabstractmethod__.
serhiy-storchaka commentedFeb 16, 2025
Yes, I aware of this. Actually, I wrote also code and tests for |
395335d intopython:mainUh oh!
There was an error while loading.Please reload this page.
AA-Turner commentedFeb 18, 2025
@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 commentedFeb 18, 2025
There is a bug here. It affects also pydoc, |
serhiy-storchaka commentedFeb 18, 2025
I have found the cause, but it is late here, I'll fix this tomorrow. |
AA-Turner commentedFeb 18, 2025
Thanks! A |
dg-pb commentedMar 5, 2025
So caching was abandoned all together? With this, the call of Or am I missing something? |
eendebakpt commentedMar 5, 2025
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 commentedMar 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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: fromfunctoolsimportsingledispatchmethod,singledispatch@singledispatchdefflatten_js(obj,parent_key=None):yieldobjifparent_keyisNoneelse (parent_key,obj)@flatten_js.registerdef_(obj:dict,parent_key=None):fork,vinobj.items():new_key= (k,)ifparent_keyisNoneelseparent_key+ (k,)yieldfromflatten_js(v,new_key)@flatten_js.registerdef_(obj:list,parent_key=None):fork,vinenumerate(obj):new_key= (k,)ifparent_keyisNoneelseparent_key+ (k,)yieldfromflatten_js(v,new_key)classA:@singledispatchmethoddefflatten_js(self,obj,parent_key=None):yieldobjifparent_keyisNoneelse (parent_key,obj)@flatten_js.registerdef_(self,obj:dict,parent_key=None):fork,vinobj.items():new_key= (k,)ifparent_keyisNoneelseparent_key+ (k,)yieldfromself.flatten_js(v,new_key)@flatten_js.registerdef_(self,obj:list,parent_key=None):fork,vinenumerate(obj):new_key= (k,)ifparent_keyisNoneelseparent_key+ (k,)yieldfromself.flatten_js(v,new_key)a=A()# ---./python.exe-mtimeit-s $S"list(a.flatten_js({'a': [1, 2, 3, [4]]}))"# 10µs./python.exe-mtimeit-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 |
serhiy-storchaka commentedMar 5, 2025
Yes, singledispatchmethod has an overhead. And caching does not solve it. If you have other solution, please open a new issue. |
dg-pb commentedMar 5, 2025
With this: classsingledispatchmethod(functools.singledispatchmethod):def__set_name__(self,obj,name):self.attrname=namedef__get__(self,obj,cls=None):cache=obj.__dict__try:returncache[self.attrname]except:defmethod(*args,**kwargs):returnself.dispatcher.dispatch(args[0].__class__).__get__(obj,cls)(*args,**kwargs)cache[self.attrname]=methodreturnmethod./python.exe-mtimeit-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 commentedMar 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
Uh oh!
There was an error while loading.Please reload this page.
Remove broken singledispatchmethod caching introduced ingh-85160. Achieve the same performance using different optimization.