| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I tried this locally and added and it seems to be working correctly. Haven't benchmarked it either though. The cached methods seem to stick around in the cache until the gc fires. I haven't been able to find a way to make them stick around. |
Sorry, something went wrong.
|
Does CPython re-use id values after instances are destroyed? SO says that yes, but it was in 2018. Is it still true? If so, then can identity be reliably cached at all? Maybe similar approach to cached_property could be used here? Although it would restrict to classes that have instance.__dict__. |
Sorry, something went wrong.
Yes, cpython re-uses the id (the id is equal to the memory address, and memory is reused). But the id is unique during the lifetime of an object, see https://docs.python.org/3/library/functions.html#id. The _method in the WeakValueDictionary has a reference to obj, so the key id(obj)is valid and unique as long as _method exists. Once the garbage collector detects a cycle containing the _method it will remove it from the dictionary. But since obj is part of this cycle, it also means obj no longer exist and id(obj) is free to re-use. But your remark got me thinking: the PR right now is reliable, but maybe not very efficient. Running gc.collect clears the entire cache. The benchmarks I executed showed an improvement, but that might very well be because the garbage collector is not running often. |
Sorry, something went wrong.
Could you clarify what do you mean by this? |
Sorry, something went wrong.
@dataclass
class A:
value: int
@singledispatchmethod
def dispatch(self, x):
return id(self)
t1 = A(1)
t1.dispatch(10)
Right after running this code the cache (e.g. _method_cache) will contain a single key-value pair: the key is id(t1) and the value is the _method object created in the dispatcher. The _method has a positive reference count (since _method.__self_reference holds a reference to _method). We can address the issue by creating a reference from obj to the entry in the cache. Something like: try:
setattr(obj, f'_singledispatchmethod_cache_{id(self)}', _method) # create strong reference to _method which is not an isolated cycle. see gh-127751
except Exception as ex:
# frozen object, disable the cache
self._method_cache = None
This works, but has the disadvantage that we are adding some (private) attributes in the object instance. Note: the documentation https://docs.python.org/3/library/weakref.html#weakref.WeakValueDictionary states:
That text is a bit misleading: there is a reference to _method, but still it is removed. I will make a separate PR to address this. |
Sorry, something went wrong.
|
I see now. Also, I would like the method to be cached for the lifetime of my instance, doesn't it currently garbage collect perfectly good cached methods?
What about the same way as cached_property works? class singledispatchmethod:
def __set_name__(self, owner, name):
self.attrname = name
def __get__(self, instance, owner=None):
try:
cache = instance.__dict__
except AttributeError:
# disable caching going forward
else:
method = cache.get(self.attrname)
if method is not None:
return method
else:
... |
Sorry, something went wrong.
| return _method | ||
|
|
||
| dispatch = self.dispatcher.dispatch | ||
| funcname = getattr(self.func, '__name__', 'singledispatchmethod method') |
There was a problem hiding this comment.
Maybe moving this line just above the TypeError would be a bit more readable?
The performance benefit this offers would only matter if there was a code which actively depends on handling this exception.
Sorry, something went wrong.
Yes, that is why I did not like the approach in this PR.
Nice idea. I created an alternative PR for this variation. |
Sorry, something went wrong.
|
Issue resolved in other PRs, closing |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
We address two issues:
See #127751