FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-127750: Fix singledispatchmethod caching (v2) by eendebakpt · Pull Request #128648 · python/cpython · GitHub

Repository navigation

gh-127750: Fix singledispatchmethod caching (v2) - #128648

Closed
eendebakpt wants to merge 10 commits into
python:mainfrom
eendebakpt:singledispatchmethod_caching_v2
Closed

eendebakpt wants to merge 10 commits into
python:mainfrom
eendebakpt:singledispatchmethod_caching_v2

Conversation

eendebakpt commented Jan 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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)

eendebakpt marked this pull request as ready for review January 8, 2025 20:50
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vodik Jan 23, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Copy link
Copy Markdown
Contributor Author

Closing in favor of #130008

eendebakpt closed this Mar 5, 2025
eendebakpt deleted the singledispatchmethod_caching_v2 branch December 11, 2025 19:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL