Refcounting for Singular rings

[gagern]

Ref counting for Singular rings

Python makes no guarantees that destructors are called if circular references are involved. This patch implements an extra Python proxy layer that correctly refcounts Singular rings. In contrast to our earlier code, it will release the singular rings once they are no longer in use. This triggers various issues in Sage/libSingular that are dealt with in the follow-up ticket #10903

Historic discussion

As described for the Sage on Gentoo project, there is a problem in how parts of sage use __deallocate__ in their Cython code. I've actually traced an instance of groebner_strategy.pyx causing segmentation faults under Python 2.7.

What happens is that the garbage collector invokes the tp_clear function for the object. Its implementation is provided by Cython, and one of its effects is that every python reference will be set to None. A bit later on, tp_dealloc is called and invokes the __deallocate__ method. By that time, _parent is None, so accessing _parent._ring is a bad idea, and in this case it turns out to be null.

Others have had similar problems before. There are crude workarounds floating around. I guess a proper solution would be twaking cython to allow custom code in the tp_clear function. In other words, have a "magic" mehod __clear__ similar to the magic __deallocate__. But I'll wait for comments here first, before taking this to cython upstream. Perhaps people with more experience have better solutions to offer. And I won't mind if you decide to take this to Cython devs yourselves.

Apply

Apply:

• attachment: trac_11339_refcount_singular_rings.patch
• attachment: trac_11339_refcount_singular_polynomials.patch

Depends on #10903

Dependencies: #10903 (for doctests)

CC: @kiwifb @robertwb @burcin @malb @simon-king-jena @nexttime

Component: algebra

Keywords: sd31

Author: Volker Braun, Martin von Gagern

Reviewer: François Bissey, Steven Trogdon, Martin Albrecht

Merged: sage-4.7.2.alpha4

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

[vbraun]

comment:2

The documentation is fairly clear, you are only allowed to touch C data structures in __dealloc__: http://docs.cython.org/src/userguide/special_methods.html#finalization-method-dealloc

Really, this shows the difference between the C++ and the Python object model. In Python, executing constructors/destructors in derived classes is optional and leaves a lot of freedom to the programmer. Because of the garbage collection you can allocate resources wherever you want. By comparison, C++ is very strict and essentially forces you to use RAII. This is why there is no try/finally in C++. Which is why there is necessarily some tension in Cython between C++ and Python object instantiation/finalization.

Having said that, it seems like the cleanest solution would be if Cython would also call the Python destructor __del__ according to the normal Python rules (why __clear__?). Then the Python and C++ object model could just peacefully coexist. The lifetime of a cdef class would be

__cinit__()    # always called
__init__()     # not necessarily called for derived classes
... do something ...
__del__()      # not necessarily called for derived classes
__dealloc__()  # always called

[kiwifb]

comment:3

Thanks for your input Volker. I think we should definitely put some work in libs/singular to have some __del__ and __dealloc__ functions. I think the current state of things in there is a bit messy. I remember reading about __del__ in the python doc about garbage collection and wondering why singular_ring_delete in libs/singular/rings.pyx wasn't a __del__ method instead as its intent is clear.

[vbraun]

comment:4

Just to clarify, right now Cython extension classes do not call the Python destructor __del__ upon finalization, so Sage can't rely on this mechanism. I'm not quite sure if it is an intentional thing or if the Cython devs just haven't gotten around to implementing it. Maybe we can hack on that on Sage Days 31 since both Robert and Burcin will be around :-)

[kiwifb]

comment:5

OK, well something as to be done about this if we want to move to python-2.7. I think this problem is probably the biggest issue in the way of #9958 there are a few other things to look at but that's the only one producing a segfault.

[gagern]

comment:6

I notice that PyTypeObject has a member tp_del which is not included in the documentation. So perhaps it's as simple as associating a function with that slot. Otoh, perhaps there is a reason that member isn't documented. The single reply to Python issue 4934 doesn't rule out that possibility.

[gagern]

Workaround patch

[gagern]

Attachment: bug11339a.patch.gz

Workaround hg bundle

[kiwifb]

comment:8

Attachment: bug11339a.bundle.gz

For the record this works with sage-on-gentoo with python-2.7.1 so as far as I am concerned it works as advertised so far. I can do a review against a vanilla sage using python-2.6 later this week provided that I get the all clear to go back to work (we got a couple more earthquakes down here - the magnitude 6 one was scary but very few people got hurt).

What is the "workaround hg bundle" Martin? I can reformat things if needed.

[gagern]

comment:9

Replying to @kiwifb:

What is the "workaround hg bundle" Martin? I can reformat things if needed.

It's what I perceived to be the hg equivalent of a "bzr send": a file which describes a number of hg commits inclusing metadata, so that other hg users can pull from it.
See hg manual for unbundle and related documentation.

[kiwifb]

comment:10

Hi Martin, Steve sent me a note about it earlier this morning. The patch is the only thing necessary here, am I right?

[strogdon]

comment:11

Replying to @kiwifb:

For the record this works with sage-on-gentoo with python-2.7.1 so as far as I am concerned it works as advertised so far. I can do a review against a vanilla sage using python-2.6 later this week provided that I get the all clear to go back to work (we got a couple more earthquakes down here - the magnitude 6 one was scary but very few people got hurt). What is the "workaround hg bundle" Martin? I can reformat things if needed.

This patch works fine for me too on sage-on-gentoo with python-2.7.1. I have applied this patch to the vanilla sage-4.7.1.alpha2 (python-2.6.4.p10) spkg and all long tests pass. I have also applied the patch along with the patches and required spkgs in ticket #9958 to the sage-4.7.1.alpha2 (python-2.7.1) spkg and all the tests that failed without this patch because of segfaults, i.e.

sage -t  -force_lib "devel/sage/sage/schemes/generic/scheme.py"

sage -t  -force_lib "devel/sage/sage/rings/morphism.pyx"    

sage -t  -force_lib "devel/sage/sage/rings/homset.py"

now pass.

[kiwifb]

Reviewer: François Bissey, Steven Trogdon

[kiwifb]

comment:12

I am giving this a positive review on my and Steve's observations. I unfortunately won't be able to run some extra tests before Monday it seems.

[vbraun]

comment:13

As it is, the patch is spectacularly ugly. I agree that it works, but we should investigate alternatives. It is not a particularly pressing issue since Sage is not going to switch to Python-2.7 this week. I'll have a closer look at it with Burcin duing this week at SD31.

[kiwifb]

comment:14

I am ok with this. I don't plan to push python-2.7.1 in 4.7.1.

I would like most of the other pending bits and pieces that will make testing easier to be in 4.7.1 however. It would be nice if at least some testing could be done in 4.7.2 and really land it by the next release.

[gagern]

comment:15

Replying to @vbraun:

As it is, the patch is spectacularly ugly. [...] we should investigate alternatives.

I agree, that's why I titled the patch "workaround" instead of "solution". I can't think of a way to address this in sage alone, though, as in my opinion a proper solution would entail modifications to cython as discussed above.

[vbraun]

comment:16

In the case at hand we don't really need the Cython class GroebnerStrategy._parent, only the C struct GroebnerStrategy._parent._ring is used in __deallocate__(). I've made a minimal patch that adds a cdef ring *_parent_ring data member to GroebnerStrategy to keep track of the ring only. Because it refers to a C struct, it is safe to access it the Cython destructor. Because parents are immutable we don't have to worry about the ring * pointer becoming invalid.

The patch should fix the crash, but I don't have a Sage-on-Python-2.7 install to test it. Can somebody give it a try?

[vbraun]

Author: Volker Braun, Martin von Gagern

[jdemeyer]

Changed work issues from Conflict with #11856 to none

[simon-king-jena]

comment:82

For the record: The conflicts with #11856 and also another conflict with #11068 were caused by cosmetic changes introduced by the first patch. Both are resolved now.

[simon-king-jena]

comment:83

It is amazing how many patches of mine fail for trivial reasons because of this ticket. E.g., introspection: One of my examples for introspection of code fails, because __cinint__ was introduced. So, in addition to #11856 and #11068, I'll have to change at least a third patch as well.

[simon-king-jena]

comment:84

You should run the tests with your patches applied on top of sage-4.7.2.alpha3. At least with the alpha3-prerelease, I get some doctest errors, for example sage -t "devel/sage-main/sage/misc/sageinspect.py".

So, it seems it would have been better to change this patch, not #11856 and #11068. Too bad.

[vbraun]

comment:85

The doctest failure in sage/misc/sageinspect (would have been nice if it would have used its own module as a doctest example btw) is fixed in #10903. There are no doctest failures if you use sage-4.7.2.alpha3 + #11339 + #10903. Really the two tickets should be one but thats not how it panned out historically.

[nexttime]

comment:86

Replying to @vbraun:

The doctest failure in sage/misc/sageinspect (would have been nice if it would have used its own module as a doctest example btw) is fixed in #10903. There are no doctest failures if you use sage-4.7.2.alpha3 + #11339 + #10903. Really the two tickets should be one but thats not how it panned out historically.

So we have a cyclic dependency now...

Somehow matches the ticket's title.

[simon-king-jena]

comment:87

Question to the release manager: How are the odds that both #11339/#10903 and #11856 (which both fix critical bugs) will be merged into sage-4.7.2?

If #11339/#10903 are ready to be merged then I guess one can easily modify #11856 so that it matches (it is just a different line break in one hunk). And #11068 was shifted to sage-4.7.3 anyway, so, it can wait.

[simon-king-jena]

comment:88

Replying to @vbraun:

The doctest failure in sage/misc/sageinspect (would have been nice if it would have used its own module as a doctest example btw) ...

The point was to have a cdef class, but sageinspect is pure Python, so it wouldn't have been a good example.

[vbraun]

comment:89

Jeroen said that Singular will go into sage-4.7.2 on sage-release. This should be #11339 + #10903 + #11856, preferably in that order.

[jdemeyer]

comment:90

Replying to @vbraun:

Jeroen said that Singular will go into sage-4.7.2 on sage-release. This should be #11339 + #10903 + #11856, preferably in that order.

Exactly. Just make sure you clearly state the order in which patches have to be applied. The "cyclic" dependency is not really a problem as these tickets will be merged together anyway.

[nexttime]

comment:91

Replying to @jdemeyer:

The "cyclic" dependency is not really a problem as these tickets will be merged together anyway.

I'll implement merging strongly connected components... ;-)

[simon-king-jena]

comment:92

Great, #11339 together with #10903 really seem to fix it. So, let it be a positive review, then. I will rebase #11856 now, and #11068 can wait a little.

[simon-king-jena]

comment:93

For the record: I made #11856 depend on #10903. So, it should all be good now...

[jdemeyer]

Dependencies: #10903 (for doctests)

[jdemeyer]

Merged: sage-4.7.2.alpha4