GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||
GeezerVison is showing github.com — gh-127750: Fix and optimize functools.singledispatchmethod() by serhiy-storchaka · Pull Request #130008 · python/cpython Conversation
|
| @property | ||
| def __isabstractmethod__(self): | ||
| return getattr(self.func, '__isabstractmethod__', False) | ||
| def __module__(self): |
There was a problem 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, singledispatch
from IPython.lib.pretty import pretty
class 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 string
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 x
a = 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>
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Yes, I aware of this. Actually, I wrote also code and tests for |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There is a bug here. It affects also pydoc, |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have found the cause, but it is late here, I'll fix this tomorrow. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks! A |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
So caching was abandoned all together? With this, the call of Or am I missing something? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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: from functools import singledispatchmethod, singledispatch @singledispatch def flatten_js(obj, parent_key=None): yield obj if parent_key is None else (parent_key, obj) @flatten_js.register def _(obj: dict, parent_key=None): for k, v in obj.items(): new_key = (k,) if parent_key is None else parent_key + (k,) yield from flatten_js(v, new_key) @flatten_js.register def _(obj: list, parent_key=None): for k, v in enumerate(obj): new_key = (k,) if parent_key is None else parent_key + (k,) yield from flatten_js(v, new_key) class A: @singledispatchmethod def flatten_js(self, obj, parent_key=None): yield obj if parent_key is None else (parent_key, obj) @flatten_js.register def _(self, obj: dict, parent_key=None): for k, v in obj.items(): new_key = (k,) if parent_key is None else parent_key + (k,) yield from self.flatten_js(v, new_key) @flatten_js.register def _(self, obj: list, parent_key=None): for k, v in enumerate(obj): new_key = (k,) if parent_key is None else parent_key + (k,) yield from self.flatten_js(v, new_key) a = A() # --- ./python.exe -m timeit -s $S "list(a.flatten_js({'a': [1, 2, 3, [4]]}))" # 10µs ./python.exe -m timeit -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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, singledispatchmethod has an overhead. And caching does not solve it. If you have other solution, please open a new issue. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
With this: class singledispatchmethod(functools.singledispatchmethod): def __set_name__(self, obj, name): self.attrname = name def __get__(self, obj, cls=None): cache = obj.__dict__ try: return cache[self.attrname] except: def method(*args, **kwargs): return self.dispatcher.dispatch(args[0].__class__).__get__(obj, cls)(*args, **kwargs) cache[self.attrname] = method return method ./python.exe -m timeit -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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.