Proper deallocation of the (unique) pari instance

[simon-king-jena]

Currently, the unique instance of Pari is deallocated manually when Sage executes sage.all.quit_sage.

I think that's unsafe. In fact, it led to problems at #12215. My suggestion is to deallocate the unique Pari instance in the default Cython way: With a __dealloc__ method.

Hence, I am moving a part of the second patch of #12215 to here. I believe this is cleaner than packing a bunch of unrelated changes into one ticket.

CC: @jpflori @zimmermann6 @vbraun @robertwb @nbruin @malb @orlitzky

Component: memleak

Keywords: pari deallocation

Author: Simon King

Reviewer: Volker Braun

Merged: sage-5.5.rc1

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

[simon-king-jena]

comment:1

I don't know how to make a proper doctest. One could (just for testing) create a second pari instance - and the fact that there is no crash would then constitute the test...

What do you think?

[jpflori]

comment:2

Replying to @simon-king-jena:

I don't know how to make a proper doctest. One could (just for testing) create a second pari instance - and the fact that there is no crash would then constitute the test...

What do you think?

That might be a good idea.

I'll have a look at the libpari interface code and see if that looks feasible.

[jpflori]

comment:4

I don't think allocating a second instance is possible at all because of C stuff in PARI which would get overwritten (e.g. calling pari_init twice in a row would not be that nice).

We could hope to be able to properly shutdown the PARI instance and reinstantiating it, but that looks non-trivial.
E.g. some gen elements are defined in gen.pyx at the top level and point to the unique PARI.
So we should first list all of them, delete them properly, then reinstantiate PARI.
Fortunately for us, in a normal use, this is done automatically by Python upon exit (and thanks to your patch the unique PARI instance lives long enough).

[simon-king-jena]

comment:5

So, Jean-Pierre, you suggest that the fact that Sage does not crash when exiting is good enough for a test? Shall I add a docstring to the dealloc method saying exactly this?

[jpflori]

comment:6

I think so.

The only other "proper" solution I see is to gather all allocation made at the top level of gen.pyx into a global function or a member function of PariInstance, and call this function from the top level after the PariInstance is initialized.
(This part part might be optional, we could just list them for deallocation below).
Then define a counter part to this function which would deallocate these objects.
And for our test we would first call this last function, then del the pari unique instance, then reinit it (provided the PARI lib really supports that, but that seems plausible).
But that would only work if no things outside of gen.pyx directly use the unique pari instance (or at least no other things that would get initialized by the doctest framework), as far as I know and flint is concerned, there are several places in Sage code where the FLINT memory is accessed making an approach as suggested above impossible in the case of FLINT, maybe not in a doctesting context, but for general use of Sage.

All that said, deallocation of gen's does not involve the Pari instance they were created with, and the memory they use does not belong to PARI (i.e. PARI will not try to deallocate this memory when quitting, what FLINT would do following my above remarks), but the fact that at the python level they have a reference to this instance would make calling "del" on the unique Pari instance useless unless we proceed as above and first delete all these global gen's automatically created when importing gen.pyx.