Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#13741 closed defect (fixed)

Proper deallocation of the (unique) pari instance

Reported by: SimonKing Owned by: rlm
Priority: blocker Milestone: sage-5.5
Component: memleak Keywords: pari deallocation
Cc: jpflori, zimmerma, vbraun, robertwb, nbruin, malb, mjo Merged in: sage-5.5.rc1
Authors: Simon King Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

trac13741_pari_dealloc.patch (1.4 KB) - added by SimonKing 7 years ago.
Pari deallocation in a more Cythonic way

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 7 years ago by SimonKing

  • Status changed from new to needs_review

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?

comment:2 in reply to: ↑ 1 Changed 7 years ago by jpflori

Replying to SimonKing:

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.

comment:3 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.6 to sage-5.5
  • Priority changed from major to blocker

comment:4 Changed 7 years ago by jpflori

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).

comment:5 Changed 7 years ago by SimonKing

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?

comment:6 Changed 7 years ago by jpflori

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.

Changed 7 years ago by SimonKing

Pari deallocation in a more Cythonic way

comment:7 Changed 7 years ago by SimonKing

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

comment:8 Changed 7 years ago by jpflori

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?

comment:9 follow-up: Changed 7 years ago by 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?

comment:10 in reply to: ↑ 9 Changed 7 years ago by jpflori

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.

comment:11 Changed 7 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 5 years ago by leif

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.)

Note: See TracTickets for help on using tickets.