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.

[simon-king-jena]

Attachment: trac13741_pari_dealloc.patch.gz

Pari deallocation in a more Cythonic way

[simon-king-jena]

comment:7

I did not add a test, for the reasons we discussed, but I added a comment in the docstring of __dealloc__. Needs review.

[jpflori]

comment:8

Could you just remove the ending newline and use a ..NOTE:: contruction?

I'm not really sure the shared C-data is really what should be said.
I'd rather say something like
"Crafting a direct doctest for this method would entail properly deallocating all Sage objects indirectly pointing to the PARI library and all C data instantiated at PARI library initialization to be sure that a new initialization of the PARI library does not create conflicts.
The first step is exactly what Python garbage collector will do when exiting Sage and can be highly non-trivial.
The second one is exactly what the __dealloc__ method should do.
If one of these steps is not performed carefully, then it can lead to crashes when Sage exits.
Therefore, a direct doctest does not provide more evidence on the fact that the unique PARI instance is properly deallocated, than the fact that Sage does not crash when exiting and we rely on this indirect doctest."

What do you think?

[vbraun]

comment:9

Is this ticket actually up for review? Are we just bikeshedding the hypothetical layout of the docstring if underscore methods would actually appear in the reference manual?

[jpflori]

comment:10

Replying to @vbraun:

Is this ticket actually up for review? Are we just bikeshedding the hypothetical layout of the docstring if underscore methods would actually appear in the reference manual?

Exactly... although this docstring will be useful for future development and let people know what really happens under the rug.

But feel free to pick either Simon docstring or mine, or mix'em up as you prefer and we can get this in.
Now Jeroen seems to be back this issue looks indeed less important than releasing Sage 5.5.

[vbraun]

Reviewer: Volker Braun

[jdemeyer]

Merged: sage-5.5.rc1

[nexttime]

comment:13

Follow-up: #18385

(Since the PARI instance is a [module-]global variable, its __dealloc__() never gets called. This doesn't really hurt, except that Valgrind complains about even more memory leaks.)