Sage: Ticket #11521: Memleak when resolving the action of Integers on an Elliptic Curve
https://trac.sagemath.org/ticket/11521
<p>
The following piece of code leaks memory. <br /> <br /> sage: K = GF(1<<55,'t') <br /> sage: a = K.random_element() <br /> sage: while 1: <br /> ....: E = EllipticCurve(j=a); P = E.random_point(); 2*P;
</p>
<p>
The problem seems to occur while resolving the action of ZZ on E.
</p>
<p>
It does not happen if:
</p>
<ul><li>one does not change the curve in the loop
</li></ul><ul><li>does P+P instead of a multiplication
</li></ul><p>
<strong>Apply</strong>
</p>
<p>
<a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11521/trac11521_triple_homset.patch" title="Attachment 'trac11521_triple_homset.patch' in Ticket #11521">trac11521_triple_homset.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11521/trac11521_triple_homset.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/11521
Trac 1.1.6jpfloriFri, 01 Jul 2011 10:28:33 GMT
https://trac.sagemath.org/ticket/11521#comment:1
https://trac.sagemath.org/ticket/11521#comment:1
<p>
After looking at <a class="closed ticket" href="https://trac.sagemath.org/ticket/10548" title="defect: The coercion model is keeping references to tracebacks which causes ... (closed: fixed)">#10548</a>, I might have a better idea of the culprit:
</p>
<pre class="wiki">sage: import gc
sage: from sage.schemes.elliptic_curves.ell_finite_field import EllipticCurve_finite_field
sage: K = GF(1<<60,'t')
sage: j = K.random_element()
sage: for i in xrange(100):
....: E = EllipticCurve(j=j); P = E.random_point(); 2*P;
....:
sage: gc.collect()
440
sage: len([x for x in gc.get_objects() if isinstance(x,EllipticCurve_finite_field)])
100
</pre>
TicketjpfloriFri, 01 Jul 2011 10:35:53 GMTowner, component changed
https://trac.sagemath.org/ticket/11521#comment:2
https://trac.sagemath.org/ticket/11521#comment:2
<ul>
<li><strong>owner</strong>
changed from <em>rlm</em> to <em>robertwb</em>
</li>
<li><strong>component</strong>
changed from <em>memleak</em> to <em>coercion</em>
</li>
</ul>
<p>
So this could be just <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> .
</p>
TicketjpfloriFri, 01 Jul 2011 13:29:05 GMT
https://trac.sagemath.org/ticket/11521#comment:3
https://trac.sagemath.org/ticket/11521#comment:3
<p>
This is definitely <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
<p>
Resetting the coercion cache and calling gc.collect() deletes the cached elements.
</p>
<p>
I guess weak refs should be used in the different <a class="missing wiki">TripleDict?</a> objects of !CoercionModel_cache_maps.
</p>
<p>
So this should be closed as duplicate/won't fix.
</p>
TicketjpfloriTue, 13 Sep 2011 12:56:49 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:4
https://trac.sagemath.org/ticket/11521#comment:4
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketzimmermaThu, 13 Oct 2011 11:17:03 GMT
https://trac.sagemath.org/ticket/11521#comment:5
https://trac.sagemath.org/ticket/11521#comment:5
<p>
Jean-Pierre, why did you change the status to "needs review"? There is no patch to review.
</p>
<p>
Also, how to you reset the coercion cache? I would be interested if you have a workaround for the
memory leak in:
</p>
<pre class="wiki">for p in prime_range(10^8):
k = GF(p)
</pre><p>
Paul
</p>
TicketjpfloriThu, 13 Oct 2011 11:26:16 GMT
https://trac.sagemath.org/ticket/11521#comment:6
https://trac.sagemath.org/ticket/11521#comment:6
<p>
As far as I'm concerned, this is nothing but a concrete example of the vague <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
So I guess I put it to "needs review" so that it could be closed as "duplicate/won't fix".
Not sure it was the right way to do it.
</p>
<p>
I seem to remember that I had some workarounds to delete some parts of the cache (so that I could perform my computations), but not all of them.
In fact there are several dictionnaries hidden in different places.
It was quite a while ago, but I'll have another look at it at some point.
Anyway, using weak references for all these dictionnaries seems to be a quite non trivial task.
Moreover it should also not slow things down too much to be viable...
</p>
TicketzimmermaThu, 13 Oct 2011 11:30:11 GMT
https://trac.sagemath.org/ticket/11521#comment:7
https://trac.sagemath.org/ticket/11521#comment:7
<p>
for the computations I need to perform, which need to create about 200000 prime fields, this
memory leak makes it impossible to perform it with Sage, which eats all the available memory.
</p>
<p>
I would be satisfied if I had a magic command to type to explicitly free the memory used by
every <code>k=GF(p)</code>.
</p>
<p>
Paul
</p>
TicketjpfloriFri, 14 Oct 2011 12:52:20 GMT
https://trac.sagemath.org/ticket/11521#comment:8
https://trac.sagemath.org/ticket/11521#comment:8
<p>
I'm having a look at your problem right now. Here are some hints to study the problem, mostly stolen from <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=ticket%3A10548"><span class="icon"></span>#10548</a>.
</p>
<p>
I put it here for the record, and so that i can go faster next time.
</p>
<p>
First, if I only create the finite fields and do nothing with them, I do not seem to get a memleak. Some fields are not garbage collected immediately, but calling gc.collect() does the trick.
</p>
<pre class="wiki">sage: import gc
sage: for p in prime_range(10**5):
....: k = GF(p)
....:
sage: del p, k
sage: gc.collect()
1265
sage: from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_modn as FF
sage: L = [x for x in gc.get_objects() if isinstance(x, FF)]
sage: len(L)
1
sage: L
[Finite Field of size 2]
</pre><p>
Of course I guess you actually do something with your finite fields.
</p>
<p>
So here is a small example causing the fields to stay cached.
</p>
<pre class="wiki">sage: import gc
sage: for p in prime_range(10**5):
....: k = GF(p)
....:
sage: del p, k
sage: gc.collect()
0
</pre><p>
The zero here is bad and indeed
</p>
<pre class="wiki">sage: from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_modn as FF
sage: L = [x for x in gc.get_objects() if isinstance(x, FF)]
sage: len(L)
9592
</pre><p>
If you want to know where it comes from you can use the objgraph python module (on my debian I just installed the python-objgraph package, updated sys.path in Sage to include '/usr/lib/python2.6/dist-packages')
</p>
<pre class="wiki">sage: sys.path.append('/usr/lib/python2.6/dist-packages')
sage: import inspect, objgraph
sage: objgraph.show_backrefs(L[-1],filename='/home/jp/br.png',extra_ignore=[id(L)])
</pre><p>
And look at the png or use
</p>
<pre class="wiki">sage: brc = objgraph.find_backref_chain(L[-1],inspect.ismodule,max_depth=15,extra_ignore=[id(L)])
sage: map(type,brc)
[<type 'module'>, <type 'dict'>, <type 'dict'>,...
sage: brc[0]
<module 'sage.categories.homset'...
</pre><p>
So the culprit is "_cache" in sage.categories.homset which has a direct reference to the finite fields in its keys.
</p>
<p>
The clean solution woul be to used weakref in the keys (let's say something as <a class="missing wiki">WeakKeyDirectory?</a> in python).
</p>
<p>
However, resetting cache should be a (potentially partial) workaround (and could kill your Sage?).
</p>
<pre class="wiki">sage: sage.categories.homset.cache = {}
sage: gc.collect()
376595
</pre><p>
It also seems to be enough if I do "a = k.random_element(); a = a+a" in the for loop, but not if I add "a = 47*a+3".
</p>
<p>
I'm currently investigating that last case.
</p>
TicketjpfloriFri, 14 Oct 2011 13:12:50 GMT
https://trac.sagemath.org/ticket/11521#comment:9
https://trac.sagemath.org/ticket/11521#comment:9
<p>
For info, using "k(47)*a+k(3)" is solved, so the problem left really comes from coercion and action of integers.
</p>
<pre class="wiki">sage: cm = sage.structure. get_coercion_model()
sage: cm.reset_cache()
</pre><p>
does not help.
</p>
TicketjpfloriFri, 14 Oct 2011 15:02:56 GMT
https://trac.sagemath.org/ticket/11521#comment:10
https://trac.sagemath.org/ticket/11521#comment:10
<p>
First, the second example above is missing the line "k(1);" in the for loop, otherwise it does nothing more than the first example.
</p>
<p>
Second, I guess the remaining references to the finite fields are in the different lists and dictionnaries of the integer ring named _coerce_from_list, _convert_from_list etc.
</p>
<p>
You can not directly access them from Python level, but there a function _introspect_coerce() (defined in parent.pyx) which returns them.
</p>
TicketjpfloriFri, 14 Oct 2011 15:48:37 GMT
https://trac.sagemath.org/ticket/11521#comment:11
https://trac.sagemath.org/ticket/11521#comment:11
<p>
In fact, everything is in _*_hash.
</p>
<p>
And to conclude, I'd say that right now you can not directly delete entries in this dictionaries from the Python level.
</p>
<p>
So for minimum changes, you should eitheir avoid coercion, or make the dictionaries "cdef public" in parent.pxd to be able to explicitely delete every created entries (be aware that it could be written in different places for example in ZZ but also in QQ and CC and I don't know where...).
</p>
<p>
And I could also have missed other dictionaries used by Sage.
</p>
TicketzimmermaFri, 14 Oct 2011 15:51:25 GMT
https://trac.sagemath.org/ticket/11521#comment:12
https://trac.sagemath.org/ticket/11521#comment:12
<p>
Jean-Pierre, I cannot reproduce that:
</p>
<pre class="wiki">sage: sys.path.append('/usr/lib/python2.6/dist-packages')
sage: import inspect, objgraph
---------------------------------------------------------------------------
ImportError Traceback (most recent call last)
/users/caramel/zimmerma/Adm/Strass/SED/2011/<ipython console> in <module>()
ImportError: No module named objgraph
</pre><p>
Paul
</p>
TicketjpfloriFri, 14 Oct 2011 15:54:09 GMT
https://trac.sagemath.org/ticket/11521#comment:13
https://trac.sagemath.org/ticket/11521#comment:13
<p>
Did you "apt-get install python-objgraph" on your system?
</p>
<p>
If yes, is it a version for python 2.6 ?
</p>
TicketjpfloriFri, 14 Oct 2011 15:56:56 GMT
https://trac.sagemath.org/ticket/11521#comment:14
https://trac.sagemath.org/ticket/11521#comment:14
<p>
The path I gave above might also be different on your system...
</p>
<p>
As the package manager.
</p>
TicketjpfloriMon, 07 Nov 2011 09:28:41 GMT
https://trac.sagemath.org/ticket/11521#comment:15
https://trac.sagemath.org/ticket/11521#comment:15
<p>
Any progress on your side?
</p>
<p>
If you found any other dicitonaries leading to cahing problems, it would be great to mention them here for the record.
</p>
<p>
Hence the day someone will finally decide to tackle ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, it will speed up the search of the culprit.
</p>
TicketzimmermaMon, 07 Nov 2011 09:37:16 GMT
https://trac.sagemath.org/ticket/11521#comment:16
https://trac.sagemath.org/ticket/11521#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:15" title="Comment 15">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Any progress on your side?
</p>
</blockquote>
<p>
no time so far. I will look at this during the <a class="missing wiki">SageFlint?</a> days in December, unless someone else has
some time before.
</p>
<p>
Paul
</p>
TicketjohanbosmanMon, 14 Nov 2011 16:34:56 GMT
https://trac.sagemath.org/ticket/11521#comment:17
https://trac.sagemath.org/ticket/11521#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:6" title="Comment 6">jpflori</a>:
</p>
<blockquote class="citation">
<p>
As far as I'm concerned, this is nothing but a concrete example of the vague <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
So I guess I put it to "needs review" so that it could be closed as "duplicate/won't fix".
Not sure it was the right way to do it.
</p>
</blockquote>
<p>
If you think it should be closed, then I think you should set the milestone to duplicate/wontfix. Otherwise, it is probably better to change the status back to 'new'.
</p>
TicketzimmermaSun, 18 Dec 2011 16:22:34 GMT
https://trac.sagemath.org/ticket/11521#comment:18
https://trac.sagemath.org/ticket/11521#comment:18
<p>
with Sage 4.7.2 I get the following:
</p>
<pre class="wiki">sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: import gc
sage: gc.collect()
0
sage: from sage.rings.finite_rings.finite_field_prime_modn import \
....: FiniteField_prime_modn as FF
sage: L = [x for x in gc.get_objects() if isinstance(x, FF)]
sage: len(L), L[0], L[len(L)-1]
(9592, Finite Field of size 2, Finite Field of size 99767)
</pre><p>
thus whenever we use the finite field we get a memleak.
(If I remove the <code>a=K(0)</code> line, I get only two elements in L, for p=2 and p=99991.)
</p>
<p>
Paul
</p>
TicketzimmermaSun, 18 Dec 2011 19:18:57 GMT
https://trac.sagemath.org/ticket/11521#comment:19
https://trac.sagemath.org/ticket/11521#comment:19
<p>
I confirm (cf comment <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:8" title="Comment 8">8</a>) that if I comment out the line
</p>
<pre class="wiki"> _cache[(X, Y, category)] = weakref.ref(H)
</pre><p>
in categories/homset.py, then the memory leak from comment <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:18" title="Comment 18">18</a> disappears.
</p>
<p>
Paul
</p>
TicketSimonKingMon, 19 Dec 2011 09:41:56 GMT
https://trac.sagemath.org/ticket/11521#comment:20
https://trac.sagemath.org/ticket/11521#comment:20
<p>
I think we have a different problem here.
</p>
<p>
The finite fields themselves should be cached. The cache (see <code>GF._cache</code>) uses weak references, which should be fine.
</p>
<p>
Also, there are special methods <code>zero_element</code> and <code>one_element</code> which should do caching, because zero and one are frequently used and should not be created over and over again.
</p>
<p>
However, it seems that <em>all</em> elements of a finite field are cached - and that's bad!
</p>
<pre class="wiki">sage: K = GF(5)
sage: K(2) is K(2)
True
sage: K.<a> = GF(17^2)
sage: K(5) is K(5)
True
</pre><p>
I see absolutely no reason why all <code>17^2</code> elements should be cached.
</p>
<p>
Fortunately, we have no caching for larger finite fields:
</p>
<pre class="wiki">sage: K.<a> = GF(17^10)
sage: K(5) is K(5)
False
</pre>
TicketSimonKingMon, 19 Dec 2011 09:58:20 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:21
https://trac.sagemath.org/ticket/11521#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
In the following example, there is no memory leak that would be caused by the <code>sage.categories.homset</code> cache:
</p>
<pre class="wiki">sage: len(sage.categories.homset._cache.keys())
100
sage: for p in prime_range(10^5):
....: K = GF(p)
....:
sage: len(sage.categories.homset._cache.keys())
100
</pre><p>
However, when you do a conversion <code>K(...)</code> then a convert map is created, and apparently is cached:
</p>
<pre class="wiki">sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: len(sage.categories.homset._cache.keys())
9692
</pre><p>
The homset cache does use weak references. Hence, it is surprising that the unused stuff can not be garbage collected. Apparently there is some direct reference involved at some point.
</p>
<p>
I am very stronglyagainst removing the cache of <code>sage.categories.homset</code>. Namely, elliptic curve code uses finite fields and maps involving finite fields <em>a lot</em>, and killing the cache is likely to cause a massive regression.
</p>
<p>
However, since we actually have coercion maps (not just any odd map), I expect that the direct reference comes from the coercion model. I suggest to look into the coercion code and use weak references there.
</p>
<p>
By the way, I don't know why the status is "needs review". I think it clearly is "needs work".
</p>
TicketSimonKingMon, 19 Dec 2011 10:02:29 GMT
https://trac.sagemath.org/ticket/11521#comment:22
https://trac.sagemath.org/ticket/11521#comment:22
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:8" title="Comment 8">jpflori</a>:
</p>
<blockquote class="citation">
<p>
So the culprit is "_cache" in sage.categories.homset which has a direct reference to the finite fields in its keys.
</p>
</blockquote>
<p>
Oops, that could indeed be the problem! Namely, the homset cache uses weak references to its values, but uses direct references to its keys! Perhaps using weak references as keys would work?
</p>
TicketzimmermaMon, 19 Dec 2011 10:21:48 GMT
https://trac.sagemath.org/ticket/11521#comment:23
https://trac.sagemath.org/ticket/11521#comment:23
<p>
it seems the complete caching of field elements only occurs for p < 500:
</p>
<pre class="wiki">sage: K=GF(499)
sage: K(5) is K(5)
True
sage: K=GF(503)
sage: K(5) is K(5)
False
</pre><p>
A workaround to this memory leak is to free the cache from time to time (thanks Simon):
</p>
<pre class="wiki">sage: sage.categories.homset._cache.clear()
</pre><p>
Paul
</p>
TicketSimonKingMon, 19 Dec 2011 10:26:14 GMT
https://trac.sagemath.org/ticket/11521#comment:24
https://trac.sagemath.org/ticket/11521#comment:24
<p>
On the other hand, it could be that using weak keys in the homset cache will not work: The keys are triples: domain, codomain and category.
</p>
<p>
What we want is: If either the domain or the codomain or the category have no strong reference, then the homset can be garbage collected.
</p>
<p>
Hence: Why don't we use a dictionary of dictionaries of dictionaries?
</p>
<p>
What I mean is:
</p>
<ul><li>The keys of sage.categories.homset._cache should be weak references to the domain
</li><li>The values of sage.categories.homset._cache should be dictionaries whose keys are weak references to the codomain.
</li><li>The keys of these dictionaries should be weak references to the category, and the value a weak reference to the homset.
</li></ul><p>
Hence, if there is no strong reference to either domain or codomain or category, then the homset can be deallocated.
</p>
TicketSimonKingMon, 19 Dec 2011 10:49:48 GMT
https://trac.sagemath.org/ticket/11521#comment:25
https://trac.sagemath.org/ticket/11521#comment:25
<p>
The idea sketched in the previous comment seems to work!!!
</p>
<p>
With it, after running
</p>
<pre class="wiki">sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....: print get_memory_usage()
</pre><p>
ends with printing 825.45703125
</p>
<p>
Without it, it ends with printing 902.8125
</p>
<p>
I don't know if we should ban caching of field elements?
</p>
<p>
How could fixing that memory leak be demonstrated by a doc test?
</p>
<p>
Anyway, I will post a preliminary patch in a few minutes (so that you can see if it fixes at least part of the problem for you), while I am running <code>sage -tp 2 devel/sage</code>.
</p>
TicketSimonKingMon, 19 Dec 2011 10:51:05 GMT
https://trac.sagemath.org/ticket/11521#comment:26
https://trac.sagemath.org/ticket/11521#comment:26
<p>
Patch's up!
</p>
TicketSimonKingMon, 19 Dec 2011 10:56:28 GMT
https://trac.sagemath.org/ticket/11521#comment:27
https://trac.sagemath.org/ticket/11521#comment:27
<p>
Hm. There seems to be a problem.
</p>
<pre class="wiki">sage -t devel/sage/doc/en/constructions/linear_codes.rst
The doctested process was killed by signal 11
</pre><p>
What is signal 11?
</p>
TicketzimmermaMon, 19 Dec 2011 11:01:26 GMT
https://trac.sagemath.org/ticket/11521#comment:28
https://trac.sagemath.org/ticket/11521#comment:28
<p>
signal 11 is Segmentation Fault
</p>
<p>
Paul
</p>
TicketSimonKingMon, 19 Dec 2011 11:06:55 GMT
https://trac.sagemath.org/ticket/11521#comment:29
https://trac.sagemath.org/ticket/11521#comment:29
<p>
Indeed it is a segfault. And it is triggered by
</p>
<pre class="wiki">sage: w = vector([1,1,-4])
</pre>
TicketSimonKingMon, 19 Dec 2011 11:14:47 GMT
https://trac.sagemath.org/ticket/11521#comment:30
https://trac.sagemath.org/ticket/11521#comment:30
<p>
Monique van Beek just pointed me to the fact that the error occurs even earlier:
</p>
<pre class="wiki">sage: is_Ring(None)
<BOOOOOM>
</pre>
TicketSimonKingMon, 19 Dec 2011 11:16:39 GMT
https://trac.sagemath.org/ticket/11521#comment:31
https://trac.sagemath.org/ticket/11521#comment:31
<p>
Moreover,
</p>
<pre class="wiki">sage: None in Rings()
<BOOOOOOM>
</pre>
TicketSimonKingMon, 19 Dec 2011 11:19:09 GMT
https://trac.sagemath.org/ticket/11521#comment:32
https://trac.sagemath.org/ticket/11521#comment:32
<p>
That said: I am not working on top of vanilla sage, but I use some patches. In particular, I use <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, which introduces so called <code>Category_singleton</code>, which has a specialised and very fast containment test.
</p>
<p>
I would not like to change <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, but prefer to change my patch from here so that it works on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>.
</p>
TicketSimonKingMon, 19 Dec 2011 11:32:13 GMT
https://trac.sagemath.org/ticket/11521#comment:33
https://trac.sagemath.org/ticket/11521#comment:33
<p>
It turns out that indeed the bug is in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>. So, I <em>have</em> to fix it there.
</p>
TicketSimonKingMon, 19 Dec 2011 14:43:38 GMT
https://trac.sagemath.org/ticket/11521#comment:34
https://trac.sagemath.org/ticket/11521#comment:34
<p>
Cc to Nicolas, since it concerns categories:
</p>
<p>
Do we want that <code>Hom(1,1)</code> is still supported?
</p>
<p>
I think it does not make sense at all to talk about the homomorphisms of the number 1 to the number 1. The problem (for my patch as it is posted here) is the fact that one can't create a weak reference to the number 1.
</p>
TicketSimonKingMon, 19 Dec 2011 14:44:19 GMTcc changed
https://trac.sagemath.org/ticket/11521#comment:35
https://trac.sagemath.org/ticket/11521#comment:35
<ul>
<li><strong>cc</strong>
<em>nthiery</em> added
</li>
</ul>
<p>
Sorry, I forgot to update the Cc field.
</p>
<p>
Nicolas, please read my previous comment.
</p>
TicketSimonKingMon, 19 Dec 2011 14:49:07 GMT
https://trac.sagemath.org/ticket/11521#comment:36
https://trac.sagemath.org/ticket/11521#comment:36
<p>
With my patch, applied on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, I get
</p>
<pre class="wiki"> sage -t devel/sage-main/sage/structure/parent.pyx # 2 doctests failed
sage -t devel/sage-main/sage/structure/category_object.pyx # 2 doctests failed
sage -t devel/sage-main/sage/rings/polynomial/polynomial_singular_interface.py # 1 doctests failed
sage -t devel/sage-main/sage/rings/polynomial/multi_polynomial_ring.py # 36 doctests failed
sage -t devel/sage-main/sage/structure/parent_base.pyx # 2 doctests failed
</pre><p>
At least some of the errors are like
</p>
<pre class="wiki"> sage: n = 5; Hom(n,7)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_3[4]>", line 1, in <module>
n = Integer(5); Hom(n,Integer(7))###line 108:
sage: n = 5; Hom(n,7)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.integer.Integer' object
</pre><p>
and I really don't see why this should be considered a bug.
</p>
TicketSimonKingMon, 19 Dec 2011 16:12:15 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:37
https://trac.sagemath.org/ticket/11521#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
</ul>
<p>
At least one of the errors in polynomial rings is due to a wrong order of initialising things: There is some information missing, and by consequence a weak reference can't be created.
</p>
<p>
I fixed this problem in the new patch version.
</p>
<p>
I put it as "needs info", because of my question on whether or not we want to consider an integer as object in a category.
</p>
TicketSimonKingMon, 19 Dec 2011 16:45:28 GMT
https://trac.sagemath.org/ticket/11521#comment:38
https://trac.sagemath.org/ticket/11521#comment:38
<p>
I was told by Mike Hansen why weak references to integers and rationals do not work.
</p>
<p>
I see three options:
</p>
<blockquote>
<p>
#. Drop the support for <code>Hom(1,1)</code> (which I'd prefer)
#. Add a cdef'd attribute <code>__weakref__</code> to <code>sage.structure.element.Element</code>, which would create an overhead for garbage collection for elements, and also a memory overhead.
#. Use two category.homset caches in parallel: One (the default) that uses weak references, and another one that uses "normal" references if weak references fail.
</p>
</blockquote>
TicketSimonKingMon, 19 Dec 2011 21:09:05 GMT
https://trac.sagemath.org/ticket/11521#comment:39
https://trac.sagemath.org/ticket/11521#comment:39
<p>
FWIW:
</p>
<p>
With the latest patch, the tests in polynomial_singular_interface and in multi_polynomial_ring pass.
</p>
<p>
There remain the following problems:
</p>
<pre class="wiki">sage -t "devel/sage-main/sage/structure/parent.pyx"
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/parent.pyx", line 1410:
sage: n = 5; Hom(n,7)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_33[4]>", line 1, in <module>
n = Integer(5); Hom(n,Integer(7))###line 1410:
sage: n = 5; Hom(n,7)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.integer.Integer' object
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/parent.pyx", line 1412:
sage: z=(2/3); Hom(z,8/1)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_33[5]>", line 1, in <module>
z=(Integer(2)/Integer(3)); Hom(z,Integer(8)/Integer(1))###line 1412:
sage: z=(2/3); Hom(z,8/1)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.rational.Rational' object
**********************************************************************
1 items had failures:
2 of 8 in __main__.example_33
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/simon/.sage//tmp/parent_2986.py
[11.6 s]
</pre><p>
and
</p>
<pre class="wiki">sage -t "devel/sage-main/sage/structure/category_object.pyx"
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/category_object.pyx", line 590:
sage: n = 5; Hom(n,7)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_17[4]>", line 1, in <module>
n = Integer(5); Hom(n,Integer(7))###line 590:
sage: n = 5; Hom(n,7)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.integer.Integer' object
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/category_object.pyx", line 592:
sage: z=(2/3); Hom(z,8/1)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_17[5]>", line 1, in <module>
z=(Integer(2)/Integer(3)); Hom(z,Integer(8)/Integer(1))###line 592:
sage: z=(2/3); Hom(z,8/1)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.rational.Rational' object
**********************************************************************
1 items had failures:
2 of 8 in __main__.example_17
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/simon/.sage//tmp/category_object_3050.py
[2.7 s]
</pre><p>
and
</p>
<pre class="wiki">sage -t "devel/sage-main/sage/structure/parent_base.pyx"
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/parent_base.pyx", line 108:
sage: n = 5; Hom(n,7)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_3[4]>", line 1, in <module>
n = Integer(5); Hom(n,Integer(7))###line 108:
sage: n = 5; Hom(n,7)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.integer.Integer' object
**********************************************************************
File "/home/simon/SAGE/sage-4.8.alpha3/devel/sage-main/sage/structure/parent_base.pyx", line 110:
sage: z=(2/3); Hom(z,8/1)
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-4.8.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_3[5]>", line 1, in <module>
z=(Integer(2)/Integer(3)); Hom(z,Integer(8)/Integer(1))###line 110:
sage: z=(2/3); Hom(z,8/1)
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python/site-packages/sage/categories/homset.py", line 159, in Hom
cache2 = _cache[X]
File "/home/simon/SAGE/sage-4.8.alpha3/local/lib/python2.6/weakref.py", line 243, in __getitem__
return self.data[ref(key)]
TypeError: cannot create weak reference to 'sage.rings.rational.Rational' object
**********************************************************************
1 items had failures:
2 of 8 in __main__.example_3
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/simon/.sage//tmp/parent_base_3078.py
[2.6 s]
</pre><p>
So, essentially this is just a single test that comes in two versions and is repeated three times - and I would actually say that <em>not</em> raising an error was a bug.
</p>
<p>
It seems that <code>Hom(1/2,2/3)</code> and similar nonsense is not used in Sage. Hence, I think these tests should be removed. I'll ask sage-devel.
</p>
TicketSimonKingMon, 19 Dec 2011 21:43:48 GMT
https://trac.sagemath.org/ticket/11521#comment:40
https://trac.sagemath.org/ticket/11521#comment:40
<p>
Without the patch:
</p>
<pre class="wiki">sage: def test():
....: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: m0 = get_memory_usage()
sage: %time test()
CPU times: user 7.75 s, sys: 0.08 s, total: 7.83 s
Wall time: 7.84 s
sage: get_memory_usage() - m0
80.234375
</pre><p>
With the patch:
</p>
<pre class="wiki">sage: def test():
....: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: m0 = get_memory_usage()
sage: %time test()
CPU times: user 7.59 s, sys: 0.01 s, total: 7.60 s
Wall time: 7.61 s
sage: get_memory_usage() - m0
8.53515625
</pre><p>
So, the memory does mildly increase, but it seems that most of the leak is fixed.
</p>
<p>
I think that a test of the kind
</p>
<pre class="wiki">sage: get_memory_usage() - -m0 < 10
True
</pre><p>
might be used as a doc test.
</p>
TicketnthieryMon, 19 Dec 2011 22:18:16 GMT
https://trac.sagemath.org/ticket/11521#comment:41
https://trac.sagemath.org/ticket/11521#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:34" title="Comment 34">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Cc to Nicolas, since it concerns categories:
</p>
<p>
Do we want that <code>Hom(1,1)</code> is still supported?
</p>
<p>
I think it does not make sense at all to talk about the homomorphisms of the number 1 to the number 1. The problem (for my patch as it is posted here) is the fact that one can't create a weak reference to the number 1.
</p>
</blockquote>
<p>
I don't see much point either. We had a similar discussion a while ago about whether elements should be objects in a category, and as far as I remember, the answer was no by default (Element does not inherit from <a class="missing wiki">CategoryObject?</a>). So +1 on my side to kill this dubious feature. You might want to double check on sage-algebra just to make sure.
</p>
TicketzimmermaTue, 20 Dec 2011 08:53:47 GMT
https://trac.sagemath.org/ticket/11521#comment:42
https://trac.sagemath.org/ticket/11521#comment:42
<p>
Simon, you can also use the test suggested by Jean-Pierre Flori (see comment <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:18" title="Comment 18">18</a> for an
example).
</p>
<p>
Paul
</p>
TicketSimonKingTue, 20 Dec 2011 09:16:01 GMT
https://trac.sagemath.org/ticket/11521#comment:43
https://trac.sagemath.org/ticket/11521#comment:43
<p>
Hi Paul,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:42" title="Comment 42">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
Simon, you can also use the test suggested by Jean-Pierre Flori (see comment <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:18" title="Comment 18">18</a> for an
example).
</p>
</blockquote>
<p>
Yes, that looks good. With my patch, the test would be like
</p>
<pre class="wiki">sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: import gc
sage: gc.collect()
1881
sage: from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_modn as FF
sage: L = [x for x in gc.get_objects() if isinstance(x, FF)]
sage: len(L), L[0], L[len(L)-1]
(2, Finite Field of size 2, Finite Field of size 99991)
</pre><p>
The people at sage-devel somehow seem to agree that objects of a category should be instances of <code>CategoryObject</code> (which elements aren't!), and that we should thus drop the <code>Hom(2/3,8/1)</code> test.
</p>
<p>
In addition to that, I suggest to provide a better error message, something like
</p>
<pre class="wiki">sage: Hom(2/3, 8/1)
Traceback (most recent call last):
...
TypeError: Objects of categories must be instances of <type 'sage.structure.category_object.CategoryObject'>, but 2/3 isn't.
</pre><p>
Cheers,
</p>
<p>
Simon
</p>
TicketSimonKingTue, 20 Dec 2011 09:22:42 GMTkeywords set
https://trac.sagemath.org/ticket/11521#comment:44
https://trac.sagemath.org/ticket/11521#comment:44
<ul>
<li><strong>keywords</strong>
<em>sd35</em> added
</li>
</ul>
<p>
One bad detail: I'd like to add the test to the documentation of sage.categories.homset. However, if I insert it in the appropriate place, there will be a conflict with both <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>.
</p>
<p>
I could try to insert the test in a less logical place, in order to avoid to have a dependency.
</p>
TicketSimonKingTue, 20 Dec 2011 09:54:46 GMTstatus changed; dependencies set
https://trac.sagemath.org/ticket/11521#comment:45
https://trac.sagemath.org/ticket/11521#comment:45
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
set to <em>#11900</em>
</li>
</ul>
<p>
I am very much afraid that I have not been able to make my patch independent of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>. This is not just because of the documentation, but also because of some details in the choice of the homset's category.
</p>
<p>
Anyway, it needs review (and so does the most recent version of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, by the way)!
</p>
TicketSimonKingTue, 20 Dec 2011 12:04:58 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:46
https://trac.sagemath.org/ticket/11521#comment:46
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I did try the new doctests in sage/categories/homset.py. However, with other patches applied, the number returned by <code>gc.collect()</code> changes.
</p>
<p>
So, for stability, I suggest to simplify the test, so that only the number of finite fields remaining in the cache is tested.
</p>
TicketSimonKingTue, 20 Dec 2011 14:05:33 GMTstatus changed; author set
https://trac.sagemath.org/ticket/11521#comment:47
https://trac.sagemath.org/ticket/11521#comment:47
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Simon King</em>
</li>
</ul>
<p>
I updated the patch.
</p>
<p>
Difference to the previous patch: The number of objects collect by gc is marked as random (indeed, it will change with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a> applied). What we are really interested in is the number of finite fields that remains in the cache after garbage collection. This number is two and is not random. Thus, that test is preserved.
</p>
TicketSimonKingSun, 25 Dec 2011 15:13:55 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:48
https://trac.sagemath.org/ticket/11521#comment:48
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I think I need help with debugging.
</p>
<p>
When I have sage-4.8.alpha3 with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, then all doctests pass.
</p>
<p>
When I also have <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, then the test sage/rings/number_field/number_field_rel.py segfaults. When I run the tests in verbose mode, then all tests seem to pass, but in the very end it says
</p>
<pre class="wiki">4 items had no tests:
__main__
__main__.change_warning_output
__main__.check_with_tolerance
__main__.warning_function
69 items passed all tests:
...
660 tests in 73 items.
660 passed and 0 failed.
Test passed.
The doctested process was killed by signal 11
[15.0 s]
</pre><p>
So, could it be that not one of the tests was killed, but the test <em>process</em> itself?
</p>
<p>
What is even more confusing: When I run the tests with the option -randorder, then most of the time the tests pass without a problem.
</p>
<p>
Can you give me any pointer on how those things could possibly be debugged?
</p>
TicketfbisseySun, 25 Dec 2011 22:45:28 GMT
https://trac.sagemath.org/ticket/11521#comment:49
https://trac.sagemath.org/ticket/11521#comment:49
<p>
It sometimes happen that the sage session itself crash on exit. This is probably one of these. Last time I got one it was related to singular I think. It is quite difficult to corner these with gdb. The best you can hope is start a sage session with gdb and then try the last doctest sequence and quit sage, it may lead to the crash in which case you may have some luck with gdb. But this is one of these case where gdb itself may be interfering. I don't think I have time to look into this right now but I'll put it into my "To do" list in case it isn't solved when i have time to spare.
</p>
TicketSimonKingMon, 26 Dec 2011 18:31:17 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac11521_new_homset_cache.patch</em>
</li>
</ul>
<p>
Use weak references for the keys of the homset cache. If weak references are not supported, then raise an error, pointing out that category objects should be <code>CategoryObject</code>s.
</p>
TicketSimonKingMon, 26 Dec 2011 18:44:32 GMTwork_issues set
https://trac.sagemath.org/ticket/11521#comment:50
https://trac.sagemath.org/ticket/11521#comment:50
<ul>
<li><strong>work_issues</strong>
set to <em>Understand why a weak key dictionary is not enough</em>
</li>
</ul>
<p>
I have attached a new patch version. It fixes the segfault I mentioned. However, it also does <em>not</em> fix the memory leak.
</p>
<p>
The difference between the two versions is: The new patch still uses weak references to the key of the cache, but a strong reference to the value (i.e., the homset).
</p>
<p>
The homset has a reference to domain and codomain, which constitute the cache key. Thus, I expected that it does not make any difference whether one has a strong or a weak reference to the homset. But I stand corrected. That needs to be investigated more deeply.
</p>
TicketjpfloriMon, 26 Dec 2011 20:28:24 GMT
https://trac.sagemath.org/ticket/11521#comment:51
https://trac.sagemath.org/ticket/11521#comment:51
<p>
Dear Simon,
</p>
<p>
Thanks a lot for taking care of all of this !
</p>
<p>
I'm just back from vacation and will have a look at all your patches in the following days.<br />
</p>
<p>
I must point out that even if the memory leak was small, it did still mater because I used a LOT of them and after several hours of computations it ate all the available memory the piece of code in the ticket description is just a minimal example, in my actual code I used different curves and similar simple computations on them)...
</p>
<p>
And to make things clear, I must say I put that ticket as need review in order to get it closed as wont fix/duplicate because I thought it could be seen as a concrete example of ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">715</a> and all the work could be done there.
</p>
<p>
Of course youre the one currently doing all the wok, so do as you want :)
</p>
<p>
Cheers,
</p>
<p>
JP
</p>
TicketSimonKingMon, 26 Dec 2011 20:39:25 GMT
https://trac.sagemath.org/ticket/11521#comment:52
https://trac.sagemath.org/ticket/11521#comment:52
<p>
Hi Jean-Pierre,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:51" title="Comment 51">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I must point out that even if the memory leak was small,
</p>
</blockquote>
<p>
It isn't small.
</p>
<blockquote class="citation">
<p>
And to make things clear, I must say I put that ticket as need review in order to get it closed as wont fix/duplicate because I thought it could be seen as a concrete example of ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">715</a> and all the work could be done there.
</p>
</blockquote>
<p>
I am not sure whether it would be good to do everything on one ticket, as the topics are related, but clearly disting: <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> is about weak "<code>TripleDict</code>" for coercion, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> is about a weak version of cached_function, and the ticket here is about the cache of homsets.
</p>
<p>
On the other hand: I am about to post a new patch here, with <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> as a dependency. It will use the new version of <code>TripleDict</code> from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>. So, one could argue that there is a common tool for both tickets, and they belong together.
</p>
<p>
Anyway. The new patch will fix the leak, but it will not suffer from the segfaults.
</p>
<p>
Cheers,
</p>
<p>
Simon
</p>
TicketSimonKingMon, 26 Dec 2011 20:57:41 GMTstatus, dependencies, description changed; work_issues deleted
https://trac.sagemath.org/ticket/11521#comment:53
https://trac.sagemath.org/ticket/11521#comment:53
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#11900</em> to <em>#11900 #715</em>
</li>
<li><strong>work_issues</strong>
<em>Understand why a weak key dictionary is not enough</em> deleted
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=53">diff</a>)
</li>
</ul>
<p>
I have attached another patch under a new name, using a new approach: The weak <code>TripleDict</code>, that I introduce at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, is an appropriate tool for the cache of homsets. The key is the triple <code>(domain, codomain, category)</code>, and the value is a weak reference to the corresponding homset.
</p>
<p>
There is a new test (the same as in the other patch), showing that the leak is fixed. And all tests in sage/schemes, sage/rings, sage/categories and sage/structure pass.
</p>
<p>
Hence: Needs review!
</p>
<p>
Apply trac11521_triple_homset.patch
</p>
TicketSimonKingTue, 27 Dec 2011 00:33:46 GMT
https://trac.sagemath.org/ticket/11521#comment:54
https://trac.sagemath.org/ticket/11521#comment:54
<p>
In fact <em>all</em> tests pass for me, with <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11900" title="defect: Serious regression caused by #9138 (closed: fixed)">#11900</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11115" title="enhancement: Rewrite cached_method in Cython (closed: fixed)">#11115</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11521/trac11521_triple_homset.patch" title="Attachment 'trac11521_triple_homset.patch' in Ticket #11521">trac11521_triple_homset.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11521/trac11521_triple_homset.patch" title="Download"></a> applied on top of sage-4.8.alpha3.
</p>
TicketjpfloriWed, 28 Dec 2011 08:09:19 GMT
https://trac.sagemath.org/ticket/11521#comment:55
https://trac.sagemath.org/ticket/11521#comment:55
<p>
Applied all patches mentionned in the previous comment without problems on sage-4.8.alpha5 and "make ptestlong" passed all tests.
I'll try to check that the memory leaks actually disappeared :) today and have a look at your patches to give them positive reviews as well.
</p>
TicketSimonKingWed, 28 Dec 2011 09:27:19 GMT
https://trac.sagemath.org/ticket/11521#comment:56
https://trac.sagemath.org/ticket/11521#comment:56
<p>
It depends on what you call "the" leak.
</p>
<p>
At least, the patch fixes <em>one</em> leak, namely the one that is doctested against. I am not sure whether it is enough to fix the leak exposed in the ticket description:
</p>
<pre class="wiki">sage: K = GF(1<<55,'t')
sage: a = K.random_element()
sage: get_memory_usage()
872.26171875
sage: while 1:
....: E = EllipticCurve(j=a)
....: P = E.random_point()
....: Q = 2*P;
....: print get_memory_usage()
</pre><p>
The memory usage does climb, but it climbs a lot slower than, for example, in sage-4.6.2.
</p>
TicketSimonKingWed, 28 Dec 2011 09:36:38 GMT
https://trac.sagemath.org/ticket/11521#comment:57
https://trac.sagemath.org/ticket/11521#comment:57
<p>
PS: I just tested that even <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> (which is a lot more aggressive in using weak references and fixes a gaping leak) is not enough to totally fix the example from the ticket description. Hence, I think that, for now, we should be happy with a partial fix and investigate the remaining problems on different tickets.
</p>
TicketSimonKingWed, 28 Dec 2011 09:43:33 GMT
https://trac.sagemath.org/ticket/11521#comment:58
https://trac.sagemath.org/ticket/11521#comment:58
<p>
After running the example for a couple of minutes, interrupting and doing garbage collection, I find:
</p>
<pre class="wiki">sage: from sage.schemes.elliptic_curves.ell_finite_field import EllipticCurve_finite_field
sage: LE = [x for x in gc.get_objects() if isinstance(x,EllipticCurve_finite_field)]
sage: len(LE)
632
sage: import objgraph
sage: from sage.rings.finite_rings.finite_field_prime_modn import FiniteField_prime_modn as FF
sage: LF = [x for x in gc.get_objects() if isinstance(x, FF)]
sage: len(LF)
1
</pre><p>
LF shows that one leak is fixed. However, the curves in LE, which are all defined over the same finite field, can not be collected.
</p>
TicketSimonKingWed, 28 Dec 2011 10:56:26 GMT
https://trac.sagemath.org/ticket/11521#comment:59
https://trac.sagemath.org/ticket/11521#comment:59
<p>
Using objgraph, I found that the remaining leak seem to be related with <code>sage.schemes.generic.homset.SchemeHomsetModule_abelian_variety_coordinates_field_with_category</code>. Since this is another homset, it would make sense to try and fix it <em>here</em>.
</p>
TicketSimonKingWed, 28 Dec 2011 11:03:02 GMT
https://trac.sagemath.org/ticket/11521#comment:60
https://trac.sagemath.org/ticket/11521#comment:60
<p>
Aha!!
</p>
<p>
I found that we have many <em>equal</em> instances of these scheme homsets:
</p>
<pre class="wiki">sage: LE = [x for x in gc.get_objects() if isinstance(x,SchemeHomsetModule_abelian_variety_coordinates_field)]
sage: LE[100] == LE[150]
True
</pre><p>
So, I guess we should fix the memory leak by using <code>UniqueRepresentation</code> or <code>UniqueFactory</code>!
</p>
TicketjpfloriWed, 28 Dec 2011 11:30:38 GMT
https://trac.sagemath.org/ticket/11521#comment:61
https://trac.sagemath.org/ticket/11521#comment:61
<p>
My 2 cents: Isn't it somehow related to the fact that elliptic curves are not unique parents?
In some of my original tests, I used different curves and depending on the cache where they were stored "equality" testing was made either with "is" or with "==".
In the former case, the "same" elliptic curve would be stored several times making the "leak" even bigger.
</p>
TicketSimonKingWed, 28 Dec 2011 12:22:01 GMT
https://trac.sagemath.org/ticket/11521#comment:62
https://trac.sagemath.org/ticket/11521#comment:62
<p>
OK. Then why not reduce the non-uniqueness? <code>EllipticCurve</code> could easily be turned into a cached function, which would mean that two elliptic curves became identical if they are defined by equal data. That is <em>not</em> enough to be a unique parent (there could be equal elliptic curves defined by different data), but it could be a step forward. And with <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>, it could then actually be turned into a <em>weak</em> cached function.
</p>
TicketSimonKingWed, 28 Dec 2011 12:32:47 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:63
https://trac.sagemath.org/ticket/11521#comment:63
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Yes, using a cached function would indeed fix the leak that is cause by the useless creation of creating an elliptic curve with the same basic data over and over again. In particular, it would fix the leak from the ticket description (<a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> is not needed for that).
</p>
<p>
I am preparing a new patch (it requires do tests and stuff), so, it is "needs work" for now.
</p>
TicketSimonKingWed, 28 Dec 2011 12:40:45 GMT
https://trac.sagemath.org/ticket/11521#comment:64
https://trac.sagemath.org/ticket/11521#comment:64
<p>
It is definitely not easy. It seems that the elliptic curve code relies on the non-uniqueness of elliptic curves: I get loads of errors.
</p>
TicketSimonKingWed, 28 Dec 2011 12:47:37 GMT
https://trac.sagemath.org/ticket/11521#comment:65
https://trac.sagemath.org/ticket/11521#comment:65
<p>
Ah! Tuples of elements of different rings can be equal, but the corresponding elliptic curves would be different. So, the ring needs to be part of the description.
</p>
TicketSimonKingWed, 28 Dec 2011 15:46:26 GMT
https://trac.sagemath.org/ticket/11521#comment:66
https://trac.sagemath.org/ticket/11521#comment:66
<p>
I am undecided what we should do:
</p>
<p>
We could argue that my patch does fix some memory leak, and leave it like that (modulo comments of the reviewer, of course). In order to fix the memory leak exposed by the example from the ticket description, we have no chance but to have some kind of uniqueness for elliptic curves. But that is a different topic and should thus be dealt with on a different ticket (perhaps such ticket already exists?).
</p>
<p>
Or we could argue that this ticket is about fixing the memory leak that is exposed in the description. Hence, we should do all necessary steps <em>here</em>.
</p>
<p>
And then, there is still the question whether the number theorists really want the elliptic curves be "weakly unique" (i.e., identical if the given data are equal). In addition, we might want that the elliptic curve cache is weak - which might imply that we have to wait for <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>.
</p>
<p>
What do you think?
</p>
<p>
I guess I'll also ask on sage-nt.
</p>
TicketjpfloriWed, 28 Dec 2011 15:56:48 GMT
https://trac.sagemath.org/ticket/11521#comment:67
https://trac.sagemath.org/ticket/11521#comment:67
<p>
I think we can go your way and stop working on this ticket now (or rather once I've taken the time to go through your patches to properly review them).
Thus we can already some non negligible improvements merged.
</p>
<p>
Anyway the problem of uniqueness of elliptic curve is highly non trivial and deserves its own ticket.
I guess <a class="closed ticket" href="https://trac.sagemath.org/ticket/11474" title="defect: Elliptic curves should be unique parent structures (closed: fixed)">#11474</a> would be a right place to treat that problem.
An alternative would be to make this ticket depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11474" title="defect: Elliptic curves should be unique parent structures (closed: fixed)">#11474</a>.
</p>
TicketSimonKingWed, 28 Dec 2011 16:05:31 GMT
https://trac.sagemath.org/ticket/11521#comment:68
https://trac.sagemath.org/ticket/11521#comment:68
<p>
Hi Jean-Pierre,
</p>
<p>
indeed, you found the correct ticket.
</p>
<p>
And there is no need to ask on sage-nt, since I already did before opening <a class="closed ticket" href="https://trac.sagemath.org/ticket/11474" title="defect: Elliptic curves should be unique parent structures (closed: fixed)">#11474</a>: <a class="ext-link" href="http://groups.google.com/group/sage-nt/browse_thread/thread/ec8d0ad14a819082/049bfad6fd0680d1"><span class="icon"></span>Sage-nt</a> first seemed to agree that uniqueness is good, so I opened the ticket. But then, they became convinced that much of the elliptic curve stuff depends on choices (generators), so that we should consider elliptic curves to be mutable objects, for which we wouldn't like to have uniqueness.
</p>
<p>
Considering the discussion on sage-nt, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11474" title="defect: Elliptic curves should be unique parent structures (closed: fixed)">#11474</a> probably is "wontfix".
</p>
TicketSimonKingWed, 28 Dec 2011 16:11:32 GMT
https://trac.sagemath.org/ticket/11521#comment:69
https://trac.sagemath.org/ticket/11521#comment:69
<p>
I am not sure whether we should really stop work right there. After all, it is still not 100% clear to me why the elliptic curve E, that is created in the loop in an increasing number of copies, can not be garbage collected.
</p>
<p>
First, E is created, and some coercion into it is created. The coercion is cached. By <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, some key of the cache provides a weak reference to E. In addition, the coerce map refers to a homset, and the homset refers to its codomain, which is E. I wonder whether there is a chain of strong references from E to the homset (one could try to find out using objgraph, I guess). If there is, then we would have a reference cycle. If that was the case, then we needed to find a <code>__del__</code> method that prevents the cycle from being garbage collected.
</p>
TicketjpfloriWed, 28 Dec 2011 16:30:46 GMT
https://trac.sagemath.org/ticket/11521#comment:70
https://trac.sagemath.org/ticket/11521#comment:70
<p>
Objgraph only shows a reference left as _codomain in a dict in a SchemeHomsetModule_a_v_c_f (defined in sage.schemes.generic.homset).
</p>
TicketjpfloriWed, 28 Dec 2011 17:13:36 GMT
https://trac.sagemath.org/ticket/11521#comment:71
https://trac.sagemath.org/ticket/11521#comment:71
<p>
The homset of points of the ab. group of the curve is itself reference by an <a class="missing wiki">IntegerMulAction?</a>, the point at infinity on the curve (no idea when it gets created) and a dict with 11 elements.
I guess the problem might be that in addition to the _cache in sage.categories.homset the homset of points is directly link within the Action object ?
That could also be nonsense.
</p>
TicketSimonKingWed, 28 Dec 2011 18:47:06 GMT
https://trac.sagemath.org/ticket/11521#comment:72
https://trac.sagemath.org/ticket/11521#comment:72
<p>
The <code>IntegerMulAction</code> should be fine, as it is stored in a <code>TripleDict</code> (hence, weakly, by <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>). Where does the dict occur?
</p>
TicketSimonKingWed, 28 Dec 2011 19:04:40 GMT
https://trac.sagemath.org/ticket/11521#comment:73
https://trac.sagemath.org/ticket/11521#comment:73
<p>
I am a bit puzzled.
</p>
<p>
I see that there are a couple of cycles, involving an elliptic curve, a <code>SchemeHomsetModule...</code>, an <code>IntegerMulAction</code>, and an <code>EllipticPoint_finit_field</code>. However, none of them has a <code>__del__</code> method, thus, the garbage collection should be able to collect the cycles.
</p>
<p>
But the backref graph also shows something that I don't understand: Apparently there is a top level dict with three items that points to the elliptic curve. Where is it?
</p>
TicketSimonKingWed, 28 Dec 2011 21:18:58 GMT
https://trac.sagemath.org/ticket/11521#comment:74
https://trac.sagemath.org/ticket/11521#comment:74
<p>
I got it!!
</p>
<p>
By <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, both the keys and the value of <code>sage.structure.coerce_dict.TripleDict</code> are by weak reference -- if possible. If the value does not allow for a weak reference, then (to simplify code) a constant function is used instead.
</p>
<p>
Actions are stored in such <code>TripleDict</code>s. However, an <code>IntegerMulAction</code> can not be weakly referenced. Hence, there is a strong reference, and no garbage collection can occur (the value points back to the keys, hence, they can't be collected either).
</p>
<p>
Solution: Make <code>IntegerMulAction</code> weakly referenceable! That should suffice.
</p>
TicketSimonKingWed, 28 Dec 2011 21:22:40 GMT
https://trac.sagemath.org/ticket/11521#comment:75
https://trac.sagemath.org/ticket/11521#comment:75
<p>
It does not suffice. But I think it is a step to the right direction.
</p>
TicketSimonKingWed, 28 Dec 2011 21:39:51 GMT
https://trac.sagemath.org/ticket/11521#comment:76
https://trac.sagemath.org/ticket/11521#comment:76
<p>
Apparently the weak reference to the value is not used in the <code>TripleDict</code>! So, I have to look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, perhaps I forgot to implement something there.
</p>
TicketSimonKingWed, 28 Dec 2011 21:42:55 GMT
https://trac.sagemath.org/ticket/11521#comment:77
https://trac.sagemath.org/ticket/11521#comment:77
<p>
Aha, I was mistaken: At <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> I only took care for weak references to the keys of <code>TripleDict</code>. The big question is whether we additionally want weak references to the values. I am somehow against doing this at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
TicketjpfloriWed, 28 Dec 2011 21:54:31 GMT
https://trac.sagemath.org/ticket/11521#comment:78
https://trac.sagemath.org/ticket/11521#comment:78
<p>
My point was that it seemed to me that there was some (not weak!) reference in the dictionary ZZ._action_hash pointing to the <a class="missing wiki">IntegerMulAction?</a> itself pointing to the curve and I thought it could prevent garbage collection.
</p>
<p>
So I added a cpdef function to be able to clear that dictionary and see if garbage collection occurs once it is emptied, however it is a the C level so kind of the whole Sage library had to be rebuilt and I had to go back home before rebuilding was finished...
</p>
<p>
Anyway, I'll have a look at it tomorrow :)
</p>
<p>
By the way, have you any idea if dictionaries declared at the C level such as ZZ._action_hash are detected by objgraph ?
</p>
TicketSimonKingWed, 28 Dec 2011 23:09:58 GMT
https://trac.sagemath.org/ticket/11521#comment:79
https://trac.sagemath.org/ticket/11521#comment:79
<p>
Hi Jean-Pierre,
</p>
<p>
The thing with the _action_hash is a good finding. I thought it would be a <code>TripleDict</code>, but it is just a usual dict. And this could indeed be a problem. I don't know if this is visible to objgraph.
</p>
<p>
But also I think that in addition we have the problem of strong references to the values of <code>TripleDict</code>. In principle, one could use weak references for not only the keys but also to the values -- perhaps this could be done in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
<p>
Or one could leave <code>TripleDict</code> as it is currently in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, but explicitly use weak references to functors for coercion (which needs to be enabled first). _action_hash then has to use weak references as well.
</p>
<p>
There might be a speed problem, though.
</p>
TicketjpfloriThu, 29 Dec 2011 09:23:03 GMT
https://trac.sagemath.org/ticket/11521#comment:80
https://trac.sagemath.org/ticket/11521#comment:80
<p>
For info, resetting ZZ._action_hash to {} is not sufficient to let the <a class="missing wiki">IntegerMulAction?</a> be garbage collected.
</p>
<p>
Objgraph shows 3 objects pointing to the abelian group of points of the curve (itself pointing to the curve):
</p>
<ul><li>the <a class="missing wiki">IntegerMulAction?</a>
</li><li>the point at infinity (?)
</li><li>a dict of size 11 which is in fact an attribute (_Scheme<span class="underline">ring_point_homset) of the curve (as a scheme) itself.
</span></li></ul><p>
I'd be happy to test a weakref for values version of your patch regardless of the spedd impact.
</p>
TicketjpfloriThu, 29 Dec 2011 10:11:40 GMT
https://trac.sagemath.org/ticket/11521#comment:81
https://trac.sagemath.org/ticket/11521#comment:81
<p>
I guess you're solution should be the right one because resetting the coercion cache solves the problem, so dealing with it should be enough for the example in the ticket description.
</p>
<p>
Only one copy gets cached in _action_hash because the curves are equal (==) but not identical (is). However, if one uses (really) different curves, one of each gets also cached in _action_hash
</p>
<p>
Here is small piece of code to test the effect of clearing different caches (the code to clear the ZZ cache is left as an exercise, anyway we should use weakrefs there):
</p>
<pre class="wiki">sage: import gc, inspect
sage: load /usr/share/shared/objgraph.py # or whatever you should type to use objgraph
sage: from sage.schemes.elliptic_curves.ell_finite_field import EllipticCurve_finite_field
sage: K = GF(1<<60, 't')
sage: j = K.random_element()
sage: for i in xrange(100):
....: E = EllipticCurve(j=j); P = E.random_point(); 2*P; del P; del E;
....:
sage: gc.collect()
68
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L)
100
sage: del L
sage: get_coercion_model().reset_cache()
sage: gc.collect()
6172
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L)
1
sage: del L
sage: ZZ.del_hash()
sage: gc.collect()
56
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L)
0
sage: del L
sage: for i in xrange(100):
....: E = EllipticCurve(j=K.random_element()); P = E.random_point(); 2*P; del P; del E;
....:
sage: gc.collect()
174
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L)
100
sage: del L
sage: get_coercion_model().reset_cache()
sage: gc.collect()
738
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L) # _action_hash
100
sage: del L
sage: ZZ.del_hash()
sage: gc.collect()
5742
sage: L = [x for x in gc.get_objects() if isintance(x, EllipticCurve_finite_field)]
sage: len(L) # mmm got one left!!! not sure where it comes from yet...
1
</pre>
TicketSimonKingThu, 29 Dec 2011 14:44:16 GMT
https://trac.sagemath.org/ticket/11521#comment:82
https://trac.sagemath.org/ticket/11521#comment:82
<p>
Hi Jean-Pierre,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:81" title="Comment 81">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I guess you're solution should be the right one because resetting the coercion cache solves the problem, so dealing with it should be enough for the example in the ticket description.
</p>
</blockquote>
<p>
But it is not so easy because...
</p>
<blockquote class="citation">
<p>
Only one copy gets cached in _action_hash because the curves are equal (==) but not identical (is).
</p>
</blockquote>
<p>
... elliptic curves are not unique parents.
</p>
<p>
I think it would be a mistake to use a weak reference to the value of <code>TripleDict</code>. I tried and got many errors - and I think this was because some important data (homsets, actions, ...) was garbage collected even though there was still a strong reference to domain and codomain. In that situation, the value must not be garbage collected.
</p>
<blockquote class="citation">
<p>
sage: len(L) # mmm got one left!!! not sure where it comes from yet...
</p>
</blockquote>
<p>
Don't forget the last copy of E that was defined in the loop!
</p>
<p>
Since I think that a weak reference to the values of <code>TripleDict</code> won't work: What else could one do? Or perhaps I should try again: It could be that the errors came from the wrong choice of a callback function.
</p>
TicketjpfloriThu, 29 Dec 2011 14:50:40 GMT
https://trac.sagemath.org/ticket/11521#comment:83
https://trac.sagemath.org/ticket/11521#comment:83
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:82" title="Comment 82">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Hi Jean-Pierre, Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:81" title="Comment 81">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I guess you're solution should be the right one because resetting the coercion cache solves the problem, so dealing with it should be enough for the example in the ticket description.
</p>
</blockquote>
<p>
But it is not so easy because...
</p>
<blockquote class="citation">
<p>
Only one copy gets cached in _action_hash because the curves are equal (==) but not identical (is).
</p>
</blockquote>
<p>
... elliptic curves are not unique parents.
</p>
</blockquote>
<p>
Yep :)
</p>
<blockquote class="citation">
<p>
I think it would be a mistake to use a weak reference to the value of <code>TripleDict</code>. I tried and got many errors - and I think this was because some important data (homsets, actions, ...) was garbage collected even though there was still a strong reference to domain and codomain. In that situation, the value must not be garbage collected.
</p>
</blockquote>
<p>
I see...
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
sage: len(L) # mmm got one left!!! not sure where it comes from yet...
</p>
</blockquote>
<p>
Don't forget the last copy of E that was defined in the loop!
</p>
</blockquote>
<p>
In the new code I posted I explicitely added "del P; del E;" to the loop so no copy should be left.
What's even stranger is that this remaining copy appears after the second loop (with random j invariants so different curves) but not after the first one (with constant j invariant so only one curve)!
</p>
<blockquote class="citation">
<p>
Since I think that a weak reference to the values of <code>TripleDict</code> won't work: What else could one do? Or perhaps I should try again: It could be that the errors came from the wrong choice of a callback function.
</p>
</blockquote>
<p>
Mmm, have to think more about all of that...
</p>
TicketSimonKingThu, 29 Dec 2011 15:24:33 GMT
https://trac.sagemath.org/ticket/11521#comment:84
https://trac.sagemath.org/ticket/11521#comment:84
<p>
I think I should describe the problem in more detail.
</p>
<p>
Currently,
</p>
<ul><li>There is a strong reference from <code>__main__</code> to the action and coercion caches that are stored in the coercion model. That's fine.
</li><li>Parents store actions as well. Some of these parents (the integer ring, for example) will always stay in memory.
</li><li>There is a strong reference from any action and homset back to domain and codomain, which are used as keys in the cache. I think that's fine: If a homset is alive then its domain and codomain must remain alive as well.
</li><li>The action and coercion caches have a strong reference to the values.
</li><li>There is no direct reference from domain and codomain to the homset.
</li></ul><p>
Hence, if an action is stored in the action cache, then there will always be a chain of strong references from <code>__main__</code> via the cache to the action, and further to domain and codomain, so that it can not be collected.
</p>
<p>
On the other hand, if weak references to the values of the action and coercion caches are used, then an action or a coercion could die even when domain and codomain were still alive. That's probably not good. To the very least, it would imply that actions would be needed to be re-created over and over again.
</p>
<p>
How could that be solved?
</p>
TicketjpfloriThu, 29 Dec 2011 16:12:29 GMT
https://trac.sagemath.org/ticket/11521#comment:85
https://trac.sagemath.org/ticket/11521#comment:85
<p>
If I understand correctly, the keys to all these dictionaries are triples and what you've done is that ift elements in that triple are weakrefs to non strongly refed objects, the key-value pair gets deleted so that garbage collection occur for these only weakrefed objects?
</p>
<p>
Therefore, if ZZ._action_hash gets deleted, only weakrefs to the abelian groups of points of the curve should be left in the coercion model (in the second triple dict _action_maps) and it should not prevent garbage collection...
</p>
TicketjpfloriThu, 29 Dec 2011 16:16:44 GMT
https://trac.sagemath.org/ticket/11521#comment:86
https://trac.sagemath.org/ticket/11521#comment:86
<p>
Maybe the problem is that the value in that last dict corresponding to the triple with a (abelian group of points of a) curve is a (weakref to unless not weakreferrable) the <a class="missing wiki">IntegerMulAction?</a> which itself has a strong ref to the curve which would prevent the curve to get collected?
</p>
TicketjpfloriThu, 29 Dec 2011 16:25:58 GMT
https://trac.sagemath.org/ticket/11521#comment:87
https://trac.sagemath.org/ticket/11521#comment:87
<p>
This is basically what you concluded in <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:74" title="Comment 74">Comment 74</a>, so a solution could be to allow Functors to be weakreferreable or make them use weak refs for their [co]domains?
You posted that making making <a class="missing wiki">IntegerMulAction?</a> weak referrable is not enough, could you post a patch applying these ideas so that I can play with it?
</p>
TicketSimonKingThu, 29 Dec 2011 17:06:22 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>weak_ref_to_functor.patch</em>
</li>
</ul>
<p>
Experimental patch using weak references for actions
</p>
TicketSimonKingThu, 29 Dec 2011 17:09:15 GMT
https://trac.sagemath.org/ticket/11521#comment:88
https://trac.sagemath.org/ticket/11521#comment:88
<p>
I was mistaken: Using weak references to the actions (on top of the other patch) does fix the leak - see the experimental patch that I have just posted.
</p>
<p>
I did not run any doctests on it, yet. But I tested that only one <code>SchemeHomsetModule...</code> survives the garbage collection (and I did not delete E).
</p>
TicketjpfloriThu, 29 Dec 2011 17:24:59 GMT
https://trac.sagemath.org/ticket/11521#comment:89
https://trac.sagemath.org/ticket/11521#comment:89
<p>
I confirm the leak is fixed with your last patch, just launched a ptestlong.
</p>
TicketSimonKingThu, 29 Dec 2011 17:41:16 GMT
https://trac.sagemath.org/ticket/11521#comment:90
https://trac.sagemath.org/ticket/11521#comment:90
<p>
My first impression from some tests is that the additional patch causes a massive slow-down.
</p>
TicketjpfloriThu, 29 Dec 2011 18:28:50 GMT
https://trac.sagemath.org/ticket/11521#comment:91
https://trac.sagemath.org/ticket/11521#comment:91
<p>
I ran the sage test suite on the same pc, on a 4.8.alpha5 with patches and a 4.7.2 without patches, just on the schemes directory and got 1512 vs 1060 sec... some files required between 2 and 3 times more time (e.g. hyperelliptic_padic_field, heegner and ell_number_field which are already quite long, i'd say most of the file already long), others did not change at all.
</p>
TicketSimonKingThu, 29 Dec 2011 19:03:20 GMT
https://trac.sagemath.org/ticket/11521#comment:92
https://trac.sagemath.org/ticket/11521#comment:92
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:91" title="Comment 91">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I ran the sage test suite on the same pc, on a 4.8.alpha5 with patches and a 4.7.2 without patches, just on the schemes directory and got 1512 vs 1060 sec...
</p>
</blockquote>
<p>
That is not half as bad as I thought! However, it is far from good.
</p>
<p>
Do you also have the time with only the first patch, resp. with <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> only? After all, <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> uses weak references and may thus be responsible for some slow-down.
</p>
TicketSimonKingThu, 29 Dec 2011 22:38:04 GMT
https://trac.sagemath.org/ticket/11521#comment:93
https://trac.sagemath.org/ticket/11521#comment:93
<p>
I have slightly modifies <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11521/trac11521_triple_homset.patch" title="Attachment 'trac11521_triple_homset.patch' in Ticket #11521">trac11521_triple_homset.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11521/trac11521_triple_homset.patch" title="Download"></a>: In the old version, I had created a <code>TripleDict(50)</code>, but meanwhile I learnt that the parameter of the <code>TripleDict</code> should not be even and should best be a prime number. In the new version, it is prime...
</p>
<p>
Concerning the weak references: Why exactly is the experimental patch so slow? Is it because the access to the weakly referenced actions is so slow? Or is it because the actions are garbage collected even if their domain/codomain is still alive, so that the actions are uselessly created over and over again? I suspect it is the latter.
</p>
TicketjpfloriFri, 30 Dec 2011 09:14:42 GMT
https://trac.sagemath.org/ticket/11521#comment:94
https://trac.sagemath.org/ticket/11521#comment:94
<p>
I guess you're right.
</p>
<p>
Here is a piece of code emphasizing the problem.
</p>
<pre class="wiki">sage: K = GF(1<<60, 't')
sage: a = K.random_element()
sage: E = EllipticCurve([1, 0, 0, 0, a])
sage: P = E.random_point()
sage: 2*P
...
sage: ZZ._introspect_coerce()['_action_hash']
{(<type 'list'>, ...} # No Abelian group of points of the curve!!!
sage: get_coercion_model().get_cache()[1]
{(Integer Ring, Abelian group of points on Elliptic Curve defined by ..., <built-in function mul>): <weakref at ...; dead>, ...} # the "dead" are bad
</pre><p>
Indeed, has all the dicts for coercion caches have weakrefs to their values, the actions get garbage collected. That becomes kind of tricky...
</p>
<p>
As pointed before, the problem is that if we let strong reference to the actions in the values of the dict, these action themselves have strong refs to the domain and codomain and so prevent the whole garbage collection to occur. Is it sensible to use weakrefs for [co]domains in Functors? Hence garbage collection can occur in the cache dicts, but if someone actually use functors directly, he must be sure to have some strong references to its domain and codomain somewhere to avoid garbage collection...
</p>
<p>
Another question: why not use a <a class="missing wiki">TripleDict?</a> in the parent class for the _action_hash dict rather that a <a class="missing wiki">WeakValueDict?</a> with three keys? That could somehow unify the approach taken here!
</p>
TicketSimonKingFri, 30 Dec 2011 09:47:40 GMT
https://trac.sagemath.org/ticket/11521#comment:95
https://trac.sagemath.org/ticket/11521#comment:95
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:94" title="Comment 94">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Indeed, has all the dicts for coercion caches have weakrefs to their values, the actions get garbage collected. That becomes kind of tricky...
</p>
</blockquote>
<p>
Yes, and note that we have two locations for storing the actions:
</p>
<ul><li>in the coercion model
</li><li>in the attribute <code>_action_hash</code> of parents.
</li></ul><p>
I found that having weak references in the coercion model is enough for fixing the leak - even if one has strong references in <code>_action_hash</code>. That is something I don't fully understand. In the example of the ticket description, we have an action of <code>ZZ</code>. <code>ZZ</code> is not subject to garbage collection, hence, having a strong reference in <code>ZZ._action_hash</code> should keep the action alive, and thus the elliptic curve (namely the different copies of E that are created in the loop).
</p>
<p>
Anyway, even in that case we would see the drastic slow-down.
</p>
<blockquote class="citation">
<p>
As pointed before, the problem is that if we let strong reference to the actions in the values of the dict, these action themselves have strong refs to the domain and codomain and so prevent the whole garbage collection to occur. Is it sensible to use weakrefs for [co]domains in Functors? Hence garbage collection can occur in the cache dicts, but if someone actually use functors directly, he must be sure to have some strong references to its domain and codomain somewhere to avoid garbage collection...
</p>
</blockquote>
<p>
Indeed that would be the consequence. I think that would not be acceptable: If the user keeps an action, then s/he can rightfully expect that domain and codomain are automatically kept alive.
</p>
<blockquote class="citation">
<p>
Another question: why not use a <a class="missing wiki">TripleDict?</a> in the parent class for the _action_hash dict rather that a <a class="missing wiki">WeakValueDict?</a> with three keys? That could somehow unify the approach taken here!
</p>
</blockquote>
<p>
I was thinking of that, too. However, in addition to <code>_action_hash</code>, the parent also has a <code>_action_list</code>. And that might be a bigger problem.
</p>
TicketjpfloriFri, 30 Dec 2011 09:53:51 GMT
https://trac.sagemath.org/ticket/11521#comment:96
https://trac.sagemath.org/ticket/11521#comment:96
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:95" title="Comment 95">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Yes, and note that we have two locations for storing the actions: * in the coercion model * in the attribute <code>_action_hash</code> of parents. I found that having weak references in the coercion model is enough for fixing the leak - even if one has strong references in <code>_action_hash</code>. That is something I don't fully understand.
</p>
</blockquote>
<p>
As I posted above in ZZ._action_hash equality is tested with "==" (rather than identity with "is") so in the code of the ticket description where only "one" curve is used, only one curve gets stored in _action_hash.
If you try the code posted some comments above where (really) different curves are used, you'll see that the leak is not fixed if you dont use a similar approach for _action_hash as for the coercion model.
</p>
<blockquote class="citation">
<p>
In the example of the ticket description, we have an action of <code>ZZ</code>. <code>ZZ</code> is not subject to garbage collection, hence, having a strong reference in <code>ZZ._action_hash</code> should keep the action alive, and thus the elliptic curve (namely the different copies of E that are created in the loop). Anyway, even in that case we would see the drastic slow-down.
</p>
<blockquote>
<p>
Indeed that would be the consequence. I think that would not be acceptable: If the user keeps an action, then s/he can rightfully expect that domain and codomain are automatically kept alive.
</p>
</blockquote>
</blockquote>
<p>
The weakref domain and codomain in functors problem could be tackled by adding an optional parameter for the use of weakref.
</p>
<p>
By default, the behavior of functors would be as before with strong refs (and the option to false).
</p>
<p>
For the coercion models we would set the option to True and use weakref whenever possible.
</p>
TicketSimonKingFri, 30 Dec 2011 10:13:13 GMT
https://trac.sagemath.org/ticket/11521#comment:97
https://trac.sagemath.org/ticket/11521#comment:97
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:96" title="Comment 96">jpflori</a>:
</p>
<blockquote class="citation">
<p>
As I posted above in ZZ._action_hash equality is tested with "==" (rather than identity with "is") so in the code of the ticket description where only "one" curve is used, only one curve gets stored in _action_hash.
</p>
</blockquote>
<p>
Yes, but then I don't understand why there is no error: In some places, the coercion model tests for identity, and raises a big fat "coercion BUG" otherwise.
</p>
<blockquote class="citation">
<p>
The weakref domain and codomain in functors problem could be tackled by adding an optional parameter for the use of weakref.
</p>
<p>
By default, the behavior of functors would be as before with strong refs (and the option to false).
</p>
<p>
For the coercion models we would set the option to True and use weakref whenever possible.
</p>
</blockquote>
<p>
Again, I believe that it must not be up to the user: "Evidently" (for a human), the different copies of E in the while loop can be garbage collected. It is our job to hammer the "evidence" into Sage. We shouldn't simply say "let the user decide" whether he wants a leak or a potential disaster: I would call it a disaster if one has an action and suddenly its codomain is gone.
</p>
TicketjpfloriFri, 30 Dec 2011 10:34:54 GMT
https://trac.sagemath.org/ticket/11521#comment:98
https://trac.sagemath.org/ticket/11521#comment:98
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:97" title="Comment 97">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Yes, but then I don't understand why there is no error: In some places, the coercion model tests for identity, and raises a big fat "coercion BUG" otherwise.
</p>
</blockquote>
<p>
I'll have a look at it, I'm not completely up to what the coercion model actually does :)
I guess the doc could also be extended on that :)
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
The weakref domain and codomain in functors problem could be tackled by adding an optional parameter for the use of weakref.
</p>
<p>
By default, the behavior of functors would be as before with strong refs (and the option to false).
</p>
<p>
For the coercion models we would set the option to True and use weakref whenever possible.
</p>
</blockquote>
<p>
Again, I believe that it must not be up to the user: "Evidently" (for a human), the different copies of E in the while loop can be garbage collected. It is our job to hammer the "evidence" into Sage. We shouldn't simply say "let the user decide" whether he wants a leak or a potential disaster: I would call it a disaster if one has an action and suddenly its codomain is gone.
</p>
</blockquote>
<p>
That would not really be up to the user if by default the functor does not use weakref, but if we change this behavior for the coercion model by switching the option.
It would be fatly documented not to set the option to True for general use, so normal use would not lead to problems.
Of course it all depends on what you mean by "up to the user".
One could still intentionally set the option to true, and then cry that its codomain disappeared...
</p>
TicketjpfloriFri, 30 Dec 2011 11:59:10 GMT
https://trac.sagemath.org/ticket/11521#comment:99
https://trac.sagemath.org/ticket/11521#comment:99
<p>
In the verify_action of the coerce model there is a PY_TYPE_CHECK(action, <a class="missing wiki">IntegerMulAction?</a>) that might explain the absence of fat BUG.
</p>
TicketjpfloriFri, 30 Dec 2011 12:53:52 GMT
https://trac.sagemath.org/ticket/11521#comment:100
https://trac.sagemath.org/ticket/11521#comment:100
<p>
Anyway, it does not make much sense to me that the _action_hash dict in the Parent class uses "==" rather than "is", especially since the <a class="missing wiki">TripleDict?</a> of the coercion model do.
What do yo think? Have you any good reason to justify such a choice that I might have missed?
</p>
TicketSimonKingFri, 30 Dec 2011 13:21:10 GMT
https://trac.sagemath.org/ticket/11521#comment:101
https://trac.sagemath.org/ticket/11521#comment:101
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:100" title="Comment 100">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Anyway, it does not make much sense to me that the _action_hash dict in the Parent class uses "==" rather than "is", especially since the <a class="missing wiki">TripleDict?</a> of the coercion model do.
</p>
</blockquote>
<p>
The old <code>TripleDict</code> didn't - I changed that only in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>. Perhaps it is debatable whether that change is fine. But:
</p>
<blockquote class="citation">
<p>
What do yo think? Have you any good reason to justify such a choice that I might have missed?
</p>
</blockquote>
<p>
See sage/structure/coerce.pyx:
</p>
<pre class="wiki"> if y_mor is not None:
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)
</pre><p>
That is why I think one should test for identity, not equality.
</p>
<p>
On the other hand, we also have
</p>
<pre class="wiki"> # Make sure the domains are correct
if R_map.domain() is not R:
if fix:
connecting = R_map.domain().coerce_map_from(R)
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)
if S_map is not None and S_map.domain() is not S:
if fix:
connecting = S_map.domain().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)
</pre><p>
in the same file. These lines apparently cope with the fact that there are non-unique parents, and they suggest that <code>==</code> is the right thing to do in the coerce caches.
</p>
<p>
But I think this is a discussion that belongs to <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
TicketSimonKingFri, 30 Dec 2011 14:01:53 GMT
https://trac.sagemath.org/ticket/11521#comment:102
https://trac.sagemath.org/ticket/11521#comment:102
<p>
My suggestion at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> is to use a different way of comparison in the case of actions respectively coerce maps. This complies with existing defaults in sage/structure/coerce.pyx
</p>
TicketjpfloriFri, 30 Dec 2011 14:11:45 GMT
https://trac.sagemath.org/ticket/11521#comment:103
https://trac.sagemath.org/ticket/11521#comment:103
<p>
I agree to move the discussion for "==" and "is" to <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> (and also agree with your solution).
</p>
<p>
So, what about the original problem here and your thoughts about moving the weakref from the action itself to its domains and codomains?
</p>
<p>
If you really dislike the idea of having an option switched off by default (and a priori only on in the coercion model and not outside it unless one knows what he is doing), we could mimick what you wanna do with the <a class="missing wiki">TripleDict?</a> and have the usual Functor and a Functor_weakref version (or you can go the other way around and add an extra arg to the <a class="missing wiki">TripleDict?</a> constructor taking the equality operator as argument :) rather than having two classes)?
</p>
TicketSimonKingFri, 30 Dec 2011 15:20:35 GMT
https://trac.sagemath.org/ticket/11521#comment:104
https://trac.sagemath.org/ticket/11521#comment:104
<p>
Interestingly, the experimental patch for <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> that I have not submitted yet (need more tests) seems to be almost enough to fix the memory leak from the ticket description!
</p>
<p>
Namely:
</p>
<pre class="wiki">sage: <the usual loop>
Traceback
...
KeyboardInterrupt:
sage: import gc
sage: gc.collect()
585
sage: from sage.schemes.generic.homset import SchemeHomsetModule_abelian_variety_coordinates_field
sage: LE = [x for x in gc.get_objects() if isinstance(x,SchemeHomsetModule_abelian_variety_coordinates_field)]
sage: len(LE)
2
</pre><p>
And the experimental patch is <em>not</em> using weak references to the values of the <code>TripleDict</code> - only to the keys! Perhaps this ticket will eventually be a duplicate of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>?
</p>
<p>
Anyway, I need more tests and will probably submit the patch to <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> later today.
</p>
TicketjpfloriFri, 30 Dec 2011 15:25:24 GMT
https://trac.sagemath.org/ticket/11521#comment:105
https://trac.sagemath.org/ticket/11521#comment:105
<p>
If you used "==" for equality testing for actions in the coercion model (as in the parent caches) and let "j=j" in the "usual piece of code", this is not surprising because for "==" all the curves are equal, so there will be no duplication nor memory leak.
</p>
<p>
To perform a better test, you should replace j=j by j=K.random_element() in the constructor of the elliptic curve (as I did a few tickets up) to get really different curves (i.e. for "is" and for "==").
I fear the memory leak is still there with this second example...
</p>
TicketSimonKingFri, 30 Dec 2011 23:18:12 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac715_two_tripledicts.patch</em>
</li>
</ul>
<p>
Experimental: Have to versions of <code>TripleDict</code>, using "==" or "is" for comparison
</p>
TicketSimonKingFri, 30 Dec 2011 23:26:52 GMT
https://trac.sagemath.org/ticket/11521#comment:106
https://trac.sagemath.org/ticket/11521#comment:106
<p>
Sorry, I have just posted a patch to the wrong ticket. The new patch belongs to <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, not to here. Just ignore it.
</p>
TicketSimonKingTue, 03 Jan 2012 11:08:20 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/11521#comment:107
https://trac.sagemath.org/ticket/11521#comment:107
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.8</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
<p>
Concerning "wrong ticket": Shouldn't we consider this ticket a duplicate of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>? After all, the examples from the two ticket descriptions are almost identical.
</p>
TicketjpfloriTue, 03 Jan 2012 11:12:34 GMT
https://trac.sagemath.org/ticket/11521#comment:108
https://trac.sagemath.org/ticket/11521#comment:108
<p>
That's what I originally suggested :)
See the first comments on this ticket.
</p>
<p>
When I found about ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> I copied the description from here to there (I must have introduced some typographical difference in the way) because I thought the problem belonged there and the original description of ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> was non-existent.
</p>
TicketSimonKingTue, 03 Jan 2012 11:48:09 GMT
https://trac.sagemath.org/ticket/11521#comment:109
https://trac.sagemath.org/ticket/11521#comment:109
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:108" title="Comment 108">jpflori</a>:
</p>
<blockquote class="citation">
<p>
That's what I originally suggested :)
See the first comments on this ticket.
</p>
<p>
When I found about ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> I copied the description from here to there (I must have introduced some typographical difference in the way) because I thought the problem belonged there and the original description of ticket <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> was non-existent.
</p>
</blockquote>
<p>
I see. And I also found that I was originally responsible for not making it a duplicate.
</p>
<p>
The reason was that I found another leak: It is the strong cache for the homsets, and that is not addressed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
<p>
So (question to the release manager), what shall we do? Mark this as a duplicate and open a different ticket for the cache of homsets? Or change the ticket description such that it is only about homsets, not about elliptic curves?
</p>
TicketzimmermaTue, 03 Jan 2012 15:51:20 GMT
https://trac.sagemath.org/ticket/11521#comment:110
https://trac.sagemath.org/ticket/11521#comment:110
<p>
before tagging that ticket as a duplicate of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, I'd like to check the leak is indeed
fixed with the (upcoming) patch of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
<p>
Paul
</p>
TicketjpfloriTue, 03 Jan 2012 15:56:12 GMT
https://trac.sagemath.org/ticket/11521#comment:111
https://trac.sagemath.org/ticket/11521#comment:111
<p>
I fear the work on <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> is far from being finished...
</p>
TicketSimonKingWed, 04 Jan 2012 16:11:49 GMTstatus, description, milestone changed
https://trac.sagemath.org/ticket/11521#comment:112
https://trac.sagemath.org/ticket/11521#comment:112
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=112">diff</a>)
</li>
<li><strong>milestone</strong>
changed from <em>sage-duplicate/invalid/wontfix</em> to <em>sage-5.0</em>
</li>
</ul>
TicketSimonKingWed, 04 Jan 2012 16:13:18 GMT
https://trac.sagemath.org/ticket/11521#comment:113
https://trac.sagemath.org/ticket/11521#comment:113
<p>
The original example of this ticket is in fact fixed with <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, but I think the other problem should be dealt with here, namely by using a weak cache for homsets.
</p>
TicketSimonKingWed, 04 Jan 2012 16:36:14 GMT
https://trac.sagemath.org/ticket/11521#comment:114
https://trac.sagemath.org/ticket/11521#comment:114
<p>
Note that with the patch applied on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, the memleak is fixed:
</p>
<pre class="wiki">sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: import gc
sage: gc.collect()
8320
sage: LE = [x for x in gc.get_objects() if isinstance(x,type(K))]
sage: len(LE)
2
sage: LE
[Finite Field of size 2, Finite Field of size 99991]
</pre><p>
Later today, I'll run doctests.
</p>
TicketSimonKingWed, 04 Jan 2012 16:37:54 GMT
https://trac.sagemath.org/ticket/11521#comment:115
https://trac.sagemath.org/ticket/11521#comment:115
<p>
Note that the patch was written before the latest version of the patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> was created. In particular, it uses <code>TripleDict</code>, but should better use <code>TripleDictById</code> (which hasn't been there when I wrote the patch).
</p>
TicketSimonKingWed, 04 Jan 2012 19:26:36 GMT
https://trac.sagemath.org/ticket/11521#comment:116
https://trac.sagemath.org/ticket/11521#comment:116
<p>
Not bad: <code>make ptest</code> results in only one error with the patch and its dependencies applied on top of sage-5.0.prealpha0:
</p>
<pre class="wiki">sage -t -force_lib "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
Exception AttributeError: 'PolynomialRing_field_with_category' object has no attribute '_modulus' in ignored
Exception AttributeError: 'PolynomialRing_field_with_category' object has no attribute '_modulus' in ignored
**********************************************************************
File "/home/simon/SAGE/sage-5.0.prealpha0/devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 418:
sage: len(ring_refcount_dict) == n
Expected:
True
Got:
False
**********************************************************************
1 items had failures:
1 of 18 in __main__.example_4
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/simon/.sage//tmp/multi_polynomial_libsingular_6755.py
[3.6 s]
----------------------------------------------------------------------
The following tests failed:
sage -t -force_lib "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
Total time for all tests: 3.6 seconds
</pre>
TicketSimonKingThu, 05 Jan 2012 12:12:22 GMT
https://trac.sagemath.org/ticket/11521#comment:117
https://trac.sagemath.org/ticket/11521#comment:117
<p>
It turns out that the failing test had in fact a wrong design. It was:
</p>
<pre class="wiki"> sage: import gc
sage: from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular
sage: from sage.libs.singular.ring import ring_refcount_dict
sage: n = len(ring_refcount_dict)
sage: R = MPolynomialRing_libsingular(GF(547), 2, ('x', 'y'), TermOrder('degrevlex', 2))
sage: len(ring_refcount_dict) == n + 1
True
sage: Q = copy(R) # indirect doctest
sage: p = R.gen(0) ^2+R.gen(1)^2
sage: q = copy(p)
sage: del R
sage: del Q
sage: del p
sage: del q
sage: gc.collect() # random output
sage: len(ring_refcount_dict) == n
True
</pre><p>
Hence, before n is defined, no garbage collection takes place. This is, of course, not correct: The ring_refcount_dict may contain references to a ring created in another doctest, that is only garbage collected in the line before <code>len(ring_refcount_dict)==n</code>.
</p>
<p>
In other words: The test did not fail <em>because</em> there was a memory leak.
</p>
<p>
When I insert a garbage collection right before the definition of n, the test works, and in addition the warning about <code>AttributeError</code> being ignored vanishes.
</p>
<p>
The patch fixes the memory leak caused by a strong homset cache (which was not addressed by <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>):
</p>
<pre class="wiki">sage: for p in prime_range(10^3):
....: K = GF(p)
....: a = K(0)
....:
sage: import gc
sage: gc.collect()
3128
sage: LE = [x for x in gc.get_objects() if isinstance(x,type(K))]
sage: LE
[Finite Field of size 2, Finite Field of size 997]
</pre><p>
The patch adds a new doctest demonstrating that it is fixed.
</p>
<p>
Needs review!
</p>
TicketjpfloriThu, 05 Jan 2012 17:20:32 GMT
https://trac.sagemath.org/ticket/11521#comment:118
https://trac.sagemath.org/ticket/11521#comment:118
<p>
I'll this patch as soon as 715 is finally closed.
</p>
<p>
I think the ticket title also needs to be changed to reflect the issue addressed, namely that when homset are created, a strong ref is kept forever in sage.???.homset.
</p>
TicketSimonKingMon, 09 Jan 2012 23:43:12 GMTstatus, summary changed
https://trac.sagemath.org/ticket/11521#comment:119
https://trac.sagemath.org/ticket/11521#comment:119
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>summary</strong>
changed from <em>Memleak when resolving the action of Integers on an Elliptic Curve</em> to <em>Use weak references to cache homsets</em>
</li>
</ul>
<p>
When applying the new patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, all tests pass. But when also applying the patch from here, <code>make ptest</code> results in
</p>
<pre class="wiki">sage -t -force_lib devel/sage/sage/libs/singular/ring.pyx # 6 doctests failed
</pre><p>
The failing tests concern <code>ring_refcount_dict</code> - and it is perhaps not surprising that a patch enabling garbage collection of rings has an influence on refcount, such as:
</p>
<pre class="wiki"> sage: ring_ptr = set(ring_refcount_dict.keys()).difference(prev_rings).pop()
Exception raised:
Traceback (most recent call last):
File "/home/simon/SAGE/sage-5.0.prealpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/home/simon/SAGE/sage-5.0.prealpha0/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/home/simon/SAGE/sage-5.0.prealpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_8[9]>", line 1, in <module>
ring_ptr = set(ring_refcount_dict.keys()).difference(prev_rings).pop()###line 452:
sage: ring_ptr = set(ring_refcount_dict.keys()).difference(prev_rings).pop()
KeyError: 'pop from an empty set'
</pre><p>
Anyway, I need to analyse what happens, and provide a new patch after catching some sleep.
</p>
<p>
Jean-Pierre, I hope you like the new ticket title!
</p>
TicketjpfloriTue, 10 Jan 2012 06:54:39 GMT
https://trac.sagemath.org/ticket/11521#comment:120
https://trac.sagemath.org/ticket/11521#comment:120
<p>
Yes, better title !
</p>
<p>
I've not have any time to have a look at these cache tickets in the last few days, but be sure I'll have a look soon.
</p>
TicketSimonKingTue, 10 Jan 2012 08:26:27 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac11521_triple_homset.patch</em>
</li>
</ul>
<p>
Use the weak <code>TripleDict</code> from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> for the cache of homsets
</p>
TicketSimonKingTue, 10 Jan 2012 08:30:45 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:121
https://trac.sagemath.org/ticket/11521#comment:121
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
The doctest problem was easy enough to solve: When starting the example with a garbage collection, then things work. Actually I am surprised that the doctest framework does not do a garbage collection at the beginning of each example, in order to bring the system in a defined state - I'll ask on sage-devel.
</p>
<p>
Anyway, since the problem is solved and the patch updated, stuff is now ready for review!
</p>
<p>
Apply trac11521_triple_homset.patch
</p>
TicketjpfloriTue, 24 Jan 2012 13:42:37 GMT
https://trac.sagemath.org/ticket/11521#comment:122
https://trac.sagemath.org/ticket/11521#comment:122
<p>
Ive look at the patch and has nothing to say except for weakref being imported twice, I'll provided a reviewer patch for that.
</p>
<p>
Ha, maybe we should mention the caching system in the doc as well, I'm ready to do that in a reviewer patch as well.
</p>
<p>
However, I'll wait for <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> to be closed before positive reviewing this one, just in case the changes there have consequences here (which should not be the case).
</p>
TicketSimonKingTue, 24 Jan 2012 15:12:48 GMT
https://trac.sagemath.org/ticket/11521#comment:123
https://trac.sagemath.org/ticket/11521#comment:123
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:122" title="Comment 122">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Ha, maybe we should mention the caching system in the doc as well, I'm ready to do that in a reviewer patch as well.
</p>
</blockquote>
<p>
Oops, you are right! I thought I had add to the docs, but I just added a doctest, no elaborate explanation.
</p>
TicketjpfloriWed, 21 Mar 2012 09:15:05 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:124
https://trac.sagemath.org/ticket/11521#comment:124
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
While finally looking at this ticket seriously and adding some doc, I remarked that if you run
</p>
<pre class="wiki">sage: V = VectorSpace(QQ,3)
sage: H = Hom(V,V)
sage: H is Hom(V,V)
False
</pre><p>
With this tickets patches but also WITHOUT them.
</p>
<p>
Is this a consequence of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9138" title="defect: Categories for all rings (closed: fixed)">#9138</a> ?
</p>
<p>
What's strange is that V does not break the unique parent assumption:
</p>
<pre class="wiki">sage: V is VectorSpace(QQ, 3)
True
</pre><p>
Is that intended because there is a ? It does not look very right to me.
</p>
<p>
The same "problem" occurs playing for example with finite fields...
</p>
<p>
With more classical stuff, the cache seems to work as intended
</p>
<pre class="wiki">sage: H = Hom(ZZ, QQ)
sage: H is Hom(ZZ, QQ)
True
</pre><p>
This last point let me think that we can discuss and fix the <a class="missing wiki">VectorSpace?</a> situation elsewhere if this is indeed a problem.
</p>
<p>
My concern here is that it's hard to provide a "smart" example where the hom set is cached and where it can be garbage collected when its domainand codomain are.
</p>
<p>
Id on't think deleting ZZ and QQ is a good idea :) (and they would be refed elsewhere anyway)
</p>
TicketjpfloriWed, 21 Mar 2012 09:37:41 GMT
https://trac.sagemath.org/ticket/11521#comment:125
https://trac.sagemath.org/ticket/11521#comment:125
<p>
My bad, in fact the "issue" seems to be that X._Hom_(Y, category) succeeds but returns non identical results.
</p>
TicketjpfloriWed, 21 Mar 2012 10:18:42 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac_11521-reviewer.patch</em>
</li>
</ul>
<p>
Reviewer patch; added doc
</p>
TicketjpfloriWed, 21 Mar 2012 10:21:03 GMTstatus, description changed
https://trac.sagemath.org/ticket/11521#comment:126
https://trac.sagemath.org/ticket/11521#comment:126
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=126">diff</a>)
</li>
</ul>
<p>
Here is a reviewer patch added a little doc.
</p>
<p>
I hope you will find it satisfactory.
</p>
<p>
If so, feel free to set the ticket to positive review, I personnally feel happy with youre code.
</p>
<p>
My above rant, if it should be addressed, should be elsewhere.
</p>
TicketdavidloefflerMon, 26 Mar 2012 13:25:13 GMT
https://trac.sagemath.org/ticket/11521#comment:127
https://trac.sagemath.org/ticket/11521#comment:127
<p>
To reiterate what I just posted at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, I've just successfully run doctests against 5.0.beta10 with qseries
</p>
<pre class="wiki">trac715_one_triple_dict.patch
trac_715-reviewer.patch
trac_715-rebase_11599.patch
trac11521_triple_homset.patch
trac_11521-reviewer.patch
</pre><p>
The reviewer patch from jpflori looks fine to me. Is this ready to go now?
</p>
TicketSimonKingMon, 26 Mar 2012 13:28:53 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/11521#comment:128
https://trac.sagemath.org/ticket/11521#comment:128
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Jean-Pierre Flori</em>
</li>
</ul>
<p>
The reviewer patch looks fine to me. Sorry that I did not react a few days ago. Since Jean-Pierre said I should set it to positive review, I am now doing so.
</p>
TicketdavidloefflerMon, 26 Mar 2012 13:42:11 GMTdescription changed
https://trac.sagemath.org/ticket/11521#comment:129
https://trac.sagemath.org/ticket/11521#comment:129
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=129">diff</a>)
</li>
</ul>
TicketjdemeyerSun, 01 Apr 2012 19:21:49 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:130
https://trac.sagemath.org/ticket/11521#comment:130
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.0</em> to <em>sage-pending</em>
</li>
</ul>
TicketSimonKingSat, 14 Apr 2012 12:03:36 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac_11521_homset_weakcache_combined.patch</em>
</li>
</ul>
<p>
Use the weak <a class="missing wiki">TripleDict?</a> from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> for the cache of homsets. Includes the reviewer patch
</p>
TicketSimonKingSat, 14 Apr 2012 12:05:38 GMTdescription changed
https://trac.sagemath.org/ticket/11521#comment:131
https://trac.sagemath.org/ticket/11521#comment:131
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=131">diff</a>)
</li>
</ul>
<p>
I have created a combined patch, by folding the two patches (i.e., mine and Jean-Pierre's reviewer patch).
</p>
<p>
With <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and this patch, all doctests pass on my machine. So, from my perspective, it is a positive review, but we should wait for Jean-Pierre's results on 32 bit.
</p>
<p>
For the patchbot:
</p>
<p>
Apply trac_11521_homset_weakcache_combined.patch
</p>
TicketcremonaTue, 17 Apr 2012 19:05:25 GMT
https://trac.sagemath.org/ticket/11521#comment:132
https://trac.sagemath.org/ticket/11521#comment:132
<p>
I'm just confirming that applying this patch & that at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> to 5.0-beta13 on a 32-bit linux machine, all tests pass.
</p>
TicketjdemeyerThu, 26 Apr 2012 22:36:38 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:133
https://trac.sagemath.org/ticket/11521#comment:133
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.1</em>
</li>
</ul>
TicketjdemeyerSun, 06 May 2012 12:14:48 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11521#comment:134
https://trac.sagemath.org/ticket/11521#comment:134
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.1.beta0</em>
</li>
</ul>
TicketjdemeyerThu, 05 Jul 2012 08:37:08 GMTstatus changed; resolution, merged deleted
https://trac.sagemath.org/ticket/11521#comment:135
https://trac.sagemath.org/ticket/11521#comment:135
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
<li><strong>merged</strong>
<em>sage-5.1.beta0</em> deleted
</li>
</ul>
<p>
Unmerged because unmerging <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> while keeping <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> gives on OS X 10.6:
</p>
<pre class="wiki">$ ./sage -t --verbose devel/sage/sage/misc/cachefunc.pyx
[...]
Trying:
P = QQ['a, b, c, d']; (a, b, c, d,) = P._first_ngens(4)###line 1038:_sage_ >>> P.<a,b,c,d> = QQ[]
Expecting nothing
------------------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off(). You might
want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate.
------------------------------------------------------------------------
</pre>
TicketjdemeyerThu, 05 Jul 2012 08:55:25 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:136
https://trac.sagemath.org/ticket/11521#comment:136
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.1</em> to <em>sage-5.2</em>
</li>
</ul>
TicketjdemeyerThu, 05 Jul 2012 12:40:36 GMTdependencies changed
https://trac.sagemath.org/ticket/11521#comment:137
https://trac.sagemath.org/ticket/11521#comment:137
<ul>
<li><strong>dependencies</strong>
changed from <em>#11900 #715</em> to <em>#11900, #715, to be merged with #12313</em>
</li>
</ul>
TicketjdemeyerFri, 13 Jul 2012 11:52:26 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:138
https://trac.sagemath.org/ticket/11521#comment:138
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketjdemeyerFri, 13 Jul 2012 11:52:33 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:139
https://trac.sagemath.org/ticket/11521#comment:139
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketjdemeyerFri, 13 Jul 2012 11:53:18 GMTdependencies changed
https://trac.sagemath.org/ticket/11521#comment:140
https://trac.sagemath.org/ticket/11521#comment:140
<ul>
<li><strong>dependencies</strong>
changed from <em>#11900, #715, to be merged with #12313</em> to <em>to be merged with #715</em>
</li>
</ul>
TicketSimonKingMon, 23 Jul 2012 14:54:26 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:141
https://trac.sagemath.org/ticket/11521#comment:141
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I found that all tests pass on bsd.math, with
</p>
<pre class="wiki">trac12969_fix_coercion_cache.patch
trac12215_weak_cached_function-sk.patch
trac12215_segfault_fixes.patch
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12313-mono_dict-combined-random-sk.patch
</pre><p>
applied on top of sage-5.2.rc0. I think this justifies returning to needs_review. Or what is the work issue?
</p>
TicketjpfloriMon, 23 Jul 2012 17:50:04 GMT
https://trac.sagemath.org/ticket/11521#comment:142
https://trac.sagemath.org/ticket/11521#comment:142
<p>
I guess it main l'y lot of testing. Thé code looked fine to me. But there were these random bugs potentially caused by à bad interaction with another part of the code. Hopefully you fixed this with your latest conntributions.
</p>
TicketSimonKingMon, 23 Jul 2012 20:18:36 GMTdependencies changed
https://trac.sagemath.org/ticket/11521#comment:143
https://trac.sagemath.org/ticket/11521#comment:143
<ul>
<li><strong>dependencies</strong>
changed from <em>to be merged with #715</em> to <em>#12969; to be merged with #715</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:142" title="Comment 142">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I guess it main l'y lot of testing. Thé code looked fine to me. But there were these random bugs potentially caused by à bad interaction with another part of the code. Hopefully you fixed this with your latest conntributions.
</p>
</blockquote>
<p>
I think at some point <a class="closed ticket" href="https://trac.sagemath.org/ticket/12969" title="defect: Coercion failures in symmetric functions (closed: fixed)">#12969</a> must be used (which has fixed an error that depends on the order in which tests are executed), so, I made it a dependency. See the list of patches in my previous comment.
</p>
<p>
What sporadic errors did actually occur, and on what system? I did one test run on bsd.math. Since the errors were sporadic, it might be good to test different variants (serially, parallely, with make test or with sage -pt) and multiple times.
</p>
TicketSimonKingTue, 24 Jul 2012 06:17:25 GMT
https://trac.sagemath.org/ticket/11521#comment:144
https://trac.sagemath.org/ticket/11521#comment:144
<p>
I have run the tests on bsd.math in different ways. There has been no error.
</p>
<p>
According to old logs of the patch bot,
</p>
<pre class="wiki">sage -t -force_lib "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py"
</pre><p>
used to crash sometimes. I repeated that test about 20 times both on my <code>OpenSuse</code> laptop and on bsd.math. Everything was fine.
</p>
<p>
Hence, I am confident that <a class="closed ticket" href="https://trac.sagemath.org/ticket/12969" title="defect: Coercion failures in symmetric functions (closed: fixed)">#12969</a> is indeed enough to solve the problems that were revealed by the other bunch of patches (<a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>).
</p>
TicketnbruinWed, 15 Aug 2012 20:01:23 GMT
https://trac.sagemath.org/ticket/11521#comment:145
https://trac.sagemath.org/ticket/11521#comment:145
<p>
<strong>sage/categories/homset.py</strong>:
</p>
<pre class="wiki">_cache = TripleDict(53)
</pre><p>
I guess this 53 is a tuned parameter. Can you comment on the choice?
</p>
<p>
line 214
</p>
<pre class="wiki"> try:
H = _cache[key]()
except KeyError:
H = None
if H:
</pre><p>
Could it happen that bool(H) is false if H is not None? Perhaps safer to test
<code>H is not None</code>? I'm not saying that you should. Just comment please.
</p>
<p>
line 247 (same thing)
</p>
<p>
line 263
</p>
<pre class="wiki"> _cache[key] = weakref.ref(H)
</pre><p>
Can you clarify the following: The way I read this is that H will not be
protected against GC (good!), but that <code>_cache</code> will be be storing a strong(?)
reference to <code>key</code>, meaning that the entry in <code>_cache</code> under <code>key</code> will linger
with a dead weakref afterwards. Shouldn't there be a callback registered here
with the weakref that contacts <code>_cache</code> to remove the <code>key</code> entry when <code>H</code> gets
collected? It may be that your <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> changes already make <code>_cache</code> into essentially a
<code>WeakKeyDict</code>, but it seems to me that <code>_cache</code> should still have a callback
registered on the <code>weakref</code> to <code>H</code> as well, to remove the entry if <code>H</code> gets GCd.
</p>
<p>
I haven't done extensive testing myself, but you have and the bot happy, so I don't think me running tests would change much.
</p>
TicketSimonKingThu, 16 Aug 2012 14:57:02 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:146
https://trac.sagemath.org/ticket/11521#comment:146
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:145" title="Comment 145">nbruin</a>:
</p>
<blockquote class="citation">
<p>
<strong>sage/categories/homset.py</strong>:
</p>
<pre class="wiki">_cache = TripleDict(53)
</pre><p>
I guess this 53 is a tuned parameter. Can you comment on the choice?
</p>
</blockquote>
<p>
My patch in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> is, of course, based on old code. In the old code, there were comments on the choice of the parameter, that I summarise here. The parameter gives the initial number of buckets in a <code>TripleDict</code>. Thus, in order to ensure a good distribution of the items into the buckets, the parameter should be prime and should certainly not be even; there actually is a doc test that shows what happens if the parameter is even. It should not be too big, because it determines the number of empty buckets that are created during initialisation of the <code>TripleDict</code>. And it should not be too small, because otherwise the <code>TripleDict</code> would soon be resized (which is a costly operation).
</p>
<p>
I thought that 53 is a good choice, given these comments. However, it is not tuned by experiments.
</p>
<blockquote class="citation">
<p>
line 214 ...
Could it happen that bool(H) is false if H is not None? Perhaps safer to test
<code>H is not None</code>? I'm not saying that you should. Just comment please.
</p>
</blockquote>
<p>
That line is old code that has not been introduced by my patch, right? I think one <em>should</em> test <code>H is not None</code> - mainly for speed, but also for preventing that bool(H) is false even though H is not None.
</p>
<blockquote class="citation">
<p>
line 247 (same thing)
</p>
<p>
line 263
</p>
<pre class="wiki"> _cache[key] = weakref.ref(H)
</pre><p>
Can you clarify the following: The way I read this is that H will not be
protected against GC (good!), but that <code>_cache</code> will be be storing a strong(?)
reference to <code>key</code>, meaning that the entry in <code>_cache</code> under <code>key</code> will linger
with a dead weakref afterwards.
</p>
</blockquote>
<p>
Correct, at least theoretically.
</p>
<p>
The reason for the code was that at some point one needed to break a chain of strong references. In the memory leak fixed here, there was a reference from H back to the keys under which H is stored in the <code>TripleDict</code>. Hence, practically, when H gets collected, the item in the <code>TripleDict</code> becomes collectable as well. Hence, the dead weakref will soon vanish.
</p>
<p>
At least, that's what <em>should</em> happen. But:
</p>
<blockquote class="citation">
<p>
Shouldn't there be a callback registered here
with the weakref that contacts <code>_cache</code> to remove the <code>key</code> entry when <code>H</code> gets
collected? It may be that your <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> changes already make <code>_cache</code> into essentially a
<code>WeakKeyDict</code>,
</p>
</blockquote>
<p>
Yes. A <code>WeakKeyDict</code>, but not a <code>WeakValueDict</code>. That's why I am using a weak reference to H.
</p>
<blockquote class="citation">
<p>
but it seems to me that <code>_cache</code> should still have a callback
registered on the <code>weakref</code> to <code>H</code> as well, to remove the entry if <code>H</code> gets GCd.
</p>
</blockquote>
<p>
Recall that each <code>TripleDict</code> has an eraser. I think, at least with the new patch at <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> that I have posted today, such a callback would be easily possible.
</p>
<blockquote class="citation">
<p>
I haven't done extensive testing myself, but you have and the bot happy, so I don't think me running tests would change much.
</p>
</blockquote>
<p>
Well, in an ideal world, a different brain will be able to invent different stress tests, thus, find different bugs...
</p>
<p>
Anyway. I think I should turn <code>if H:</code> into <code>if H is not None:</code> and should introduce a callback to the weak reference.
</p>
TicketSimonKingThu, 16 Aug 2012 15:13:27 GMT
https://trac.sagemath.org/ticket/11521#comment:147
https://trac.sagemath.org/ticket/11521#comment:147
<p>
Another comment: I use a weak reference to H (the homset) on the one hand, and a weak reference to the category C as the third argument to the <code>TripleDict</code>, on the other hand (see line 212). Since the keys of a <code>TripleDict</code> are compared by identity and not by equality, I can not use a callback function in the weak reference to C. Namely, weakref.ref(C) is identical with weakref.ref(C), whereas two weak references with callback are equal but not identical.
</p>
<p>
In conclusion, I will use a callback in the weak reference to H, so that the dictionary item gets collected as soon as H gets collected. But I think I can not use a callback in the weak reference to C. Anyway, a callback for C is not needed: If H is alive, it has a reference to C and keeps C alive. If H dies, then the callback for the weak reference to H will remove the item, and thus C can be collected as well.
</p>
TicketSimonKingThu, 16 Aug 2012 17:35:51 GMT
https://trac.sagemath.org/ticket/11521#comment:148
https://trac.sagemath.org/ticket/11521#comment:148
<p>
Now I wonder: Why did I explicitly put a weak reference to the category as a key for the <code>TripleDict</code>? Would the <code>TripleDict</code> not use weak references to <em>all three</em> parts of the key?
</p>
<p>
I think the reason for the "weakref.ref(category)" was that in older versions of <code>TripleDict</code> I did indeed not use a weak reference to the third part of the key.
</p>
TicketSimonKingThu, 16 Aug 2012 17:48:04 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac_11521_callback.patch</em>
</li>
</ul>
<p>
Use a callback, to make sure that items in the homset cache are deleted if the homset is garbage collected
</p>
TicketSimonKingThu, 16 Aug 2012 17:52:48 GMTstatus, description changed
https://trac.sagemath.org/ticket/11521#comment:149
https://trac.sagemath.org/ticket/11521#comment:149
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=149">diff</a>)
</li>
</ul>
<p>
OK, the additional patch has been posted. It removes the explicit weak reference to the category (<code>TripleDict</code> uses a weak reference anyway), adds a callback to the weak reference to the homset, so that an item of the homset cache gets deleted if the homset is garbage collected, and it replaces "if H:" by "if H is not None:".
</p>
<p>
I doctested sage/schemes/ (heuristics: Most bugs I ever authored resulted in a segfault in sage/schemes :), sage/structure/ and sage/categories/homset.py (hence, the memory leak remains fixed).
</p>
<p>
Apply <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> trac_11521_homset_weakcache_combined.patch trac_11521_callback.patch
</p>
TicketnbruinThu, 16 Aug 2012 22:08:02 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/11521#comment:150
https://trac.sagemath.org/ticket/11521#comment:150
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Jean-Pierre Flori</em> to <em>Jean-Pierre Flori, Nils Bruin</em>
</li>
</ul>
<p>
Good to go! (Note that the patchbot being happy here shows that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> is fine too. Tickets apparently cannot have post-dependencies)
</p>
TicketjdemeyerFri, 17 Aug 2012 09:32:31 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:151
https://trac.sagemath.org/ticket/11521#comment:151
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.3</em> to <em>sage-5.4</em>
</li>
</ul>
TicketnbruinMon, 20 Aug 2012 20:27:23 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:152
https://trac.sagemath.org/ticket/11521#comment:152
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Aw, this weakref caching stuff is very headache-inducing. My sincere apologies. I'm afraid I have to withdraw my positive review due to a previous failure in my logic. As I see it now, the current patch reduces the caching of coercion maps to near uselessness. The fact that it DOES seem to work to some extent might give a lead to why other things are going wrong. Let me try:
</p>
<p>
Ignoring the weakref on H for now, the coercion cache here consists of a <a class="missing wiki">TripleDict?</a>, stored on Y, indexing
</p>
<pre class="wiki">(X,Y,C) : H
</pre><p>
or
</p>
<pre class="wiki">(X,Y,C): None
</pre><p>
Here <code>H</code> is a map from <code>X</code> to <code>Y</code> wrt to the category <code>C</code>, so it stores strong references to <code>X</code>,<code>Y</code>,<code>C</code>. That means that as long as <code>H</code> is alive, no component of the key will die, so the fact that this is stored in a <code>TripleDict</code> is irrelevant: The value <code>H</code> would keep the key alive anyway. The only exception is the value <code>None</code>. There the <code>TripleDict</code> does exactly what it should.
</p>
<p>
This cycle problem is fixed by weakreffing <code>H</code>. But the whole point of the cache is to keep <code>H</code> alive as long as all of <code>X</code>, <code>Y</code> and <code>C</code> exist, so that we can look it up. Since the normal use for coercion would be to discover the coercion map and forget about it as soon as it has been applied, one would expect the cache to be empty all the time!
</p>
<p>
I don't immediately see a canonical way to solve this problem. Here are some ideas that may mitigate it:
</p>
<p>
We're caching coercions on the codomain, which I think is the right place for the following reason. Long-lived parents such as <code>ZZ</code> tend to have a lot of coercions <em>to</em> other parents, but very few <em>from</em>. As observed above, we are caching the absense of coercions, but those don't involve an <code>H</code> that might keep the codomain alive. So, we could cache the absense of coercions in a <code>TripleDict</code>. In fact, we could make that a <code>TripleSet</code> if we really want.
</p>
<p>
The PRESENCE of a coercion should probably be cached with a strong reference to the coercion. That's the point of caching the thing!
This should be keyed on <code>(X,C)</code> (the <code>Y</code> is implied by the cache we're looking in). Storing the key weakly has no effect with the present design of coercion.
</p>
<p>
That means that discovering those impose a memory cost on Y. However, usually there tend to not be too many of those (is that true in the p-adics with all those different precisions floating around? Still, that should be managable relative to creating all finite fields up to size 10<sup>9</sup> and storing strong references on <code>ZZ</code> to them to record the absence of a coercion from them, which I think was the problem in the example that started this ticket)
</p>
<p>
For permanent objects with coercions <em>from</em> lots of objects (like the symbolic ring, real/complex fields, [number fields with declared embeddings], <code>QQbar</code> [number fields again]) we have a big problem.
</p>
<p>
One solution would be to turn off caching for those (which needs infrastructure ... I guess a flag <code>Y.no_coercion_caching_on_me_please=True</code>, which needs checking any time you're about to cache something. Or, if that is too drastic, an optional parent method
<code>Y.is_this_a_coercion_cachable_domain(X)</code> or<code>Y.is_this_a_coercion_cachable_domain_type(type(X))</code>
</p>
<p>
Another would be to warn people that mixing parents makes the "larger" parent remember the "smaller" one, so if you let lots of "smaller" parent interact with a "larger" one, you'll be using memory for the lifetime of that "larger" one.
</p>
<p>
The "proper" solutions below would force coercion maps to be different from normal homomorphisms, fit for users. That's because if a user keeps a reference <code>h</code> to an element of <code>Hom(X,Y)</code>, he/she rightfully expects that to keep <code>X</code> and <code>Y</code> alive.
</p>
<p>
we'd have to weakly key the cache, but still strongly store the coercion map.
</p>
<p>
The "proper" (but possibly too expensive) option is to not store <code>X</code> on coercion maps. If we have <code>H=Y.coercion_map_from(X)</code> and <code>H</code> is used correctly, then <code>H(x)</code> can get access to X via <code>x.parent()</code> anyway, so <code>H</code> doesn't strictly need a reference to <code>X</code>.
</p>
<p>
So the object we store in the cache could be a map-like object <code>cH</code> that doesn't store its domain. Upon calling <code>Y.coercion_map_from(X)</code> we could wrap <code>cH</code> into a proper <code>Hom(X,Y)</code> member that does have a strong reference to X. This might be too expensive for something so fundamental as coercion.
</p>
<p>
<strong>TL;DR</strong>: Do not store a <code>KeyedRef</code> to <code>H</code> but store <code>H</code> itself in the cache. We won't solve the leak completely, but at least we're not leaking on the <em>absence</em> of coercions as we were before.
</p>
TicketjpfloriMon, 20 Aug 2012 20:38:03 GMT
https://trac.sagemath.org/ticket/11521#comment:153
https://trac.sagemath.org/ticket/11521#comment:153
<p>
I'm not sure I completely follow you.
Did you consider the fact that maps are also stored in the parent themselves? not only in the coercion model?
</p>
TicketjpfloriMon, 20 Aug 2012 20:44:56 GMT
https://trac.sagemath.org/ticket/11521#comment:154
https://trac.sagemath.org/ticket/11521#comment:154
<p>
In particular, please have a look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>.
</p>
TicketnbruinMon, 20 Aug 2012 21:33:23 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:155
https://trac.sagemath.org/ticket/11521#comment:155
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
OK, I retract my concern for <em>this patch</em>. No coercions are stored here! In particular, any <code>H</code> occurring here is a homset, not a map. This cache is simply ensuring that <code>Hom(X,Y)</code> is unique. So indeed, this cache should not be preventing <code>H</code> from being collected. The <code>KeyedRef</code> is entirely in place.
</p>
<p>
I <code>was</code> looking at <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> when this struck me and I think the discussion above has relevance to that case. Once a coercion <code>h: X -> Y</code> is discovered, a strong reference to <code>h</code> is stored in <code>self._coerce_from_hash</code>. Since <code>h</code> stores a strong reference to <code>X</code> we now have ensured that the life time of <code>X</code> is bounded below by the life time of <code>Y</code>. So the weak caching there only helps for <code>X</code> that do not coerce into <code>Y</code>.
</p>
<p>
How eagerly does the system use the <code>self._convert_from_hash</code>? It is quite likely that even with <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> in place,
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">for</span> p <span class="ow">in</span> prime_range<span class="p">(</span><span class="mi">1</span><span class="p">,</span><span class="mi">1000</span><span class="p">):</span>
k<span class="o">=</span>GF<span class="p">(</span>p<span class="p">)</span>
a<span class="o">=</span>k<span class="p">(</span><span class="mi">1</span><span class="p">)</span>
b<span class="o">=</span>ZZ<span class="p">(</span>a<span class="p">)</span>
</pre></div></div><p>
would still leak, due to the conversion <code>GF(p)->ZZ</code> being cached on <code>ZZ</code>.
That would make me believe we should probably not cache conversions. There are too many of them, so it's too easy to let sage discover enough of them that all objects in memory are in one connected component.
</p>
<p>
For coercion things are a little brighter because objects with lots of inbound arrows (near-universal codomains?) are rarer.
</p>
TicketjpfloriMon, 20 Aug 2012 21:34:38 GMT
https://trac.sagemath.org/ticket/11521#comment:156
https://trac.sagemath.org/ticket/11521#comment:156
<p>
You definitely already had a look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, sorry about that.
</p>
<p>
Trying to summarize the action of the different coercion patches, here is what I remember:
</p>
<ul><li>in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, see <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/715#comment:75"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/715#comment:75</a> and <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/715#comment:86"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/715#comment:86</a>
<ul><li>we now store weakref to the set acted upon in Actions.
</li><li>parents use a <a class="missing wiki">TripleDict?</a> for _action_hash where the Actions they implement are stored with a strong ref.
</li><li>there is another <a class="missing wiki">TripleDict?</a> _action_maps in the coercion model with a strong ref as well.
</li><li>so if a parent acted upon dies, it will only be weakrefed in the Action itself and in the <a class="missing wiki">TripleDicts?</a> so that it will get garbage collected with the Action as well, but as long as both of them are alive, there should be no garbage collection.
</li></ul></li></ul><ul><li>but then <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> comes in because we also have a global cache for homset which prevented parents to die. So only there the tripl dicts store weakrefs to Action to enable gc back again. (just saw you just posted...)
</li></ul>
TicketjpfloriMon, 20 Aug 2012 21:43:03 GMT
https://trac.sagemath.org/ticket/11521#comment:157
https://trac.sagemath.org/ticket/11521#comment:157
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:155" title="Comment 155">nbruin</a>:
relevance to that case. Once a coercion <code>h: X -> Y</code> is discovered, a strong reference to <code>h</code> is stored in <code>self._coerce_from_hash</code>. Since <code>h</code> stores a strong reference to <code>X</code> we now have ensured that the life time of <code>X</code> is bounded below by the life time of <code>Y</code>. So the weak caching there only helps for <code>X</code> that do not coerce into <code>Y</code>.
</p>
<blockquote class="citation">
<p>
How eagerly does the system use the <code>self._convert_from_hash</code>? It is quite likely that even with <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> in place,
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">for</span> p <span class="ow">in</span> prime_range<span class="p">(</span><span class="mi">1</span><span class="p">,</span><span class="mi">1000</span><span class="p">):</span>
k<span class="o">=</span>GF<span class="p">(</span>p<span class="p">)</span>
a<span class="o">=</span>k<span class="p">(</span><span class="mi">1</span><span class="p">)</span>
b<span class="o">=</span>ZZ<span class="p">(</span>a<span class="p">)</span>
</pre></div></div></blockquote>
<p>
I see the problem.
Another solution may be to use <a class="missing wiki">TripleDicts?</a> there as well rather than <a class="missing wiki">MonoDicts?</a>.
Or <a class="missing wiki">DuoDicts?</a>!
</p>
TicketjpfloriMon, 20 Aug 2012 21:49:56 GMT
https://trac.sagemath.org/ticket/11521#comment:158
https://trac.sagemath.org/ticket/11521#comment:158
<p>
Disregard my above comment.
The point of <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> is exactly to let k be garbage collected, as the elliptic curves in <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> description.
</p>
TicketSimonKingMon, 20 Aug 2012 22:56:48 GMT
https://trac.sagemath.org/ticket/11521#comment:159
https://trac.sagemath.org/ticket/11521#comment:159
<p>
I this post, I clarify some notions and explain why the weak reference to a homset introduced here does <em>not</em> imply that the homset will immediately be garbage collected. Sorry, while I wrote that post, you clarified things for yourself, making my post redundant. Anyway, here it goes...
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:152" title="Comment 152">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Here <code>H</code> is a map from <code>X</code> to <code>Y</code> wrt to the category <code>C</code>,
so it stores strong references to <code>X</code>,<code>Y</code>,<code>C</code>.
</p>
</blockquote>
<p>
No, it is a homset. But yes, it has strong references to X,Y,C.
</p>
<blockquote class="citation">
<p>
The PRESENCE of a coercion should probably be cached with a strong reference to the coercion. That's the point of caching the thing!
</p>
</blockquote>
<p>
Yes. But that should happen in a way that domain and codomain remain collectable. And that is a problem; see below.
</p>
<p>
But I think there is a lot of confusion now between coercion, homset, <code>TripleDict</code> post and prior to <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, and the way how coercion maps are cached. Let me try to straighten things a bit:
</p>
<p>
The original purpose of <code>TripleDict</code> was to store ACTIONS (not coercions). When you have an action of X on Y, then you would store the action in an attribute of X, addressed by the key triple that is formed by Y, the information what operation is considered (+ or *), and the information whether the action is on the left or on the right.
</p>
<p>
The original <code>TripleDict</code> did so by strong references to both keys and values. By <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, <code>TripleDict</code> got changed so that one has weak references to the keys.
</p>
<p>
Before <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, that was the <em>only</em> application of <code>TripleDict</code>. But here, I suggest a second application, namely: Use it to store homsets. A "homset" from X to Y in the category C is the parent that contains all morphisms from X to Y in the category C (regardless whether these morphisms are coercion maps or conversion maps or anything else).
</p>
<p>
Let me emphasize again that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> do not change the way coercions are stored.
</p>
<p>
How are coercions stored? Before <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, the coercions from X to Y were stored in a strong dictionary in Y, strongly keyed with X. Hence, as long as Y survives, there would be a strong reference to X. But with <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, a weak dictionary is introduced for that purpose. Hence, even if Y persists and a coercion has been cached from X to Y, X remains collectable.
</p>
<p>
Now, back to caching <code>H = Hom(X,Y,category=C)</code> (H is a set of maps). You are right, having just a weak reference to H sounds like it would be immediately collected. If I recall correctly, my reasoning for introducing the weak reference to H has been as follows:
</p>
<ul><li>Let T be the cache for the Hom function. T is a <code>TripleDict</code>. and T is in fact available under sage.categories.homset._cache. Hence, we have a strong permanent reference to T.
</li><li>T provides some buckets, and of course T has strong references to its buckets.
</li><li>With <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, the buckets contain some memory addresses representing the keys, and then strong references to the values.
</li></ul><p>
By consequence, defining T[X,Y,C]=H means that we have a chain of strong references, starting with T (T is permanent), from T to its buckets, from the buckets to H, and from H to X, Y and C. Hence, X,Y and C will never be garbage collected, even though T itself only has weak references to X,Y and C.
</p>
<p>
Now, only define T[X,Y,C]=weakref.ref(H) (or any other kind of weak reference to H). Why is H <em>not</em> immediately garbage collected, in usual applications?
</p>
<p>
Usual application means: You will not just create H, but you will also create an element of H, say, phi. We have a strong reference from phi to H, because H is the parent of phi. If you do not store phi, then H remains collectable. But in a usual application, you would store phi, say, because it is a coerce map from X to Y.
</p>
<p>
By now, we assume that phi is a coercion, which is cached in Y. To be precise, we have Y._coerce_from_hash[X] = phi.
</p>
<p>
Prior to <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, Y._coerce_from_hash is a usual dict. Hence, the existence of Y keeps X, phi and thus H alive.
</p>
<p>
With <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, Y._coerce_from_hash is a <code>MonoDict</code>, which only has a weak reference to X. However, Y._coerce_from_hash has a strong reference to its buckets, the buckets have a strong reference to phi, phi has a strong reference to its parent H, and H has a strong reference to X. Hence, we have the reference cycle (with -> weak and => strong references)
</p>
<pre class="wiki">* => T -> H
Y -> X
Y => phi => H => X
H => Y
H => C
</pre><p>
In conclusion, an external reference to Y will keep X alive (which is bad). But if there is no external reference to Y nor to X, then Y,X,H and C remain collectable (assuming that no <code>__del__</code> method makes the cyclic garbage collection impossible), which is good enough to fix a memleak.
</p>
<p>
<strong><span class="underline">Summary</span></strong>
</p>
<ul><li>If H=Hom(X,Y,C) and the homset cache keeps a strong reference to H, then H, X, Y and C will never be collectable. That's why I introduce the weak reference, strange as it may look.
</li><li>If H=Hom(X,Y,C) is created as the parent of a coercion or conversion map phi, which is stored in Y._coerce_from_cache, then:
<ul><li>An external strong reference to Y keeps phi, thus H, thus X and C alive (*).
</li><li>An external strong reference to X does <em>not</em> prevent Y, H or C from being collected.
</li></ul></li></ul><p>
In an ideal world, (*) would be improved: An external strong reference to Y would <em>not</em> be enough to keep X alive. Do you have any idea how this can be implemented?
</p>
TicketSimonKingTue, 21 Aug 2012 08:18:47 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11521#comment:160
https://trac.sagemath.org/ticket/11521#comment:160
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Test activity of weak references if addresses coincide</em>
</li>
</ul>
<p>
See my comment at <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/13370#comment:17"><span class="icon"></span>13370</a>: To be on the safe side, i.e., in order to avoid that a key is deallocated but its callback function isn't called, one could/should first look at the memory address, and then (if the addresses coincide) test whether the stored weak reference is still active. Namely, if it isn't then the new key is <em>really</em> new, even though using an old address.
</p>
<p>
Hence, I'd like to put this and <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> to "needs work".
</p>
TicketSimonKingTue, 21 Aug 2012 08:35:22 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/11521#comment:161
https://trac.sagemath.org/ticket/11521#comment:161
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>Test activity of weak references if addresses coincide</em> deleted
</li>
</ul>
<p>
Ooops, the "needs work" should have been <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>...
</p>
TicketnbruinFri, 24 Aug 2012 23:10:42 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:162
https://trac.sagemath.org/ticket/11521#comment:162
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketjdemeyerMon, 10 Sep 2012 06:32:11 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:163
https://trac.sagemath.org/ticket/11521#comment:163
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.4</em> to <em>sage-pending</em>
</li>
</ul>
TicketSimonKingMon, 17 Sep 2012 09:37:51 GMTdescription changed
https://trac.sagemath.org/ticket/11521#comment:164
https://trac.sagemath.org/ticket/11521#comment:164
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=164">diff</a>)
</li>
</ul>
TicketjdemeyerSun, 23 Sep 2012 09:59:57 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:165
https://trac.sagemath.org/ticket/11521#comment:165
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.4</em>
</li>
</ul>
TicketjdemeyerSun, 23 Sep 2012 16:15:40 GMTmilestone changed
https://trac.sagemath.org/ticket/11521#comment:166
https://trac.sagemath.org/ticket/11521#comment:166
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.4</em> to <em>sage-5.5</em>
</li>
</ul>
<p>
SInce these tickets have caused some trouble in the past, I prefer to merge them only in a .beta0 (to maximize the testing), hence the milestone bump.
</p>
TicketjdemeyerSun, 23 Sep 2012 16:33:53 GMT
https://trac.sagemath.org/ticket/11521#comment:167
https://trac.sagemath.org/ticket/11521#comment:167
<p>
Is it still true that this needs to be merged with <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>? If yes, then <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> still depends on <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>.
</p>
TicketnbruinSun, 23 Sep 2012 20:32:15 GMTdescription changed
https://trac.sagemath.org/ticket/11521#comment:168
https://trac.sagemath.org/ticket/11521#comment:168
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=168">diff</a>)
</li>
</ul>
<p>
No, we do not need that merge. We should absolutely not add any more dependencies to these tickets. They are good to go as is (modulo nasty surprises). We can improve things afterwards. The bump to 5.5 is already unfortunate, because that means we've already unnecessarily missed one sailing.
</p>
<p>
<strong>EDIT:</strong> With <em>unfortunate</em> I do not mean to imply unwise. I fully respect the judgement of the release manager on this issue.
</p>
TicketjdemeyerMon, 24 Sep 2012 08:15:14 GMT
https://trac.sagemath.org/ticket/11521#comment:169
https://trac.sagemath.org/ticket/11521#comment:169
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:168" title="Comment 168">nbruin</a>:
</p>
<blockquote class="citation">
<p>
The bump to 5.5 is already unfortunate.
</p>
</blockquote>
<p>
I understand your feelings, but given that these kind of tickets have caused wierd not-always-reproducible bugs in the past, I think we should go for maximum testing coverage. That means, merging in some .beta0 instead of now in sage-5.4.beta2.
</p>
TicketSimonKingMon, 01 Oct 2012 12:58:08 GMT
https://trac.sagemath.org/ticket/11521#comment:170
https://trac.sagemath.org/ticket/11521#comment:170
<p>
I am not sure: Is <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> needed? Or does the strong cache for polynomial rings is agreed upon?
</p>
TicketjpfloriMon, 01 Oct 2012 13:01:49 GMT
https://trac.sagemath.org/ticket/11521#comment:171
https://trac.sagemath.org/ticket/11521#comment:171
<p>
Personally, I'd say merge this with the strong cache, (merge <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> if possible in between) and (finally) include <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> when it's ready.
</p>
TicketSimonKingMon, 01 Oct 2012 13:06:19 GMT
https://trac.sagemath.org/ticket/11521#comment:172
https://trac.sagemath.org/ticket/11521#comment:172
<p>
Yes, Jean-Pierre, I agree that <em>if</em> it is possible that start with a strong polynomial cache, and postpone the proper fix at <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>, because for now it isn't ready.
</p>
<p>
But perhaps the reviewers can cross-verify that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> (including a <em>strong</em> cache for polynomial rings) plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> is indeed fine without <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>? Do the tests all pass?
</p>
TicketnbruinMon, 01 Oct 2012 15:05:34 GMT
https://trac.sagemath.org/ticket/11521#comment:173
https://trac.sagemath.org/ticket/11521#comment:173
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:172" title="Comment 172">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
But perhaps the reviewers can cross-verify that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> (including a <em>strong</em> cache for polynomial rings) plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> is indeed fine without <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>? Do the tests all pass?
</p>
</blockquote>
<p>
I have always tested these tickets together. Plus, we <em>know</em> why a strong cache prevents the problems observed.
</p>
TicketjpfloriMon, 01 Oct 2012 15:12:15 GMT
https://trac.sagemath.org/ticket/11521#comment:174
https://trac.sagemath.org/ticket/11521#comment:174
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:173" title="Comment 173">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:172" title="Comment 172">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
But perhaps the reviewers can cross-verify that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> (including a <em>strong</em> cache for polynomial rings) plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> is indeed fine without <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>? Do the tests all pass?
</p>
</blockquote>
<p>
I have always tested these tickets together.
</p>
</blockquote>
<p>
This is not completely clear, do you mean:
</p>
<ul><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> without the strong cache + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> (which is highly unprobable because of the bsd segfault, but let's mention this solution)?
</li><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> with strong cache + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>?
</li><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> + <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> (assuming the strong cache is removed either from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> directly or by means of a second reverse patch in <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>)?
<blockquote class="citation">
<p>
Plus, we <em>know</em> why a strong cache prevents the problems observed.
</p>
</blockquote>
</li></ul><p>
I guess you mean the bsd bug.
Not the memory leaks problems these tickets try to tackle, is that right?
</p>
TicketSimonKingMon, 01 Oct 2012 15:27:21 GMT
https://trac.sagemath.org/ticket/11521#comment:175
https://trac.sagemath.org/ticket/11521#comment:175
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:174" title="Comment 174">jpflori</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I have always tested these tickets together.
</p>
</blockquote>
<p>
This is not completely clear, do you mean:
</p>
<ul><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> without the strong cache + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> (which is highly unprobable because of the bsd segfault, but let's mention this solution)?
</li></ul></blockquote>
<p>
This isn't a solution, because of the segfault.
</p>
<blockquote class="citation">
<ul><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> with strong cache + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>?
</li></ul></blockquote>
<p>
Yes. Do the tests pass in that setting?
</p>
<blockquote class="citation">
<ul><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> + <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> (assuming the strong cache is removed either from <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> directly or by means of a second reverse patch in <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>)?
</li></ul></blockquote>
<p>
That would be the long term aim, from my perspective. But getting the refcount right turned out to be trickier than I originally thought. Hence, for now (again: <em>IF</em> the tests pass) I'd prefer <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>+<a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> with the ugly strong cache for polynomial rings.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Plus, we <em>know</em> why a strong cache prevents the problems observed.
</p>
</blockquote>
<p>
I guess you mean the bsd bug.
Not the memory leaks problems these tickets try to tackle, is that right?
</p>
</blockquote>
<p>
Yes and no. A strong cache for polynomial rings would mean that my current Sage applications would eat too much memory - hence, yes, a strong cache is opposite to the spirit of these tickets. However, for all other parents, the tickets would still fix some memory leaks.
</p>
TicketjpfloriMon, 01 Oct 2012 15:33:04 GMT
https://trac.sagemath.org/ticket/11521#comment:176
https://trac.sagemath.org/ticket/11521#comment:176
<p>
Thanks, Simon.
I just hope (and expect) Nils to give a similar clarification.
</p>
<p>
I'm aware that the strong cache does induce (or prevents solving) leaks,
so obviously <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> is the next issue to tackle, especially for your own work as it seems.
</p>
<p>
As far as tests are concerned, all tests used to pass on my machines without the strong cache for polynomial.
If Nils reconfirms that (i assumee that's what he meant earlier), maybe correcting the ticket desciption by removind the reference to <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> and helping the patchbots to apply the patches correctly and letting them do their job should be enough now?
</p>
TicketSimonKingMon, 01 Oct 2012 16:38:46 GMT
https://trac.sagemath.org/ticket/11521#comment:177
https://trac.sagemath.org/ticket/11521#comment:177
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:176" title="Comment 176">jpflori</a>:
</p>
<blockquote class="citation">
<p>
As far as tests are concerned, all tests used to pass on my machines without the strong cache for polynomial.
</p>
</blockquote>
<p>
Yes, for me as well (except on bsd.math, of course). The question is: Is there a test (perhaps introduced at <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>) that relies on a <em>weak</em> polynomial ring cache?
</p>
<blockquote class="citation">
<p>
If Nils reconfirms that (i assumee that's what he meant earlier), maybe correcting the ticket desciption by removind the reference to <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> and helping the patchbots to apply the patches correctly and letting them do their job should be enough now?
</p>
</blockquote>
<p>
Sounds like a plan...
</p>
TicketnbruinMon, 01 Oct 2012 17:35:56 GMT
https://trac.sagemath.org/ticket/11521#comment:178
https://trac.sagemath.org/ticket/11521#comment:178
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:175" title="Comment 175">SimonKing</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> with strong cache + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>?
</li></ul></blockquote>
<p>
Yes. Do the tests pass in that setting?
</p>
</blockquote>
<p>
As far as I know, they do.
</p>
<p>
To be clear, the strong cache there is supposed to make things <em>not worse</em> for Singular rings. It's simply reinstating strong references that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>+<a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> removes (that's the idea at least), until we figure out how to properly collaborate with singular's (lack of) ref counting.
</p>
<p>
As it stands, this should make things <em>better</em> for many other parents. At this point the most important bit is to get to a point where parent deallocation code actually does get excercised.
</p>
TicketjdemeyerFri, 05 Oct 2012 13:48:22 GMT
https://trac.sagemath.org/ticket/11521#comment:179
https://trac.sagemath.org/ticket/11521#comment:179
<p>
Sorry to spoil the party, but with sage-5.4.rc0 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, I get
</p>
<pre class="wiki">sage -t "devel/sage/sage/libs/singular/ring.pyx"
**********************************************************************
File "/release/merger/sage-5.5.beta0/devel/sage/sage/libs/singular/ring.pyx", line 490:
sage: ring_ptr in ring_refcount_dict
Expected:
False
Got:
True
**********************************************************************
1 items had failures:
1 of 22 in __main__.example_8
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/jdemeyer/.sage//tmp/ring_18750.py
[3.4 s]
sage -t "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
Exception AttributeError: AttributeError('PolynomialRing_field_with_category' object has no attribute '_modulus',) in ignored
Exception AttributeError: AttributeError('PolynomialRing_field_with_category' object has no attribute '_modulus',) in ignored
**********************************************************************
File "/release/merger/sage-5.5.beta0/devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 423:
sage: len(ring_refcount_dict) == n
Expected:
True
Got:
False
**********************************************************************
1 items had failures:
1 of 19 in __main__.example_4
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/jdemeyer/.sage//tmp/multi_polynomial_libsingular_19160.py
[5.8 s]
</pre><p>
So perhaps this does require <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>?
</p>
TicketjdemeyerFri, 05 Oct 2012 14:02:49 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:180
https://trac.sagemath.org/ticket/11521#comment:180
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
TicketSimonKingFri, 05 Oct 2012 14:12:53 GMT
https://trac.sagemath.org/ticket/11521#comment:181
https://trac.sagemath.org/ticket/11521#comment:181
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:179" title="Comment 179">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Sorry to spoil the party, but with sage-5.4.rc0 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, I get
</p>
<pre class="wiki">sage -t "devel/sage/sage/libs/singular/ring.pyx"
**********************************************************************
File "/release/merger/sage-5.5.beta0/devel/sage/sage/libs/singular/ring.pyx", line 490:
sage: ring_ptr in ring_refcount_dict
Expected:
False
Got:
True
</pre></blockquote>
<p>
}}}
</p>
<p>
I guess this is expected, because of the newly introduced strong cache for polynomial rings at the latest version of <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
</p>
<blockquote class="citation">
<p>
<strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong>
File "/release/merger/sage-5.5.beta0/devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 423:
</strong></p>
<blockquote>
<p>
sage: len(ring_refcount_dict) == n
</p>
</blockquote>
<p>
Expected:
</p>
<blockquote>
<p>
True
</p>
</blockquote>
<p>
Got:
</p>
<blockquote>
<p>
False
</p>
</blockquote>
</blockquote>
<p>
Same as above: This test was supposed to show that garbage collection for polynomial rings happens, but it doesn't, with a strong cache.
</p>
<blockquote class="citation">
<p>
<strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong></strong><strong>
sage -t "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
Exception <a class="missing wiki">AttributeError?</a>: <a class="missing wiki">AttributeError?</a>('PolynomialRing_field_with_category' object has no attribute '_modulus',) in ignored
Exception <a class="missing wiki">AttributeError?</a>: <a class="missing wiki">AttributeError?</a>('PolynomialRing_field_with_category' object has no attribute '_modulus',) in ignored
</strong></p>
</blockquote>
<p>
I guess that is more serious. IIRC, it comes from code in polynomial_template.
</p>
<p>
Anyway. If that kind of problems persist, then I guess we need <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>.
</p>
TicketnbruinFri, 05 Oct 2012 22:39:49 GMTattachment set
https://trac.sagemath.org/ticket/11521
https://trac.sagemath.org/ticket/11521
<ul>
<li><strong>attachment</strong>
set to <em>trac_11521_doctestfix.patch</em>
</li>
</ul>
<p>
small fixes
</p>
TicketnbruinFri, 05 Oct 2012 22:47:51 GMTstatus, description changed
https://trac.sagemath.org/ticket/11521#comment:182
https://trac.sagemath.org/ticket/11521#comment:182
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=182">diff</a>)
</li>
</ul>
<p>
OK, I've adapted two doctests to reflect that polynomial rings still are not garbage collected. Furthermore, I backported two changes from <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>:
</p>
<ul><li>A change to a doctest for modular forms spaces, which depended on previously cached computation. Since these things do get collected now, the caching was lost. The rewrite forces recomputation, so the dependence on things not getting deleted is removed
</li></ul><ul><li>The ignored <code>AttributeError</code> (which doesn't cause a doctest failure, by the way. The warning is only visible because it happened near a doctest that failed for another reason). Since we found where this ignore was happening anyway <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/13447#comment:30"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/13447#comment:30</a> we might as well fix it now. It's a side-effect of things getting collected, but the change itself doesn't depend on the proper refcounting details that <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> itself is hanging on.
</li></ul><p>
All doctests pass for me with this, so back to positive review.
</p>
TicketjdemeyerSat, 06 Oct 2012 09:23:42 GMTstatus, description changed
https://trac.sagemath.org/ticket/11521#comment:183
https://trac.sagemath.org/ticket/11521#comment:183
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11521?action=diff&version=183">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:182" title="Comment 182">nbruin</a>:
</p>
<blockquote class="citation">
<p>
All doctests pass for me with this, so back to positive review.
</p>
</blockquote>
<p>
I think somebody needs to review that patch. Simon, can you do that?
</p>
TicketjdemeyerSat, 06 Oct 2012 09:23:53 GMTstatus changed
https://trac.sagemath.org/ticket/11521#comment:184
https://trac.sagemath.org/ticket/11521#comment:184
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketSimonKingSat, 06 Oct 2012 11:22:51 GMT
https://trac.sagemath.org/ticket/11521#comment:185
https://trac.sagemath.org/ticket/11521#comment:185
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:183" title="Comment 183">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11521#comment:182" title="Comment 182">nbruin</a>:
</p>
<blockquote class="citation">
<p>
All doctests pass for me with this, so back to positive review.
</p>
</blockquote>
<p>
I think somebody needs to review that patch. Simon, can you do that?
</p>
</blockquote>
<p>
<a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11521/trac_11521_doctestfix.patch" title="Attachment 'trac_11521_doctestfix.patch' in Ticket #11521">trac_11521_doctestfix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11521/trac_11521_doctestfix.patch" title="Download"></a> looks fine to me. I'll try to build the latest Sage prerelease on bsd.math with these changes and do tests.
</p>
TicketSimonKingSat, 06 Oct 2012 16:16:50 GMTstatus, reviewer, author changed
https://trac.sagemath.org/ticket/11521#comment:186
https://trac.sagemath.org/ticket/11521#comment:186
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Jean-Pierre Flori, Nils Bruin</em> to <em>Jean-Pierre Flori, Nils Bruin, Simon King</em>
</li>
<li><strong>author</strong>
changed from <em>Simon King</em> to <em>Simon King, Nils Bruin</em>
</li>
</ul>
<p>
It works! All tests pass on bsd.math, and I also did <code>sage -t devel/sage/sage/rings/polynomial/</code> separately, to verify that the ignored attribute error does not occur.
</p>
<p>
The new patch looks fine. Hence, it is a positive review.
</p>
TicketjdemeyerSat, 06 Oct 2012 20:44:18 GMT
https://trac.sagemath.org/ticket/11521#comment:187
https://trac.sagemath.org/ticket/11521#comment:187
<p>
I confirm that doctests pass.
</p>
TicketjdemeyerWed, 17 Oct 2012 20:58:51 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11521#comment:188
https://trac.sagemath.org/ticket/11521#comment:188
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.5.beta0</em>
</li>
</ul>
TicketjdemeyerSat, 03 Nov 2012 17:36:16 GMTstatus changed; resolution, merged deleted
https://trac.sagemath.org/ticket/11521#comment:189
https://trac.sagemath.org/ticket/11521#comment:189
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
<li><strong>merged</strong>
<em>sage-5.5.beta0</em> deleted
</li>
</ul>
<p>
Sorry to bring bad news, but a trial sage-5.5.beta1 caused a Segmentation Fault in <code>sage/schemes/elliptic_curves/ell_number_field.py</code> on OS X 10.4 PPC. Removing <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> made the problem go away. This really feels like a déjà vu, but I'm afraid I need to remove the patch from sage-5.5.beta0.
</p>
TicketjdemeyerSun, 04 Nov 2012 08:37:35 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11521#comment:190
https://trac.sagemath.org/ticket/11521#comment:190
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.5.beta0</em>
</li>
</ul>
Ticket