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 singledispatchmethod caching (v2) by eendebakpt · Pull Request #128648 · python/cpython

Conversation

@eendebakpt

@eendebakpt eendebakpt commented Jan 8, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Version based on idea from @dg-pb in #127839. This version

  • Fixes the issue of collisions between different objects with equal __hash__/__eq__ Regression in Django with singledispatchmethod on models #127750
  • Fixes the issue of keeping object instances alive
  • Adds two regression tests

There is still a cache (stored on the object instances). Quick benchmark (windows, non-pgo):

bench singledispatchmethod: Mean +- std dev: [main] 798 ns +- 64 ns -> [prx] 495 ns +- 38 ns: 1.61x faster

Benchmark hidden because not significant (1): bench singledispatchmethod slots

Geometric mean: 1.26x faster

(note that the alternative to this PR is not to keep main, but to revert #107148)

  • 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
eendebakpt and others added 6 commits January 8, 2025 21:14
@eendebakpt
add singledispatchmethod test for objects with equal hash and ==
b9a05dc
@eendebakpt
add singledispatchmethod test for dangling references
0714c06
@eendebakpt
use id for object cache in singledispatchmethod
ef03f2d
@blurb-it @eendebakpt
📜🤖 Added by blurb_it.
6c96b9d
@eendebakpt
rework using __dict__
f282ab8
@eendebakpt
remove comments
89a4a5c
@eendebakpt
eendebakpt requested a review from rhettinger as a code owner January 8, 2025 20:25
@eendebakpt
eendebakpt marked this pull request as draft January 8, 2025 20:25
@bedevere-app bedevere-app Bot added the awaiting review label Jan 8, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Jan 8, 2025
Regression in Django with singledispatchmethod on models #127750
Closed
@bedevere-app bedevere-app Bot removed the awaiting review label Jan 8, 2025
eendebakpt added 3 commits January 8, 2025 21:36
@eendebakpt
avoid exception
ecc3e82
@eendebakpt
whitespace
efd4034
@eendebakpt
whitespace
88b5e90
@eendebakpt
eendebakpt marked this pull request as ready for review January 8, 2025 20:50
@bedevere-app bedevere-app Bot added the awaiting review label Jan 8, 2025
@eendebakpt
Merge branch 'main' into singledispatchmethod_caching_v2
1885278
dg-pb
dg-pb reviewed Jan 8, 2025
View reviewed changes
Comment thread Lib/functools.py
import weakref # see comment in singledispatch function
self._method_cache = weakref.WeakKeyDictionary()
def __set_name__(self, obj, name):
self.attrname = name

@dg-pb dg-pb Jan 8, 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.

Check cached_property.__set_name__, it has some more stuff in it - might be needed here as well.

Sorry, something went wrong.

Uh oh!

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

All reactions

@eendebakpt eendebakpt Jan 8, 2025

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

Hmm. The additions there prevent something like this:

@dataclass(frozen=True)
class A:
    value: int

    @singledispatchmethod
    def dispatch(self, x):
        return id(self)

    renamed_dispatch = dispatch # allowed? if so, how should it behave

The corresponding test for the cached_property for this is

cpython/Lib/test/test_functools.py

Line 3315 in 34e840f

def test_reuse_different_names(self):
.

But on current main renaming is allowed for the singledispatchmethod.

I am not sure here what the desired behavior is (and why)

Sorry, something went wrong.

Uh oh!

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

All reactions

@dg-pb dg-pb Jan 8, 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.

If this implementation is desirable, maybe later someone who knows more about this can comment.

Sorry, something went wrong.

Uh oh!

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

All reactions

@vodik vodik Jan 23, 2025

Copy link
Copy Markdown

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.

As far as I know, the only reason cached properties can't be renamed is because the cache is keyed by the attribute's name.

Allowing a rebind would disconnect the cached property from it's cached value.

Sorry, something went wrong.

Uh oh!

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

All reactions

@vodik vodik Jan 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

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.

Actually, I think you might want to either ignore renames or do something along these lines (ignoring error handling):

    if self.attrname:
        cache[name] = cache.pop(self.attrname)
    self.attrname = name

As far as I know, each binding shares the same instance of the descriptor, so as long as the cache key is constant, it should work no matter how many times it's been renamed.

Sorry, something went wrong.

Uh oh!

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

All reactions

@dg-pb dg-pb Jan 25, 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.

Allowing a rebind would disconnect the cached property from it's cached value.

This is kind of the same situation.

If rename is allowed, then it would simply cache to the last attrname. Drawback is that there is a small risk for unused cached methods.

I think it might be most straight forward to copy+paste cached_property.__set_name__. It does seem a sensible restriction. It comes at expense of flexibility, but personally, I have never run into that TypeError.

Also, it will be easier to address changes/improvements when 2 implementations that use the same caching approach are aligned.

Sorry, something went wrong.

Uh oh!

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

jeremyk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
serhiy-storchaka
serhiy-storchaka reviewed Feb 6, 2025
View reviewed changes
Comment thread Lib/functools.py
if self._method_cache is not None:
self._method_cache[obj] = _method
if cache is not None:
cache[self.attrname] = _method

@serhiy-storchaka serhiy-storchaka Feb 6, 2025

Copy link
Copy Markdown
Member

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.

Does not it create a reference loop? obj refers to cache, cache refers to _method, _method refers to a cell which refers to obj.

Sorry, something went wrong.

Uh oh!

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

All reactions

@eendebakpt eendebakpt Feb 6, 2025

Copy link
Copy Markdown
Contributor 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. But once there are no external references to the object obj any more the garbage collector removes the objects. (the cache is on the object obj, not on the singledispatchmethod itself or the class)

In the current main the caching is done on the singledispatchmethod which keeps the generated methods alive.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Feb 11, 2025

Copy link
Copy Markdown
Member

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, the current situation is worse, it creates strong references singledispatchmethod -> _method -> obj.

Relying on the garbage collection is not good. This particular loop can be broken by using a weak reference to obj instead of obj. But a reference from a bound method to the object should be strong, otherwise some code will not work (there was a similar issue with TemporaryFile).

I am not sure how much this optimization saves. Are there other ways to achieve the same speed up, without creating reference loops?

Sorry, something went wrong.

Uh oh!

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

All reactions
@eendebakpt

eendebakpt commented Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Closing in favor of #130008

All reactions

Sorry, something went wrong.

Uh oh!

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

@eendebakpt eendebakpt closed this Mar 5, 2025
@dg-pb dg-pb mentioned this pull request Mar 5, 2025
gh-127750: Fix and optimize functools.singledispatchmethod() #130008
Merged
@eendebakpt
eendebakpt deleted the singledispatchmethod_caching_v2 branch December 11, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
+2 more reviewers
@vodik vodik vodik left review comments
@dg-pb dg-pb dg-pb left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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

@eendebakpt @vodik @dg-pb @serhiy-storchaka
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.