jaraco.functools icon indicating copy to clipboard operation
jaraco.functools copied to clipboard

Add support for subclasses in `@method_cache`

Open johnslavik opened this issue 5 months ago • 8 comments

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_clear early 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.

johnslavik avatar Jun 27 '25 15:06 johnslavik

OK, I think it's done. I haven't added the uncached API mentioned in the issue—we can add it upon request.

johnslavik avatar Jun 27 '25 20:06 johnslavik

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.

johnslavik avatar Jun 27 '25 22:06 johnslavik

sys.intern() very slightly improves the performance on my attribute lookup stress tests.

johnslavik avatar Jun 27 '25 22:06 johnslavik

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.

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.

jaraco avatar Jun 28 '25 13:06 jaraco

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.

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.

jaraco avatar Jun 28 '25 13:06 jaraco

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.

johnslavik avatar Jun 29 '25 05:06 johnslavik

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

image

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?

johnslavik avatar Jun 30 '25 11:06 johnslavik

Marking as a draft until we reach consensus on the design and the preferred implementation.

johnslavik avatar Jun 30 '25 11:06 johnslavik