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

  1. python
  2. cpython

Repository navigation

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

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eendebakpt wants to merge 10 commits into from
Conversation 10 Commits 10 Checks 0 Files changed
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b9a05dc
add singledispatchmethod test for objects with equal hash and ==
eendebakpt Dec 11, 2024
0714c06
add singledispatchmethod test for dangling references
eendebakpt Dec 11, 2024
ef03f2d
use id for object cache in singledispatchmethod
eendebakpt Dec 11, 2024
6c96b9d
📜🤖 Added by blurb_it.
blurb-it[bot] Dec 11, 2024
f282ab8
rework using __dict__
eendebakpt Jan 8, 2025
89a4a5c
remove comments
eendebakpt Jan 8, 2025
ecc3e82
avoid exception
eendebakpt Jan 8, 2025
efd4034
whitespace
eendebakpt Jan 8, 2025
88b5e90
whitespace
eendebakpt Jan 8, 2025
1885278
Merge branch 'main' into singledispatchmethod_caching_v2
eendebakpt Jan 8, 2025
File filter

Filter by extension

Filter by extension .py  (2) .rst  (1) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
24 changes: 11 additions & 13 deletions Lib/functools.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1026,8 +1026,8 @@ def __init__(self, func):
self.dispatcher = singledispatch(func)
self.func = func

import weakref # see comment in singledispatch function
self._method_cache = weakref.WeakKeyDictionary()
def __set_name__(self, obj, name):
self.attrname = name

dg-pb Jan 8, 2025

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.

eendebakpt Jan 8, 2025

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

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)

dg-pb Jan 8, 2025

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.

vodik Jan 23, 2025

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.

dg-pb Jan 25, 2025

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.

def register(self, cls, method=None):
"""generic_method.register(cls, func) -> func
Expand All @@ -1037,15 +1037,13 @@ def register(self, cls, method=None):
return self.dispatcher.register(cls, func=method)

def __get__(self, obj, cls=None):
if self._method_cache is not None:
try:
_method = self._method_cache[obj]
except TypeError:
self._method_cache = None
except KeyError:
pass
else:
return _method
if hasattr(obj, '__dict__'):
cache = obj.__dict__
method = cache.get(self.attrname)
if method is not None:
return method
else:
cache = None

dispatch = self.dispatcher.dispatch
funcname = getattr(self.func, '__name__', 'singledispatchmethod method')
Expand All @@ -1059,8 +1057,8 @@ def _method(*args, **kwargs):
_method.register = self.register
update_wrapper(_method, self.func)

if self._method_cache is not None:
self._method_cache[obj] = _method
if cache is not None:
cache[self.attrname] = _method

serhiy-storchaka Feb 6, 2025

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.

eendebakpt Feb 6, 2025

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.

serhiy-storchaka Feb 11, 2025

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?

return _method

Expand Down
47 changes: 47 additions & 0 deletions Lib/test/test_functools.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import collections
import collections.abc
import copy
import gc
from itertools import permutations
import pickle
from random import choice
Expand Down Expand Up @@ -3240,6 +3241,52 @@ def f(arg):
def _(arg: undefined):
return "forward reference"

def test_singledispatchmethod_hash_comparision_equal(self):
from dataclasses import dataclass

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

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

t1 = A(1)
t2 = A(1)

assert t1 == t2
assert id(t1) == t1.dispatch(2)
assert id(t2) == t2.dispatch(2) # gh-127750

def test_singledispatchmethod_object_references(self):
class ReferenceTest:
instance_counter = 0

def __init__(self):
ReferenceTest.instance_counter = ReferenceTest.instance_counter + 1

def __del__(self):
ReferenceTest.instance_counter = ReferenceTest.instance_counter - 1

@functools.singledispatchmethod
def go(self, item):
pass

assert ReferenceTest.instance_counter == 0
t=ReferenceTest()
assert ReferenceTest.instance_counter == 1
x = []
for ii in range(1000):
t = ReferenceTest()
t.go(ii)
x.append(t)
del t
del x
gc.collect()
assert ReferenceTest.instance_counter == 0


class CachedCostItem:
_cost = 1

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-12-11-20-26-44.gh-issue-127750.8witaH.rst
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update caching of :class:`functools.singledispatchmethod` to avoid collisions of objects which compare equal.
Back | FazBrowse Home | New Git URL