gh-150708: Make functools.cache (unbound) and cached_property strictly idempotent - #154069
gh-150708: Make functools.cache (unbound) and cached_property strictly idempotent#154069seberg wants to merge 6 commits into
Conversation
When running multiple threads, we don't want to guarantee call-once
style behavior for functools.cache or cached_property.
That could be convenient but is not strictly needed.
However, we should use `dict.setdefault()` to ensure that:
obj.cached_property is obj.cached_property
and:
cached_fuction(arg) is cached_function(arg)
Both may be run twice (and for statistics purposes we may return
a cached value but still have a miss).
Documentation build overview
44 files changed ·
|
| val = self.func(instance) | ||
| try: | ||
| cache[self.attrname] = val | ||
| val = cache.setdefault(self.attrname, val) |
There was a problem hiding this comment.
I talked with Petr briefly at the sprint about this. It isn't clear to me if __dict__ could be something weird that might succeed with __setattr__ but not have setdefault (or even different).
It seems possible in principle, but especially in combination with cached_property maybe/hopefully non-existing in practice. But if this is a concern, I guess this could check for dict or add another level of try/except...
There was a problem hiding this comment.
That level of try/except should only cover the operation that's expected to fail -- cache.setdefault, not the call itself.
There was a problem hiding this comment.
Done, and the other comments). In case anyone wonders, splitting the attribute makes it 22ns -> 36ns for me; but this is the slow path, so I assume it's OK and the setitem was also a bit similarly a bit quicker.
| val = self.func(instance) | ||
| try: | ||
| cache[self.attrname] = val | ||
| val = cache.setdefault(self.attrname, val) |
There was a problem hiding this comment.
That level of try/except should only cover the operation that's expected to fail -- cache.setdefault, not the call itself.
| except AttributeError: | ||
| msg = ( | ||
| f"The '__dict__' attribute on {type(instance).__name__!r} instance " | ||
| f"does not support item assignment for caching {self.attrname!r} property." |
There was a problem hiding this comment.
This is now setdefault rather than item assignment.
| PyAPI_FUNC(int) _PyDict_SetItem_KnownHash(PyObject *mp, PyObject *key, | ||
| PyObject *item, Py_hash_t hash); | ||
| // Same return codes as PyDict_SetDefaultRef (0 inserted, 1 present, -1 error). | ||
| extern int _PyDict_SetDefaultRef_KnownHash(PyObject *mp, PyObject *key, |
There was a problem hiding this comment.
This should be PyAPI_FUNC(int) so it's exported properly.
…e-150708.AESsFo.rst Co-authored-by: Petr Viktorin <encukou@gmail.com>
2 participants
When running multiple threads, we don't want to guarantee call-once style behavior for functools.cache or cached_property. That could be convenient but is not strictly needed.
However, we should use
dict.setdefault()to ensure that:obj.cached_property is obj.cached_property
and:
cached_fuction(arg) is cached_function(arg)
Both may be run twice (and for statistics purposes we may return a cached value but still have a miss).
I confirmed tests were failing locally before the fixes. I added it to the free-threaded tests, because it seemed similar ones, since we release the GIL this behavior is not limited to free-threaded Python though.
(I have used an agent mostly to get the dict changes started, but it looks fine to me -- and more minimal than I had hoped for. Created at EuroPython sprints.)
dict.setdefault()in functools caching for better thread-safety #150708