Add support for subclasses in `@method_cache`
Closes #23
Test Roadmap
- [x] Subclass overrides with cache
- [x] Subclass overrides without cache
- [x] Multi-threaded resolution race
Important Notes
- @jaraco: the previous implementation added
cache_clearearly to the returned wrapper. That behavior wasn't documented nor tested, nor fully supported if the methods were special (__getitem__or__getattr__):
https://github.com/jaraco/jaraco.functools/blob/e4644e208414137c3c2618bd1c1c1dcf23a895bc/jaraco/functools/init.py#L90-L199
(notice how proxy without a cache_clear dummy method is returned immediately in _special_method_cache, breaking
.cache_clear attrgets on __getitem__ / __getattr__ method caches).
I suggest we remove this eager-cache_clear convenience entirely, as it would require us to rewrite the proposed solution to a full descriptor or use a subclass of built-in property, which is unnecessary otherwise.
OK, I think it's done. I haven't added the uncached API mentioned in the issue—we can add it upon request.
An extra optimization would be to add a possibility to inform the decorator the method is final, so we can fall back to old behavior with no hierarchy-based resolution in case there's no hierarchy involved.
The reality though is that the observed attribute lookup overhead is from negligible to ~2.5x.
In correct applications of method_cache this overhead should be outweighed magnitudes of times by retrieving cached method results.
sys.intern() very slightly improves the performance on my attribute lookup stress tests.
I suggest we remove this eager-
cache_clearconvenience entirely, as it would require us to rewrite the proposed solution to a full descriptor or use a subclass of built-inproperty, which is unnecessary otherwise.
This functionality was more than a convenience. It addressed a real-world use-case, where if the method wasn't present, it led to an interface inconsistency forcing the consumer to handle the divergent cases (https://github.com/jaraco/jaraco.functools/issues/18). I don't want to push that complication on the consumer. I don't recall and didn't link what the real-world use-case was, but I did encounter it in live code, so the requirement is likely still present.
I suggest we remove this eager-
cache_clearconvenience entirely, as it would require us to rewrite the proposed solution to a full descriptor or use a subclass of built-inproperty, which is unnecessary otherwise.This functionality was more than a convenience. It addressed a real-world use-case, where if the method wasn't present, it led to an interface inconsistency forcing the consumer to handle the divergent cases (#18). I don't want to push that complication on the consumer. I don't recall and didn't link what the real-world use-case was, but I did encounter it in live code, so the requirement is likely still present.
The idea of subclassing property doesn't sound too terrible. I'm also not opposed to a full-fledged descriptor.
This functionality was more than a convenience. It addressed a real-world use-case, where if the method wasn't present, it led to an interface inconsistency forcing the consumer to handle the divergent cases (#18).
Oh, then that's great! Because this rewrite makes resolution on attribute access, not on the first method call (that's why property is used in this change), therefore this is a non-issue from the instance level. :+1:
It could be a problem from class-level or class scope-level, but that is not the case in #18.
I asked for opinions on this implementation on Python Discord (#internals-and-peps).
I got some excellent input from @Sachaa-Thanasius and @JelleZijlstra (thanks!) about a similar case for functools.cached_property. I learned that double-checked locking can still be a performance bottleneck under heavy use, which led to the functools.cached_property's lock being entirely removed.
See also:
- https://github.com/python/cpython/issues/87634
- https://discuss.python.org/t/finding-a-path-forward-for-functools-cached-property/23757
- https://github.com/python/cpython/pull/101890
I'm leaning toward removing the lock (followed by a note on thread safety in the docs) as a lesson drawn from that case. What's your view on this @jaraco?
Marking as a draft until we reach consensus on the design and the preferred implementation.