Trac #14058: Weakly reference binary operation codomains

R(1) + S(1) caches a strong reference to both R and S, which we'd like
to avoid.

See discussion at https://groups.google.com/forum/?fromgroups=#!topic
/sage-devel/ozhIHIwQ4WA

URL: http://trac.sagemath.org/14058
Reported by: robertwb
Ticket author(s): Robert Bradshaw, Nils Bruin
Reviewer(s): Simon King, Frédéric Chapoton, Jean-Pierre Flori, Sébastien
Labbé

src/sage/categories/fields.py

@@ -97,11 +97,12 @@ def __contains__(self, x):
             sage: _ = gc.collect()
             sage: n = len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)])
             sage: for i in prime_range(100):
-            ...     R = ZZ.quotient(i)
-            ...     t = R in Fields()
+            ....:     R = ZZ.quotient(i)
+            ....:     t = R in Fields()
+            sage: del R
             sage: _ = gc.collect()
             sage: len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) - n
-            1
+            0
 
         """
         try:

src/sage/categories/map.pxd

@@ -2,6 +2,8 @@ from sage.structure.parent cimport Parent
 from sage.structure.element cimport Element
 
 cdef class Map(Element):
+    cdef object __weakref__
+
     cdef public int _coerce_cost # a rough measure of the cost of using this morphism in the coercion system.
                           # 10 by default, 100 if a DefaultCoercionMorphism, 10000 if inexact.
 

src/sage/libs/singular/ring.pyx

@@ -446,12 +446,14 @@ cdef ring *singular_ring_reference(ring *existing_ring) except NULL:
 
     INPUT:
 
-    - ``existing_ring`` -- an existing Singular ring.
+    - ``existing_ring`` -- a Singular ring.
 
     OUTPUT:
 
-    The same ring with its refcount increased. After calling this
-    function `n` times, you need to call :func:`singular_ring_delete`
+    The same ring with its refcount increased. If ``existing_ring``
+    has not been refcounted yet, it will be after calling this function.
+    If initially ``existing_ring`` was refcounted once, then after
+    calling this function `n` times, you need to call :func:`singular_ring_delete`
     `n+1` times to actually deallocate the ring.
 
     EXAMPLE::
@@ -487,8 +489,7 @@ cdef ring *singular_ring_reference(ring *existing_ring) except NULL:
     if existing_ring==NULL:
         raise ValueError('singular_ring_reference(ring*) called with NULL pointer.')
     cdef object r = wrap_ring(existing_ring)
-    refcount = ring_refcount_dict.pop(r)
-    ring_refcount_dict[r] = refcount+1
+    ring_refcount_dict[r] = ring_refcount_dict.get(r,0)+1
     return existing_ring
 
 

src/sage/rings/polynomial/plural.pyx

@@ -114,7 +114,7 @@ from sage.libs.singular.function cimport RingWrap
 from sage.libs.singular.polynomial cimport (singular_polynomial_call, singular_polynomial_cmp, singular_polynomial_add, singular_polynomial_sub, singular_polynomial_neg, singular_polynomial_pow, singular_polynomial_mul, singular_polynomial_rmul, singular_polynomial_deg, singular_polynomial_str_with_changed_varnames, singular_polynomial_latex, singular_polynomial_str, singular_polynomial_div_coeff)
 
 import sage.libs.singular.ring
-from sage.libs.singular.ring cimport singular_ring_new, singular_ring_delete, wrap_ring
+from sage.libs.singular.ring cimport singular_ring_new, singular_ring_delete, wrap_ring, singular_ring_reference
 
 from sage.libs.singular.singular cimport si2sa, sa2si, overflow_check
 
@@ -327,7 +327,7 @@ cdef class NCPolynomialRing_plural(Ring):
         cdef RingWrap rw = ncalgebra(self._c, self._d, ring = P)
 
         #       rw._output()
-        self._ring = rw._ring
+        self._ring = singular_ring_reference(rw._ring)
         self._ring.ShortOut = 0
 
         self.__ngens = n

src/sage/structure/coerce.pyx

@@ -81,6 +81,8 @@ import operator
 cdef dict operator_dict = operator.__dict__
 from operator import add, sub, mul, div, truediv, iadd, isub, imul, idiv
 
+from cpython.weakref cimport PyWeakref_GET_OBJECT, PyWeakref_NewRef
+
 from sage_object cimport SageObject
 from sage.categories.map cimport Map
 import sage.categories.morphism
@@ -409,15 +411,35 @@ cdef class CoercionModel_cache_maps(CoercionModel):
             sage: cm = sage.structure.element.get_coercion_model()
             sage: maps, actions = cm.get_cache()
 
-        Now lets see what happens when we do a binary operations with
+        Now let us see what happens when we do a binary operations with
         an integer and a rational::
 
-            sage: left_morphism, right_morphism = maps[ZZ, QQ]
-            sage: print copy(left_morphism)
+            sage: left_morphism_ref, right_morphism_ref = maps[ZZ, QQ]
+
+        Note that by :trac:`14058` the coercion model only stores a weak
+        reference to the coercion maps in this case::
+
+            sage: left_morphism_ref
+            <weakref at ...; to 'sage.rings.rational.Z_to_Q' at ...
+            (RingHomset_generic_with_category._abstract_element_class)>
+
+        Moreover, the weakly referenced coercion map uses only a weak
+        reference to the codomain::
+
+            sage: left_morphism_ref()
+            (map internal to coercion system -- copy before use)
+            Natural morphism:
+              From: Integer Ring
+              To:   Rational Field
+
+        To get an actual valid map, we simply copy the weakly referenced
+        coercion map::
+                
+            sage: print copy(left_morphism_ref())
             Natural morphism:
               From: Integer Ring
               To:   Rational Field
-            sage: print right_morphism
+            sage: print right_morphism_ref
             None
 
         We can see that it coerces the left operand from an integer to a
@@ -490,7 +512,7 @@ cdef class CoercionModel_cache_maps(CoercionModel):
         The function _test_exception_stack is executing the following code::
 
             try:
-                raise TypeError, "just a test"
+                raise TypeError("just a test")
             except TypeError:
                 cm._record_exception()
         """
@@ -518,7 +540,7 @@ cdef class CoercionModel_cache_maps(CoercionModel):
             ['Traceback (most recent call last):\n  File "sage/structure/coerce.pyx", line ...TypeError: just a test']
         """
         try:
-            raise TypeError, "just a test"
+            raise TypeError("just a test")
         except TypeError:
             self._record_exception()
 
@@ -758,7 +780,7 @@ cdef class CoercionModel_cache_maps(CoercionModel):
                 all.append("Coercion on right operand via")
                 all.append(y_mor)
                 if res is not None and res is not y_mor.codomain():
-                    raise RuntimeError, ("BUG in coercion model: codomains not equal!", x_mor, y_mor)
+                    raise RuntimeError("BUG in coercion model: codomains not equal!", x_mor, y_mor)
                 res = y_mor.codomain()
             all.append("Arithmetic performed after coercions.")
             if op is div and isinstance(res, Parent):
@@ -1045,7 +1067,7 @@ cdef class CoercionModel_cache_maps(CoercionModel):
 
         # We should really include the underlying error.
         # This causes so much headache.
-        raise TypeError, arith_error_message(x,y,op)
+        raise TypeError(arith_error_message(x,y,op))
 
     cpdef canonical_coercion(self, x, y):
         r"""
@@ -1265,33 +1287,76 @@ cdef class CoercionModel_cache_maps(CoercionModel):
             True
             sage: parent(w+v) is W
             True
+
+        TESTS:
+
+        We check that with :trac:`14058`, parents are still eligible for
+        garbage collection after being involved in binary operations::
+
+            sage: import gc
+            sage: gc.collect() #random
+            852
+            sage: T=type(GF(2))
+            sage: N0=len(list(o for o in gc.get_objects() if type(o) is T))
+            sage: L=[ZZ(1)+GF(p)(1) for p in prime_range(2,50)]
+            sage: N1=len(list(o for o in gc.get_objects() if type(o) is T))
+            sage: print N1 > N0
+            True
+            sage: del L
+            sage: gc.collect() #random
+            3939
+            sage: N2=len(list(o for o in gc.get_objects() if type(o) is T))
+            sage: print N2-N0
+            0
+
         """
         try:
-            return self._coercion_maps.get(R, S, None)
-        except KeyError:
-            homs = self.discover_coercion(R, S)
-            if 0:
-                # This breaks too many things that are going to change
-                # in the new coercion model anyways.
-                # COERCE TODO: Enable it then.
-                homs = self.verify_coercion_maps(R, S, homs)
+            refs = self._coercion_maps.get(R, S, None)
+            if refs is None:
+                return None
+            R_map_ref, S_map_ref = refs
+            if R_map_ref is None:
+                S_map = <object>PyWeakref_GET_OBJECT(S_map_ref)
+                if S_map is not None:
+                    return None, S_map
+            elif S_map_ref is None:
+                R_map = <object>PyWeakref_GET_OBJECT(R_map_ref)
+                if R_map is not None:
+                    return R_map, None
             else:
-                if homs is not None:
-                    x_map, y_map = homs
-                    if x_map is not None and not isinstance(x_map, Map):
-                        raise RuntimeError, "BUG in coercion model: coerce_map_from must return a Map"
-                    if y_map is not None and not isinstance(y_map, Map):
-                        raise RuntimeError, "BUG in coercion model: coerce_map_from must return a Map"
-            if homs is None:
-                swap = None
+                R_map = <object>PyWeakref_GET_OBJECT(R_map_ref)
+                S_map = <object>PyWeakref_GET_OBJECT(S_map_ref)
+                if R_map is not None and S_map is not None:
+                    return R_map, S_map
+        except KeyError:
+            pass
+        homs = self.discover_coercion(R, S)
+        if 0:
+            # This breaks too many things that are going to change
+            # in the new coercion model anyways.
+            # COERCE TODO: Enable it then.
+            homs = self.verify_coercion_maps(R, S, homs)
+        else:
+            if homs is not None:
+                x_map, y_map = homs
+                if x_map is not None and not isinstance(x_map, Map):
+                    raise RuntimeError("BUG in coercion model: coerce_map_from must return a Map")
+                if y_map is not None and not isinstance(y_map, Map):
+                    raise RuntimeError("BUG in coercion model: coerce_map_from must return a Map")
+        if homs is None:
+            refs = None
+            swap = None
+        else:
+            R_map, S_map = homs
+            R_map_ref = None if R_map is None else PyWeakref_NewRef(R_map, None)
+            S_map_ref = None if S_map is None else PyWeakref_NewRef(S_map, None)
+            refs = R_map_ref, S_map_ref
+            if R_map is None and isinstance(S, Parent) and (<Parent>S).has_coerce_map_from(R):
+                swap = None, PyWeakref_NewRef((<Parent>S).coerce_map_from(R), None)
             else:
-                R_map, S_map = homs
-                if R_map is None and isinstance(S, Parent) and (<Parent>S).has_coerce_map_from(R):
-                    swap = None, (<Parent>S)._internal_coerce_map_from(R)
-                else:
-                    swap = S_map, R_map
-            self._coercion_maps.set(R, S, None, homs)
-            self._coercion_maps.set(S, R, None, swap)
+                swap = S_map_ref, R_map_ref
+        self._coercion_maps.set(R, S, None, refs)
+        self._coercion_maps.set(S, R, None, swap)
         return homs
 
     cpdef verify_coercion_maps(self, R, S, homs, bint fix=False):
@@ -1334,14 +1399,14 @@ cdef class CoercionModel_cache_maps(CoercionModel):
                 if connecting is not None:
                     R_map = R_map * connecting
             if R_map.domain() is not R:
-                raise RuntimeError, ("BUG in coercion model, left domain must be original parent", R, R_map)
+                raise RuntimeError("BUG in coercion model, left domain must be original parent", R, R_map)
         if S_map is not None and S_map.domain() is not S:
             if fix:
                 connecting = S_map.domain()._internal_coerce_map_from(S)
                 if connecting is not None:
                     S_map = S_map * connecting
             if S_map.domain() is not S:
-                raise RuntimeError, ("BUG in coercion model, right domain must be original parent", S, S_map)
+                raise RuntimeError("BUG in coercion model, right domain must be original parent", S, S_map)
         # Make sure the codomains are correct
         if R_map.codomain() is not S_map.codomain():
             if fix:
@@ -1353,7 +1418,7 @@ cdef class CoercionModel_cache_maps(CoercionModel):
                     if connecting is not None:
                         R_map = connecting * R_map
             if R_map.codomain() is not S_map.codomain():
-                raise RuntimeError, ("BUG in coercion model, codomains must be identical", R_map, S_map)
+                raise RuntimeError("BUG in coercion model, codomains must be identical", R_map, S_map)
         if isinstance(R_map, IdentityMorphism):
             R_map = None
         elif isinstance(S_map, IdentityMorphism):
@@ -1431,9 +1496,9 @@ cdef class CoercionModel_cache_maps(CoercionModel):
                 coerce_R = Z._internal_coerce_map_from(R)
                 coerce_S = Z._internal_coerce_map_from(S)
                 if coerce_R is None:
-                    raise TypeError, "No coercion from %s to pushout %s" % (R, Z)
+                    raise TypeError("No coercion from %s to pushout %s" % (R, Z))
                 if coerce_S is None:
-                    raise TypeError, "No coercion from %s to pushout %s" % (S, Z)
+                    raise TypeError("No coercion from %s to pushout %s" % (S, Z))
                 return coerce_R, coerce_S
             except Exception:
                 self._record_exception()
@@ -1716,5 +1781,3 @@ Original elements %r (parent %s) and %r (parent %s) and maps
 %s %r"""%( x_elt, y_elt, parent_c(x_elt), parent_c(y_elt),
             x, parent_c(x), y, parent_c(y),
             type(x_map), x_map, type(y_map), y_map)
-
-