Memleak in UniqueRepresentation, @cached_method

[vbraun]

The documentation says that UniqueRepresentation uses weak refs, but this was switched over to the @cached_method decorator. The latter does currently use strong references, so unused unique parents stay in memory forever:

import sage.structure.unique_representation
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)

for i in range(2,1000):
    ring = ZZ.quotient(ZZ(i))
    vectorspace = ring^2

import gc
gc.collect()
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)

Related tickets:

• Use weak references to cache homsets #11521 (needs review, introducing weak references for caching homsets), and
• Parents probably not reclaimed due to too much caching #715 (needs review, using weak references for caching coerce maps).
• Weak references in Polynomial Ring cache #5970 (the polynomial rings cache use strong references, which may now be a duplicate, as I introduce the weak cache in Parents probably not reclaimed due to too much caching #715)

Further notes:

• not everything in Python can be weakref'ed, for example None cannot.
• some results that are expensive to compute should not just be cached by a weak reference. Perhaps there is place for a permanent cache, or maybe some minimal age before garbage collecting it.

Apply

• attachment: trac12215_weak_cached_function_combined.patch
• attachment: trac12215_safe_callback.patch

CC: @simon-king-jena @jdemeyer @mwhansen @vbraun @jpflori

Component: memleak

Keywords: UniqueRepresentation cached_method caching

Author: Simon King

Reviewer: Nils Bruin

Merged: sage-5.7.beta1

Issue created by migration from https://trac.sagemath.org/ticket/12215

[simon-king-jena]

Description changed:

--- 
+++ 
@@ -15,6 +15,7 @@
 Related tickets:
 * #11521 (needs review, introducing weak references for caching homsets), and
 * #715 (needs a lot of work, eventually aiming at using weak references for caching coerce maps). 
+* #5970 (the polynomial rings cache use strong references)
 
 Further notes:
 * not everything in Python can be weakref'ed, for example ``None`` cannot.

[simon-king-jena]

comment:3

See my comment at #5970: It seems that having a weak version of cached_function (which is used to decorate UniqueRepresentation.__classcall__ is the missing bit (in addition to #11521 and #715 and a two-line change in the polynomial ring constructor) for fixing the issues at #5970.

I think this should be done on top of #11115, which rewrites cached methods and already has a positive review.

[simon-king-jena]

Dependencies: #11115

[simon-king-jena]

comment:5

Here is a patch. It isn't tested yet.

[simon-king-jena]

comment:6

... and I immediately updated the patch: Join categories were not using unique representation but cached_function (by #11900). So, that had to change.

[simon-king-jena]

Changed dependencies from #11115 to #11115 #11900

[simon-king-jena]

comment:7

Sorry, it was impossible to use weak_cached_function on the join function in sage.categories.category, since it may return a list (not weakly referenceable). Hence, I had to work around. With the attached patch (applied on top of #11900 and its dependencies), sage at least starts...

[simon-king-jena]

comment:8

It turns out that all the patches can still not fix the problem. We also have to deal with sage.structure.factory.UniqueFactory.

I suggest to add an option to UniqueFactory, that decides whether a strong or a weak cache is used. And I suggest to do this here, because I don't want to create yet another ticket.

The applications of UniqueFactory should mainly be in cases where weak references work. Therefore I suggest to use the weak cache by default - I am curious how many doc tests will fail...

Coercion sucks.

[simon-king-jena]

comment:9

It turns out that UniqueFactory already was somehow using weak references, but in an improper way. The new patch version replaces that by WeakValueDictionary.

It doesn't solve the problem, though.

[simon-king-jena]

comment:10

I have slightly updated my patch, so that there is no conflict with #11935.