From ac68d9b2d7624d32dc885f42bbd83d372fa8da5f Mon Sep 17 00:00:00 2001 From: Robert Bradshaw Date: Tue, 5 Feb 2013 20:23:09 +0000 Subject: [PATCH 1/8] Remove strong references to parents used in binary operations in the coercion model. --- src/sage/categories/map.pxd | 1 + src/sage/structure/coerce.pyx | 75 +++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/sage/categories/map.pxd b/src/sage/categories/map.pxd index 4491dd18994..bf1ae116230 100644 --- a/src/sage/categories/map.pxd +++ b/src/sage/categories/map.pxd @@ -2,6 +2,7 @@ from sage.structure.parent cimport Parent from sage.structure.element cimport Element cdef class Map(Element): + cdef object __weakref__ cdef Parent _domain cdef Parent _codomain diff --git a/src/sage/structure/coerce.pyx b/src/sage/structure/coerce.pyx index c0f14a48e4c..041604234f6 100644 --- a/src/sage/structure/coerce.pyx +++ b/src/sage/structure/coerce.pyx @@ -80,6 +80,8 @@ include "coerce.pxi" import operator +from cpython.weakref cimport PyWeakref_GET_OBJECT, PyWeakref_NewRef + from sage_object cimport SageObject from sage.categories.map cimport Map import sage.categories.morphism @@ -228,12 +230,12 @@ cdef class CoercionModel_cache_maps(CoercionModel): Now lets 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 left_morphism + sage: left_morphism_ref, right_morphism_ref = maps[ZZ, QQ] + sage: print 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 @@ -1063,31 +1065,52 @@ cdef class CoercionModel_cache_maps(CoercionModel): True """ 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 = PyWeakref_GET_OBJECT(S_map_ref) + if S_map is not None: + return None, S_map + elif S_map_ref is None: + R_map = 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 = PyWeakref_GET_OBJECT(R_map_ref) + S_map = 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 PY_TYPE_CHECK(S, Parent) and (S).has_coerce_map_from(R): + swap = None, PyWeakref_NewRef((S).coerce_map_from(R), None) else: - R_map, S_map = homs - if R_map is None and PY_TYPE_CHECK(S, Parent) and (S).has_coerce_map_from(R): - swap = None, (S).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): From e153d0813221d3681030fbf24bbbc283d3717162 Mon Sep 17 00:00:00 2001 From: Nils Bruin Date: Tue, 5 Feb 2013 17:35:25 +0000 Subject: [PATCH 2/8] #14058: Add doctest --- src/sage/structure/coerce.pyx | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/sage/structure/coerce.pyx b/src/sage/structure/coerce.pyx index 041604234f6..72e05958b64 100644 --- a/src/sage/structure/coerce.pyx +++ b/src/sage/structure/coerce.pyx @@ -1063,6 +1063,26 @@ 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: 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: refs = self._coercion_maps.get(R, S, None) From 90ed181ae9df152e99a652636a7e9dc29b466916 Mon Sep 17 00:00:00 2001 From: Simon King Date: Fri, 17 Jul 2015 17:29:34 +0200 Subject: [PATCH 3/8] Trivial fix for a coercion doctest --- src/sage/structure/coerce.pyx | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/sage/structure/coerce.pyx b/src/sage/structure/coerce.pyx index 6545aa4d256..381784d710c 100644 --- a/src/sage/structure/coerce.pyx +++ b/src/sage/structure/coerce.pyx @@ -414,7 +414,27 @@ cdef class CoercionModel_cache_maps(CoercionModel): Now lets see what happens when we do a binary operations with an integer and a rational:: - sage: left_morphism, right_morphism = maps[ZZ, QQ] + 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 + + + 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 From 64b572ccf6403be0c617d0e39de4b1e6cd5dbc58 Mon Sep 17 00:00:00 2001 From: Simon King Date: Sat, 18 Jul 2015 00:37:31 +0200 Subject: [PATCH 4/8] refcount libsingular rings used in plural --- src/sage/libs/singular/ring.pyx | 11 ++++++----- src/sage/rings/polynomial/plural.pyx | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/sage/libs/singular/ring.pyx b/src/sage/libs/singular/ring.pyx index a535bfc638e..5a920139ea0 100644 --- a/src/sage/libs/singular/ring.pyx +++ b/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 diff --git a/src/sage/rings/polynomial/plural.pyx b/src/sage/rings/polynomial/plural.pyx index 5983834ef69..6dc97afb04e 100644 --- a/src/sage/rings/polynomial/plural.pyx +++ b/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 From 9793dbc9bc64bfdce43ff1e626453816c302696d Mon Sep 17 00:00:00 2001 From: Simon King Date: Sat, 18 Jul 2015 08:17:32 +0200 Subject: [PATCH 5/8] Make one test more stable --- src/sage/categories/fields.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sage/categories/fields.py b/src/sage/categories/fields.py index 95b83089269..e9f946f123c 100644 --- a/src/sage/categories/fields.py +++ b/src/sage/categories/fields.py @@ -97,11 +97,11 @@ 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: _ = 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 + sage: len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) - n <= 1 + True """ try: From 2c04029f9a194bd87befa0cd99016e2f267a836f Mon Sep 17 00:00:00 2001 From: Simon King Date: Tue, 21 Jul 2015 15:18:25 +0200 Subject: [PATCH 6/8] make a test against a memory leak clearer/more stable --- src/sage/categories/fields.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sage/categories/fields.py b/src/sage/categories/fields.py index e9f946f123c..054b1d194d6 100644 --- a/src/sage/categories/fields.py +++ b/src/sage/categories/fields.py @@ -99,9 +99,10 @@ def __contains__(self, x): sage: for i in prime_range(100): ....: 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 - True + sage: len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) - n + 0 """ try: From 8713960717f916d459e0b6a0dcef391dc992d9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Chapoton?= Date: Tue, 4 Aug 2015 10:52:23 +0200 Subject: [PATCH 7/8] trac #14058 convert some raise statements into py3 syntax --- src/sage/structure/coerce.pyx | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/sage/structure/coerce.pyx b/src/sage/structure/coerce.pyx index 381784d710c..87d2f7ab505 100644 --- a/src/sage/structure/coerce.pyx +++ b/src/sage/structure/coerce.pyx @@ -411,7 +411,7 @@ 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_ref, right_morphism_ref = maps[ZZ, QQ] @@ -512,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() """ @@ -540,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() @@ -780,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): @@ -1067,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""" @@ -1338,9 +1338,9 @@ cdef class CoercionModel_cache_maps(CoercionModel): 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" + 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" + raise RuntimeError("BUG in coercion model: coerce_map_from must return a Map") if homs is None: refs = None swap = None @@ -1397,14 +1397,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: @@ -1416,7 +1416,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): @@ -1494,9 +1494,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() @@ -1779,5 +1779,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) - - From 13cb2a78983efb8f4edb00d6d10f8fc7da62e80d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Labb=C3=A9?= Date: Wed, 12 Aug 2015 13:40:19 +0200 Subject: [PATCH 8/8] Trac #14058: fix broken doctest --- src/sage/structure/coerce.pyx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sage/structure/coerce.pyx b/src/sage/structure/coerce.pyx index 87d2f7ab505..338c12dd192 100644 --- a/src/sage/structure/coerce.pyx +++ b/src/sage/structure/coerce.pyx @@ -1294,6 +1294,8 @@ cdef class CoercionModel_cache_maps(CoercionModel): 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)]