Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchakaserhiy-storchaka commentedFeb 11, 2025
edited by bedevere-appbot
Loading

Remove broken singledispatchmethod caching introduced ingh-85160. Achieve the same performance using different optimization.

Remove broken singledispatchmethod caching introduced inpythongh-85160.Achieve the same performance using different optimization.
@property
def__isabstractmethod__(self):
returngetattr(self.func,'__isabstractmethod__',False)
def__module__(self):
Copy link
Contributor

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 string

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

@eendebakpteendebakpt left a 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>

Comment on lines +1073 to +1074
ifnamenotin {'__name__','__qualname__','__isabstractmethod__',
'__annotations__','__type_params__'}:
Copy link
Contributor

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Plus__isabstractmethod__.

@serhiy-storchaka
Copy link
MemberAuthor

Yes, I aware of this. Actually, I wrote also code and tests for__repr__, but have not included it in this PR, because the format of the repr may need a separate discussion. The current repr is an implementation artifact, and is not necessary the best.

eendebakpt reacted with thumbs up emoji

@serhiy-storchakaserhiy-storchaka merged commit395335d intopython:mainFeb 17, 2025
39 checks passed
@serhiy-storchakaserhiy-storchaka deleted the singledispatchmethod branchFebruary 17, 2025 09:11
@AA-Turner
Copy link
Member

@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
Copy link
MemberAuthor

There is a bug here. It affects also pydoc,

@serhiy-storchaka
Copy link
MemberAuthor

I have found the cause, but it is late here, I'll fix this tomorrow.

AA-Turner reacted with thumbs up emoji

@AA-Turner
Copy link
Member

Thanks!

A

@dg-pb
Copy link
Contributor

So caching was abandoned all together?

With this, the call of__get__ is extremely slow as it constructs new instance every time. 420 ns every time for__get__ only +__call__ of instance is slower than the one ofFunctionType.

Or am I missing something?

@eendebakpt
Copy link
Contributor

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

dg-pb commentedMar 5, 2025
edited
Loading

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_singledispatchmethod_get instances and calling its__call__.

@serhiy-storchaka
Copy link
MemberAuthor

Yes, singledispatchmethod has an overhead. And caching does not solve it. If you have other solution, please open a new issue.

@dg-pb
Copy link
Contributor

And caching does not solve it.

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

dg-pb commentedMar 5, 2025
edited
Loading

If you have other solution, please open a new issue.

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 ofcached_property is acceptable,_singledispatchmethod_get caching can always be implemented. For my example, its time is 7.2 µs, to sum up:

singledispatch                   -  5µssingledispatchmethod             - 10µs (this PR)cached FuntionType               -  7µscached _singledispatchmethod_get -  7.2 µs

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@eendebakpteendebakpteendebakpt left review comments

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@serhiy-storchaka@AA-Turner@dg-pb@eendebakpt

[8]ページ先頭

©2009-2025 Movatter.jp