Use weak references to cache homsets
jpflori opened this issue · 208 comments
Originally, this ticket was about the following memory leak when computing with elliptic curves:
sage: K = GF(1<<55,'t')
sage: a = K.random_element()
sage: while 1:
....: E = EllipticCurve(j=a); P = E.random_point(); 2*P;
This example is in fact solved by #715. However, while working on that ticket, another leak has been found, namely
sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: import gc
sage: gc.collect()
0
So, I suggest to start with #715 and solve the second memory leak on top of it. It seems that a strong cache for homsets is to blame. I suggest to use the weak TripleDict instead, which were introduced in #715.
To be merged with #715. Apply
- the patches from #715
- attachment: trac_11521_homset_weakcache_combined.patch
- attachment: trac_11521_callback.patch
- attachment: trac_11521_doctestfix.patch
Depends on #12969
Depends on #715
Dependencies: #12969; to be merged with #715
Component: coercion
Keywords: sd35
Author: Simon King, Nils Bruin
Reviewer: Jean-Pierre Flori, Nils Bruin, Simon King
Merged: sage-5.5.beta0
Issue created by migration from https://trac.sagemath.org/ticket/11521
After looking at #10548, I might have a better idea of the culprit:
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
This is definitely #715.
Resetting the coercion cache and calling gc.collect() deletes the cached elements.
I guess weak refs should be used in the different TripleDict objects of CoercionModel_cache_maps.
So this should be closed as duplicate/won't fix.
Jean-Pierre, why did you change the status to "needs review"? There is no patch to review.
Also, how to you reset the coercion cache? I would be interested if you have a workaround for the
memory leak in:
for p in prime_range(10^8):
k = GF(p)
Paul
As far as I'm concerned, this is nothing but a concrete example of the vague #715.
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.
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...
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.
I would be satisfied if I had a magic command to type to explicitly free the memory used by
every k=GF(p).
Paul
I'm having a look at your problem right now. Here are some hints to study the problem, mostly stolen from #10548.
I put it here for the record, and so that i can go faster next time.
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.
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]
Of course I guess you actually do something with your finite fields.
So here is a small example causing the fields to stay cached.
sage: import gc
sage: for p in prime_range(10**5):
....: k = GF(p)
....:
sage: del p, k
sage: gc.collect()
0
The zero here is bad and indeed
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
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')
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)])
And look at the png or use
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'...
So the culprit is "_cache" in sage.categories.homset which has a direct reference to the finite fields in its keys.
The clean solution woul be to used weakref in the keys (let's say something as WeakKeyDirectory in python).
However, resetting cache should be a (potentially partial) workaround (and could kill your Sage?).
sage: sage.categories.homset.cache = {}
sage: gc.collect()
376595
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".
I'm currently investigating that last case.
For info, using "k(47)*a+k(3)" is solved, so the problem left really comes from coercion and action of integers.
sage: cm = sage.structure. get_coercion_model()
sage: cm.reset_cache()
does not help.
First, the second example above is missing the line "k(1);" in the for loop, otherwise it does nothing more than the first example.
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.
You can not directly access them from Python level, but there a function _introspect_coerce() (defined in parent.pyx) which returns them.
In fact, everything is in _*_hash.
And to conclude, I'd say that right now you can not directly delete entries in this dictionaries from the Python level.
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...).
And I could also have missed other dictionaries used by Sage.
Jean-Pierre, I cannot reproduce that:
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
Paul
Did you "apt-get install python-objgraph" on your system?
If yes, is it a version for python 2.6 ?
The path I gave above might also be different on your system...
As the package manager.
Any progress on your side?
If you found any other dicitonaries leading to cahing problems, it would be great to mention them here for the record.
Hence the day someone will finally decide to tackle ticket #715, it will speed up the search of the culprit.
Replying to @jpflori:
Any progress on your side?
no time so far. I will look at this during the SageFlint days in December, unless someone else has
some time before.
Paul
Replying to @jpflori:
As far as I'm concerned, this is nothing but a concrete example of the vague #715.
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.
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'.
with Sage 4.7.2 I get the following:
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)
thus whenever we use the finite field we get a memleak.
(If I remove the a=K(0) line, I get only two elements in L, for p=2 and p=99991.)
Paul
I confirm (cf comment [comment:8]) that if I comment out the line
_cache[(X, Y, category)] = weakref.ref(H)
in categories/homset.py, then the memory leak from comment [comment:18] disappears.
Paul
I think we have a different problem here.
The finite fields themselves should be cached. The cache (see GF._cache) uses weak references, which should be fine.
Also, there are special methods zero_element and one_element which should do caching, because zero and one are frequently used and should not be created over and over again.
However, it seems that all elements of a finite field are cached - and that's bad!
sage: K = GF(5)
sage: K(2) is K(2)
True
sage: K.<a> = GF(17^2)
sage: K(5) is K(5)
True
I see absolutely no reason why all 17^2 elements should be cached.
Fortunately, we have no caching for larger finite fields:
sage: K.<a> = GF(17^10)
sage: K(5) is K(5)
False
In the following example, there is no memory leak that would be caused by the sage.categories.homset cache:
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
However, when you do a conversion K(...) then a convert map is created, and apparently is cached:
sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....:
sage: len(sage.categories.homset._cache.keys())
9692
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.
I am very stronglyagainst removing the cache of sage.categories.homset. Namely, elliptic curve code uses finite fields and maps involving finite fields a lot, and killing the cache is likely to cause a massive regression.
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.
By the way, I don't know why the status is "needs review". I think it clearly is "needs work".
Replying to @jpflori:
So the culprit is "_cache" in sage.categories.homset which has a direct reference to the finite fields in its keys.
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?
it seems the complete caching of field elements only occurs for p < 500:
sage: K=GF(499)
sage: K(5) is K(5)
True
sage: K=GF(503)
sage: K(5) is K(5)
False
A workaround to this memory leak is to free the cache from time to time (thanks Simon):
sage: sage.categories.homset._cache.clear()
Paul
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.
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.
Hence: Why don't we use a dictionary of dictionaries of dictionaries?
What I mean is:
- The keys of sage.categories.homset._cache should be weak references to the domain
- The values of sage.categories.homset._cache should be dictionaries whose keys are weak references to the codomain.
- The keys of these dictionaries should be weak references to the category, and the value a weak reference to the homset.
Hence, if there is no strong reference to either domain or codomain or category, then the homset can be deallocated.
The idea sketched in the previous comment seems to work!!!
With it, after running
sage: for p in prime_range(10^5):
....: K = GF(p)
....: a = K(0)
....: print get_memory_usage()
ends with printing 825.45703125
Without it, it ends with printing 902.8125
I don't know if we should ban caching of field elements?
How could fixing that memory leak be demonstrated by a doc test?
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 sage -tp 2 devel/sage.
Patch's up!
Hm. There seems to be a problem.
sage -t devel/sage/doc/en/constructions/linear_codes.rst
The doctested process was killed by signal 11
What is signal 11?
signal 11 is Segmentation Fault
Paul
Indeed it is a segfault. And it is triggered by
sage: w = vector([1,1,-4])
Monique van Beek just pointed me to the fact that the error occurs even earlier:
sage: is_Ring(None)
<BOOOOOM>
Moreover,
sage: None in Rings()
<BOOOOOOM>
That said: I am not working on top of vanilla sage, but I use some patches. In particular, I use #11900, which introduces so called Category_singleton, which has a specialised and very fast containment test.
I would not like to change #11900, but prefer to change my patch from here so that it works on top of #11900.
It turns out that indeed the bug is in #11900. So, I have to fix it there.
Cc to Nicolas, since it concerns categories:
Do we want that Hom(1,1) is still supported?
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.
Sorry, I forgot to update the Cc field.
Nicolas, please read my previous comment.
With my patch, applied on top of #11900, I get
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
At least some of the errors are like
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
and I really don't see why this should be considered a bug.
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.
I fixed this problem in the new patch version.
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.
I was told by Mike Hansen why weak references to integers and rationals do not work.
I see three options:
#. Drop the support for Hom(1,1) (which I'd prefer)
#. Add a cdef'd attribute __weakref__ to sage.structure.element.Element, 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.
FWIW:
With the latest patch, the tests in polynomial_singular_interface and in multi_polynomial_ring pass.
There remain the following problems:
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]
and
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]
and
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]
So, essentially this is just a single test that comes in two versions and is repeated three times - and I would actually say that not raising an error was a bug.
It seems that Hom(1/2,2/3) and similar nonsense is not used in Sage. Hence, I think these tests should be removed. I'll ask sage-devel.
Without the patch:
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
With the patch:
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
So, the memory does mildly increase, but it seems that most of the leak is fixed.
I think that a test of the kind
sage: get_memory_usage() - -m0 < 10
True
might be used as a doc test.
Replying to @simon-king-jena:
Cc to Nicolas, since it concerns categories:
Do we want that
Hom(1,1)is still supported?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.
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 CategoryObject). So +1 on my side to kill this dubious feature. You might want to double check on sage-algebra just to make sure.
Simon, you can also use the test suggested by Jean-Pierre Flori (see comment [comment:18] for an
example).
Paul
Hi Paul,
Replying to @zimmermann6:
Simon, you can also use the test suggested by Jean-Pierre Flori (see comment [comment:18] for an
example).
Yes, that looks good. With my patch, the test would be like
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)
The people at sage-devel somehow seem to agree that objects of a category should be instances of CategoryObject (which elements aren't!), and that we should thus drop the Hom(2/3,8/1) test.
In addition to that, I suggest to provide a better error message, something like
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.
Cheers,
Simon
Changed keywords from none to sd35
I am very much afraid that I have not been able to make my patch independent of #11900. This is not just because of the documentation, but also because of some details in the choice of the homset's category.
Anyway, it needs review (and so does the most recent version of #11900, by the way)!
Dependencies: #11900
I did try the new doctests in sage/categories/homset.py. However, with other patches applied, the number returned by gc.collect() changes.
So, for stability, I suggest to simplify the test, so that only the number of finite fields remaining in the cache is tested.
I updated the patch.
Difference to the previous patch: The number of objects collect by gc is marked as random (indeed, it will change with #11115 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.
Author: Simon King
I think I need help with debugging.
When I have sage-4.8.alpha3 with #9138, #11900, #715 and #11115, then all doctests pass.
When I also have #11521, 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
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]
So, could it be that not one of the tests was killed, but the test process itself?
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.
Can you give me any pointer on how those things could possibly be debugged?
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.
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 CategoryObjects.
Work Issues: Understand why a weak key dictionary is not enough
Attachment: trac11521_new_homset_cache.patch.gz
I have attached a new patch version. It fixes the segfault I mentioned. However, it also does not fix the memory leak.
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).
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.
Dear Simon,
Thanks a lot for taking care of all of this !
I'm just back from vacation and will have a look at all your patches in the following days.
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)...
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 [ticket:715] and all the work could be done there.
Of course youre the one currently doing all the wok, so do as you want :)
Cheers,
JP
Hi Jean-Pierre,
Replying to @jpflori:
I must point out that even if the memory leak was small,
It isn't small.
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 [ticket:715] and all the work could be done there.
I am not sure whether it would be good to do everything on one ticket, as the topics are related, but clearly disting: #715 is about weak "TripleDict" for coercion, #12215 is about a weak version of cached_function, and the ticket here is about the cache of homsets.
On the other hand: I am about to post a new patch here, with #715 as a dependency. It will use the new version of TripleDict from #715. So, one could argue that there is a common tool for both tickets, and they belong together.
Anyway. The new patch will fix the leak, but it will not suffer from the segfaults.
Cheers,
Simon
I have attached another patch under a new name, using a new approach: The weak TripleDict, that I introduce at #715, is an appropriate tool for the cache of homsets. The key is the triple (domain, codomain, category), and the value is a weak reference to the corresponding homset.
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.
Hence: Needs review!
Apply trac11521_triple_homset.patch
Changed work issues from Understand why a weak key dictionary is not enough to none
Description changed:
---
+++
@@ -12,3 +12,7 @@
- one does not change the curve in the loop
- does P+P instead of a multiplication
+
+**Apply**
+
+[attachment: trac11521_triple_homset.patch](https://github.com/sagemath/sage-prod/files/10653173/trac11521_triple_homset.patch.gz)In fact all tests pass for me, with #9138, #11900, #11115, #715 and attachment: trac11521_triple_homset.patch applied on top of sage-4.8.alpha3.
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.
It depends on what you call "the" leak.
At least, the patch fixes one 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:
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()
The memory usage does climb, but it climbs a lot slower than, for example, in sage-4.6.2.
PS: I just tested that even #12215 (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.
After running the example for a couple of minutes, interrupting and doing garbage collection, I find:
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
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.
Using objgraph, I found that the remaining leak seem to be related with sage.schemes.generic.homset.SchemeHomsetModule_abelian_variety_coordinates_field_with_category. Since this is another homset, it would make sense to try and fix it here.
Aha!!
I found that we have many equal instances of these scheme homsets:
sage: LE = [x for x in gc.get_objects() if isinstance(x,SchemeHomsetModule_abelian_variety_coordinates_field)]
sage: LE[100] == LE[150]
True
So, I guess we should fix the memory leak by using UniqueRepresentation or UniqueFactory!
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.
OK. Then why not reduce the non-uniqueness? EllipticCurve 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 not 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 #12215, it could then actually be turned into a weak cached function.
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 (#12215 is not needed for that).
I am preparing a new patch (it requires do tests and stuff), so, it is "needs work" for now.
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.
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.
I am undecided what we should do:
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?).
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 here.
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 #12215.
What do you think?
I guess I'll also ask on sage-nt.
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.
Anyway the problem of uniqueness of elliptic curve is highly non trivial and deserves its own ticket.
I guess #11474 would be a right place to treat that problem.
An alternative would be to make this ticket depend on #11474.
Hi Jean-Pierre,
indeed, you found the correct ticket.
And there is no need to ask on sage-nt, since I already did before opening #11474: Sage-nt 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.
Considering the discussion on sage-nt, #11474 probably is "wontfix".
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.
First, E is created, and some coercion into it is created. The coercion is cached. By #715, 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 __del__ method that prevents the cycle from being garbage collected.
Objgraph only shows a reference left as _codomain in a dict in a SchemeHomsetModule_a_v_c_f (defined in sage.schemes.generic.homset).
The homset of points of the ab. group of the curve is itself reference by an IntegerMulAction, 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.
The IntegerMulAction should be fine, as it is stored in a TripleDict (hence, weakly, by #715). Where does the dict occur?
I am a bit puzzled.
I see that there are a couple of cycles, involving an elliptic curve, a SchemeHomsetModule..., an IntegerMulAction, and an EllipticPoint_finit_field. However, none of them has a __del__ method, thus, the garbage collection should be able to collect the cycles.
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?
I got it!!
By #715, both the keys and the value of sage.structure.coerce_dict.TripleDict 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.
Actions are stored in such TripleDicts. However, an IntegerMulAction 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).
Solution: Make IntegerMulAction weakly referenceable! That should suffice.
It does not suffice. But I think it is a step to the right direction.
Apparently the weak reference to the value is not used in the TripleDict! So, I have to look at #715, perhaps I forgot to implement something there.
My point was that it seemed to me that there was some (not weak!) reference in the dictionary ZZ._action_hash pointing to the IntegerMulAction itself pointing to the curve and I thought it could prevent garbage collection.
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...
Anyway, I'll have a look at it tomorrow :)
By the way, have you any idea if dictionaries declared at the C level such as ZZ._action_hash are detected by objgraph ?
Hi Jean-Pierre,
The thing with the _action_hash is a good finding. I thought it would be a TripleDict, but it is just a usual dict. And this could indeed be a problem. I don't know if this is visible to objgraph.
But also I think that in addition we have the problem of strong references to the values of TripleDict. In principle, one could use weak references for not only the keys but also to the values -- perhaps this could be done in #715.
Or one could leave TripleDict as it is currently in #715, 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.
There might be a speed problem, though.
For info, resetting ZZ._action_hash to {} is not sufficient to let the IntegerMulAction be garbage collected.
Objgraph shows 3 objects pointing to the abelian group of points of the curve (itself pointing to the curve):
- the IntegerMulAction
- the point at infinity (?)
- a dict of size 11 which is in fact an attribute (_Scheme__ring_point_homset) of the curve (as a scheme) itself.
I'd be happy to test a weakref for values version of your patch regardless of the spedd impact.
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.
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
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):
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
Hi Jean-Pierre,
Replying to @jpflori:
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.
But it is not so easy because...
Only one copy gets cached in _action_hash because the curves are equal (==) but not identical (is).
... elliptic curves are not unique parents.
I think it would be a mistake to use a weak reference to the value of TripleDict. 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.
sage: len(L) # mmm got one left!!! not sure where it comes from yet...
Don't forget the last copy of E that was defined in the loop!
Since I think that a weak reference to the values of TripleDict 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.
Replying to @simon-king-jena:
Hi Jean-Pierre, Replying to @jpflori:
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.
But it is not so easy because...
Only one copy gets cached in _action_hash because the curves are equal (==) but not identical (is).
... elliptic curves are not unique parents.
Yep :)
I think it would be a mistake to use a weak reference to the value of
TripleDict. 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.
I see...
sage: len(L) # mmm got one left!!! not sure where it comes from yet...
Don't forget the last copy of E that was defined in the loop!
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)!
Since I think that a weak reference to the values of
TripleDictwon'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.
Mmm, have to think more about all of that...
I think I should describe the problem in more detail.
Currently,
- There is a strong reference from
__main__to the action and coercion caches that are stored in the coercion model. That's fine. - Parents store actions as well. Some of these parents (the integer ring, for example) will always stay in memory.
- 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.
- The action and coercion caches have a strong reference to the values.
- There is no direct reference from domain and codomain to the homset.
Hence, if an action is stored in the action cache, then there will always be a chain of strong references from __main__ via the cache to the action, and further to domain and codomain, so that it can not be collected.
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.
How could that be solved?
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?
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...
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 IntegerMulAction which itself has a strong ref to the curve which would prevent the curve to get collected?
This is basically what you concluded in Comment 74, 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 IntegerMulAction weak referrable is not enough, could you post a patch applying these ideas so that I can play with it?
Experimental patch using weak references for actions
Attachment: weak_ref_to_functor.patch.gz
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.
I did not run any doctests on it, yet. But I tested that only one SchemeHomsetModule... survives the garbage collection (and I did not delete E).
I confirm the leak is fixed with your last patch, just launched a ptestlong.
My first impression from some tests is that the additional patch causes a massive slow-down.
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.
Replying to @jpflori:
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...
That is not half as bad as I thought! However, it is far from good.
Do you also have the time with only the first patch, resp. with #715 only? After all, #715 uses weak references and may thus be responsible for some slow-down.