Weakly reference binary operation codomains

[robertwb]

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

CC: @nbruin @simon-king-jena @jpflori

Component: memleak

Author: Robert Bradshaw, Nils Bruin

Branch: 13cb2a7

Reviewer: Simon King, Frédéric Chapoton, Jean-Pierre Flori, Sébastien Labbé

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

[simon-king-jena]

comment:43

Sigh. #13447 is not fixed, as the following example shows.

sage: import gc
sage: _ = gc.collect()
sage: from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular
sage: from sage.libs.singular.groebner_strategy import GroebnerStrategy
sage: n = len([x for x in gc.get_objects() if isinstance(x,MPolynomialRing_libsingular)])
sage: P = MPolynomialRing_libsingular(GF(541), 2, ('x', 'y'), TermOrder('degrevlex', 2))
sage: strat = GroebnerStrategy(Ideal([P.gen(0) + P.gen(1)]))
sage: del strat
sage: del P
sage: _ = gc.collect()
sage: n == len([x for x in gc.get_objects() if isinstance(x,MPolynomialRing_libsingular)])
False

I'd suggest to proceed as follows. Provided that all tests pass, I can review Robert's and Nils' patches, someone else reviews my additions (which are the merging and the latest commit), and then we make this a dependency for #13447.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 64b572c to 9793dbc

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

9793dbc

Make one test more stable

[simon-king-jena]

comment:45

Sometimes, the following test in sage/categories/fields.py has failed:

            sage: import gc
            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()
            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

The reason for the failure: Sometimes the last line gave 0, not 1. It is not totally clear to me why the different results occurred. But in fact 0 is better than 1 here (one additional ring got collected). So, I hope my change is OK.

[simon-king-jena]

comment:46

I am giving a positive review to Robert's code and Nils' doctest. I consider my contribution too small to make me author, but I think my changes need a review.

[pjbruin]

comment:47

Replying to @simon-king-jena:

Sometimes, the following test in sage/categories/fields.py has failed:

            sage: import gc
            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()
            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

The reason for the failure: Sometimes the last line gave 0, not 1. It is not totally clear to me why the different results occurred. But in fact 0 is better than 1 here (one additional ring got collected).

Shouldn't the result always be 1 given that R refers to Zmod(97) after the loop (unless there is a surviving reference from a Zmod(97) created in some earlier test)? What happens if you do del R before the last gc.collect()?

[simon-king-jena]

comment:48

Replying to @pjbruin:

Shouldn't the result always be 1 given that R refers to Zmod(97) after the loop (unless there is a surviving reference from a Zmod(97) created in some earlier test)?

Exactly. And what's strange: It isn't reproducible. Sometimes I get 1, sometimes I get 0. And I always get 1 when I run it on the command line.

What happens if you do del R before the last gc.collect()?

No idea.

[simon-king-jena]

comment:49

To be precise: I saw this problem precisely once. When I run the previous test about 20 times, it always passes.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

2c04029

make a test against a memory leak clearer/more stable

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 9793dbc to 2c04029

[simon-king-jena]

comment:51

Hopefully the new version of the test is reliable.

[fchapoton]

comment:52

excerpt from patchbot report:

+inside file:  b/src/sage/structure/coerce.pyx
+@@ -1265,33 +1287,74 @@ cdef class CoercionModel_cache_maps(CoercionModel):
++                    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"
+Old-style raise statement inserted on 2 non-empty lines

[simon-king-jena]

comment:53

Replying to @fchapoton:

excerpt from patchbot report:

+inside file:  b/src/sage/structure/coerce.pyx
+@@ -1265,33 +1287,74 @@ cdef class CoercionModel_cache_maps(CoercionModel):
++                    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"
+Old-style raise statement inserted on 2 non-empty lines

Actually this was not inserted, but moved. Would it be possible for you to change this to new-style raise statement by a reviewer commit?

[fchapoton]

Changed commit from 2c04029 to 8713960

[fchapoton]

comment:54

done. I also did it for some of the other raise in the same file.

---

New commits:

e976b79	Merge branch 'u/SimonKing/weakly_reference_binary_operation_codomains' into 6.9.b0
8713960	trac #14058 convert some raise statements into py3 syntax

[fchapoton]

Changed branch from u/SimonKing/weakly_reference_binary_operation_codomains to public/ticket/14058

[simon-king-jena]

comment:56

Replying to @simon-king-jena:

I am giving a positive review to Robert's code and Nils' doctest. I consider my contribution too small to make me author, but I think my changes need a review.

Could someone please finish the review? The patchbot thinks the branch is fine.

[jpflori]

Description changed:

--- 
+++ 
@@ -1,8 +1,3 @@
 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
-
-__APPLY__
-
-* [attachment: 14058-weak-binop-morphisms.patch](https://github.com/sagemath/sage-prod/files/10657065/14058-weak-binop-morphisms.patch.gz)
-* [attachment: trac_14058-doctest.patch](https://github.com/sagemath/sage-prod/files/10657064/trac_14058-doctest.patch.gz)

[jpflori]

comment:57

Your change is small and makes sense.

[jpflori]

Changed reviewer from Simon King to Simon King, Frédéric Chapoton, Jean-Pierre Flori

[videlec]

comment:58

From the patchbot

sage -t --long src/sage/structure/coerce.pyx
**********************************************************************
File "src/sage/structure/coerce.pyx", line 1307, in sage.structure.coerce.CoercionModel_cache_maps.coercion_maps
Failed example:
    print N2-N0
Expected:
    0
Got:
    -1

Is it what we should get?

[videlec]

comment:59

Replying to @videlec:

From the patchbot ...

wrong ticket... should be #18905. Sorry for the noise.

[simon-king-jena]

comment:60

Replying to @videlec:

Replying to @videlec:

From the patchbot ...

wrong ticket... should be #18905. Sorry for the noise.

The patchbot HERE does report this problem. So, what do you mean that it "should be #18905"?

[videlec]

comment:61

Right! I confused myself.

[videlec]

comment:62

The failing doctest is reproducible after applying this branch onto 6.9.beta1.

[seblabbe]

comment:63

On sage-6.9.beta1, I get:

$ sage nbruin.sage
Time: CPU 7.95 s, Wall: 7.95 s
Time: CPU 7.68 s, Wall: 7.68 s
Time: CPU 76.76 s, Wall: 76.94 s

On sage-6.9.beta1 + #14058, I get a very good improvement:

$ sage nbruin.sage
Time: CPU 2.48 s, Wall: 2.48 s
Time: CPU 2.07 s, Wall: 2.07 s
Time: CPU 32.38 s, Wall: 32.42 s

Note: I originally thought it was a regression in sage since I was getting this earlier this morning with sage-6.8 + #14058. But thanksfully, it seems to be a improvement obtained from this ticket.

The file nbruin.sage is from comment 4:

 $ cat nbruin.sage
a=ZZ(1)
b=QQ(1)
c=ZZ['x'](1)
d=b+c

def test(x,y):
  for i in xrange(10^6):
    _=x+y

time test(a,b)
time test(a,c)
time test(b,c)

[seblabbe]

comment:64

I also get improvements on tests above of Simon King at comments 18.

With sage-6.9.beta1:

$ sage testcoercion.sage
Time: CPU 1.69 s, Wall: 1.69 s
Time: CPU 4.05 s, Wall: 4.05 s
C
Time: CPU 4.03 s, Wall: 4.03 s

With sage-6.9.beta1 + this ticket:

$ sage testcoercion.sage
Time: CPU 0.34 s, Wall: 0.34 s
Time: CPU 0.85 s, Wall: 0.85 s
C
Time: CPU 0.86 s, Wall: 0.86 s

(My file testcoercion.sage contains testcoercion.py + the lines at comment 18.)

[seblabbe]

comment:65

With sage-6.9.beta1 + this ticket, I get All tests passed when I do

./sage -t --long src/sage/structure/coerce.pyx

but I get the problem when I test after the build

./sage -bt --long src/sage/structure/coerce.pyx

which gives the error:

**********************************************************************
File "src/sage/structure/coerce.pyx", line 1307, in sage.structure.coerce.CoercionModel_cache_maps.coercion_maps
Failed example:
    print N2-N0
Expected:
    0
Got:
    -1
**********************************************************************

I have never seen such a behavior.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8713960 to b73d537

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

c136ebd	Merge #14058 into 6.9.beta1
b73d537	Trac #14058: fix broken doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e153d08	#14058: Add doctest
b9141ee	Merge branch 'ticket/14058' into develop
90ed181	Trivial fix for a coercion doctest
64b572c	refcount libsingular rings used in plural
9793dbc	Make one test more stable
2c04029	make a test against a memory leak clearer/more stable
e976b79	Merge branch 'u/SimonKing/weakly_reference_binary_operation_codomains' into 6.9.b0
8713960	trac #14058 convert some raise statements into py3 syntax
c136ebd	Merge #14058 into 6.9.beta1
13cb2a7	Trac #14058: fix broken doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from b73d537 to 13cb2a7

[seblabbe]

comment:68

In other doctests in the same file, GF(3), GF(7) and GF(41) are also created. While there are references to GF(7) and GF(41), no variable references to GF(3) which is available for gargage collection. If GF(3) gets garbage collected before the offending doctest, then everything is fine, the doctest pass. But it can also be garbage collected in the call to gc.collect() in the doctest. If this is the case, then N0 was also counting GF(3) which explains why N2-N0 can be negative.

One way to fix this is to call gc.collect() before creating N0.

(my first fix was using sets of ids... sorry for the noise).

[simon-king-jena]

comment:69

Replying to @seblabbe:

One way to fix this is to call gc.collect() before creating N0.

I agree, that's the way to fix it.

[jpflori]

comment:71

I assume Simon's comment gives positive review here as this was the only thing to fix.

[jpflori]

Changed reviewer from Simon King, Frédéric Chapoton, Jean-Pierre Flori to Simon King, Frédéric Chapoton, Jean-Pierre Flori, Sébastien Labbé

[vbraun]

Changed dependencies from #12313 to none

[vbraun]

Changed branch from public/ticket/14058 to 13cb2a7

[jdemeyer]

Changed commit from 13cb2a7 to none

[jdemeyer]

comment:76

On arando:

sage -t --long src/sage/categories/fields.py
**********************************************************************
File "src/sage/categories/fields.py", line 104, in sage.categories.fields.Fields.__contains__
Failed example:
    len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) - n
Expected:
    0
Got:
    -1
**********************************************************************

[jdemeyer]

comment:77

See #19244