PariInstance never deallocated ?

[Nathann Cohen]

Hello everybody,

It seems that the PariInstance object, created when Sage starts, is
never deallocated. In particular, none of the 'print "Heyyyy" ' I
added in sage.libs.pari.pari_instance.PariInstance.__dealloc__ were
ever printed.

Would anybody know why that happens ?

Nathann

[leif]

I think this is http://trac.sagemath.org/ticket/17686 (needs review).


-leif

[Nathann Cohen]

I think this is http://trac.sagemath.org/ticket/17686 (needs review).

Not sure that it is the same problem: when I apply that branch, the __dealloc__ function still is not called.

Nathann 

[Dima Pasechnik]

On Friday, 8 May 2015 14:18:22 UTC+1, Nathann Cohen wrote:

I think this is http://trac.sagemath.org/ticket/17686 (needs review).

Not sure that it is the same problem: when I apply that branch, the __dealloc__ function still is not called.

IMHO #1786 is about pexpect, whereas your problem is about a library interface to Pari.

Nathann 

[Nathann Cohen]

> IMHO #1786 is about pexpect, whereas your problem is about a library
> interface to Pari.

I am trying to figure out whether all objects with a reference to a
PariInstance are all deallocated, but I am no very sure I know how to
check that O_o

Nathann

[Dima Pasechnik]

isn't it about Python's GC? See e.g.  gc.get_referrers(*objs)

[Nathann Cohen]

> isn't it about Python's GC? See e.g. gc.get_referrers(*objs)

It tells you who points to the object indeed. It does not give you the
list of "those who will never be automatically deallocated". And
unfortunately there are many things in there :-/

Nathann

[Nathann Cohen]

Oh. Well that explains it: "module global variables are not cleaned
automatically" [1].

Copy/paste this code into some Sage module, and you will never see
"Really ?" printed:

---
cdef class A:
def __init__(self):
print "HeyA"
def __dealloc__(self):
print "Really?"

cdef A wtf = A()
---

Soooo well. I don't exactly know if we can safely remove
"pari_instance = PariInstance()" from the module and replace
everything by PariInstance(), since I do not know very well how
intensely that is being called O_o

Nathann

[1] https://groups.google.com/d/msg/cython-users/O3n8ib6DvT0/yPWCAD5mvmwJ

[leif]

You're right, the PariInstance is for the library interface, not for
calling 'gp' through pexpect.

But it indeed seems its __dealloc__() never gets called (in Sage
6.7.beta3 at least).


-leif

[Nils Bruin]

Isn't this object representing the fact that `pari.so` has been loaded as a dynamic library? Does that library ever get unloaded? I think it's just representing reality by never being deallocated. (calls into the pari library don't take a pointer to the "pari instance" they are communicating with, so necessarily there is only one, global, pari stack)

A similar thing happens with libs.ecl too. There can only be one instance of that too, so loading the module sage.libs.ecl also automatically initializes ecl. In fact, there the module sage.libs.ecl itself represents the global instance (the ecl global state is referenced via module-global variables).

[Nathann Cohen]

> Isn't this object representing the fact that `pari.so` has been loaded as a
> dynamic library? Does that library ever get unloaded? I think it's just
> representing reality by never being deallocated.

It is not deallocated when you quit Sage either. This is how I noticed it.

> A similar thing happens with libs.ecl too.

And Singular as well, apparently.

Nathann

[leif]

Nathann Cohen wrote:
> Oh. Well that explains it: "module global variables are not cleaned
> automatically" [1].
>
> Copy/paste this code into some Sage module, and you will never see
> "Really ?" printed:
>
> ---
> cdef class A:
> def __init__(self):
> print "HeyA"
> def __dealloc__(self):
> print "Really?"
>
> cdef A wtf = A()
> ---
>
> Soooo well. I don't exactly know if we can safely remove
> "pari_instance = PariInstance()" from the module and replace
> everything by PariInstance(), since I do not know very well how
> intensely that is being called O_o

What do you mean by "safely"?

There would be some performance penalty if I am not mistaken, but
otherwise doing so should be safe.

It's not clear to me why at all you want the PARI instance to get
deallocated (upon exit) though.

(Ok, its __dealloc__() is currently useless otherwise, and shared
libraries shouldn't use global variables anyway.)


-leif

[Nathann Cohen]

> What do you mean by "safely"?

I meant "without a noticeable performance penalty"

> It's not clear to me why at all you want the PARI instance to get
> deallocated (upon exit) though.

Nothing specific. I was using Valgrind for other things, and this
problem is ranked first among Sage's memory leaks.

Nathann

[leif]

Nathann Cohen wrote:
>> Isn't this object representing the fact that `pari.so` has been loaded as a
>> dynamic library? Does that library ever get unloaded? I think it's just
>> representing reality by never being deallocated.
>
> It is not deallocated when you quit Sage either. This is how I noticed it.

So we'd in principle have to revert the changes made by #13741.

Using 'del pari' in quit_sage() doesn't work, and one cannot call
pari.__dealloc__() explicitly, hence another function has to be added
back which does the clean-up (to be called from quit_sage()).


-leif

[leif]

leif wrote:

> Nathann Cohen wrote:
>> It is not deallocated when you quit Sage either. This is how I noticed it.
>
> So we'd in principle have to revert the changes made by #13741.
>
> Using 'del pari' in quit_sage() doesn't work, and one cannot call
> pari.__dealloc__() explicitly, hence another function has to be added
> back which does the clean-up (to be called from quit_sage()).

This works for me (quick'n'dirty):

diff --git a/src/sage/all.py b/src/sage/all.py
index d2494e0..0ee1d04 100644
--- a/src/sage/all.py
+++ b/src/sage/all.py
@@ -258,6 +258,10 @@ def quit_sage(verbose=True):
from sage.libs.all import symmetrica
symmetrica.end()

+ # del sage.libs.pari.pari_instance.pari won't work
+ pari._clean_up()
+
+
from sage.ext.interactive_constructors_c import inject_on, inject_off

sage.structure.sage_object.register_unpickle_override('sage.categories.category', 'Sets', Sets)
diff --git a/src/sage/libs/pari/pari_instance.pyx
b/src/sage/libs/pari/pari_instance.pyx
index 5f91c01..a2e59fe 100644
--- a/src/sage/libs/pari/pari_instance.pyx
+++ b/src/sage/libs/pari/pari_instance.pyx
@@ -495,6 +495,9 @@ cdef class PariInstance(PariInstance_auto):
indirect doctest. See the discussion at :trac:`13741`.

"""
+ self._clean_up()
+
+ def _clean_up(self):
sage_free(<void*>pari_mainstack.vbot)
pari_mainstack.rsize = 0
pari_mainstack.vsize = 0


Haven't tested with valgrind though.


-leif

[Nathann Cohen]

> This works for me (quick'n'dirty):
>

> Haven't tested with valgrind though.

Well, Valgrind seems happy! ;-)

Do you think that there is a sensible reason to not apply this change
(with some more documentation)? I do not mind doing it, unless you
prefer to do it yourself.

Nathann

[leif]

Go ahead! (I could probably review it...)


-leif

[Nathann Cohen]

Needs review !

http://trac.sagemath.org/ticket/18385

Nathann