GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-127750: Fix and optimize functools.singledispatchmethod() by serhiy-storchaka · Pull Request #130008 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Feb 11, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: Regression in Django with singledispatchmethod on models #127750

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
pythongh-127750: Fix and optimize functools.singledispatchmethod()
7c5bb0e
Remove broken singledispatchmethod caching introduced in pythongh-85160.
Achieve the same performance using different optimization.
@serhiy-storchaka
serhiy-storchaka requested a review from eendebakpt February 11, 2025 16:40
@serhiy-storchaka
serhiy-storchaka requested a review from rhettinger as a code owner February 11, 2025 16:40
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 11, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 11, 2025
Regression in Django with singledispatchmethod on models #127750
Closed
@serhiy-storchaka
Add more tests.
93d0977
eendebakpt
eendebakpt reviewed Feb 11, 2025
View reviewed changes
Comment thread Lib/functools.py Outdated
@property
def __isabstractmethod__(self):
return getattr(self.func, '__isabstractmethod__', False)
def __module__(self):

@eendebakpt eendebakpt Feb 11, 2025

Copy link
Copy Markdown
Contributor

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, 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka Feb 12, 2025

Copy link
Copy Markdown
Member Author

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka Feb 12, 2025

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
serhiy-storchaka added 2 commits February 12, 2025 12:53
@serhiy-storchaka
Fix issues with __module__ and __doc__ descriptors.
16604a1
@serhiy-storchaka
Merge branch 'main' into singledispatchmethod
259dd9c
eendebakpt
eendebakpt reviewed Feb 15, 2025
View reviewed changes

@eendebakpt eendebakpt left a comment

Copy link
Copy Markdown
Contributor

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 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/functools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Feb 16, 2025

Copy link
Copy Markdown
Member Author

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
serhiy-storchaka merged commit 395335d into python:main Feb 17, 2025
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 17, 2025
@serhiy-storchaka
serhiy-storchaka deleted the singledispatchmethod branch February 17, 2025 09:11
@AA-Turner

AA-Turner commented Feb 18, 2025

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AA-Turner AA-Turner mentioned this pull request Feb 18, 2025
Python 3.14 nightly failing sphinx-doc/sphinx#13359
Closed
@serhiy-storchaka

serhiy-storchaka commented Feb 18, 2025

Copy link
Copy Markdown
Member Author

There is a bug here. It affects also pydoc,

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Feb 18, 2025

Copy link
Copy Markdown
Member Author

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

AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AA-Turner

AA-Turner commented Feb 18, 2025

Copy link
Copy Markdown
Member

Thanks!

A

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Feb 19, 2025
gh-127750: Restore inspect and pydoc support of singledispatchmethod #130309
Merged
@AA-Turner AA-Turner mentioned this pull request Feb 21, 2025
Revert "Mark autodoc singledispatchmethod tests as expected failures" sphinx-doc/sphinx#13381
Merged
@eendebakpt eendebakpt mentioned this pull request Mar 5, 2025
gh-127750: Fix singledispatchmethod caching (v2) #128648
Closed
@dg-pb

dg-pb commented Mar 5, 2025

Copy link
Copy Markdown
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 of FunctionType.

Or am I missing something?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@eendebakpt

eendebakpt commented Mar 5, 2025

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dg-pb

dg-pb commented Mar 5, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Mar 5, 2025

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dg-pb

dg-pb commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

And caching does not solve it.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dg-pb

dg-pb commented Mar 5, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

singledispatch                   -  5µs
singledispatchmethod             - 10µs (this PR)
cached FuntionType               -  7µs
cached _singledispatchmethod_get -  7.2 µs
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@serhiy-storchaka @AA-Turner @dg-pb @eendebakpt
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.