[with patch, positive review] fix some memholes in PolyBoRi interface

[malb]

All PolyBoRi? iterators only destruct the iterator objects but never the object they act on. However, since we assign objects and not pointers/references a copy is triggered during creation and thus the destructor of the object ought to be called on self destruction of the iterator. So far the theory. In practice, while I didn't see any problems, I'd appreciate if somebody with more intime knowledge of the interface would take a careful look.

[PolyBoRi]

Ok, if one cannot force to use references here (I'm sorry, I've no experience, what can be done within pyx-files.), one has to clean up after oneself. The internal PolyBoRi? data structures use reference counting, so the missing destructor resulted in the fact, that the reference count was never decreased -> memleak. (On C++ all of this is done automatically.) So from my point of view this patch is ok.

But we should wait for Burcin's opinion, as he knows best.

Also, one should check somewhen, whether the object is still necessary in the interface for recent PolyBoRi?.

Best regards,

Alexander

[burcin]

The iterator wrappers keep a reference to the objects only to be able to check if the iterator reaches the end. As in the following (from BooleanMonomialVariableIterator.__next__):

if self.iter.equal(self.obj.variableEnd()):
    raise StopIteration

The objects that iterators act on are wrapped by python objects, e.g. there is a corresponding BooleanMonomial object associated to the PBMonom the iterator is acting on. Python should keep track of the memory of BooleanMonomial, so python will deallocate that PBMonom by calling the __dealloc__ method of BooleanMonomial. Calling the destructor of PBMonom from any other place would lead to segfaults.

Actually, it might be a good idea to add a reference to the BooleanMonomial object in the iterator as well. We wouldn't want python to garbage collect that while the iterator is still around.

Another problem you might want to ponder: We don't explicitly call the constructor of any iterator other than that of BooleSet, yet we still call their destructors. If we need to call the constructor, how do we get away without it most of the time? If we don't, why is BooleSet special?

Comments?

[PolyBoRi]

The iterator wrappers keep a reference to the objects only to be able to check if the iterator reaches the end. As in the following (from BooleanMonomialVariableIterator.__next__):

if self.iter.equal(self.obj.variableEnd()):
    raise StopIteration

If that's the only reason for the object, we could add some kind of isEnd() and skip the reference. If fact, the PolyBoRi?-Iterators have the member function isZero(), which is equivalent to this.

The objects that iterators act on are wrapped by python objects, e.g. there is a corresponding BooleanMonomial object associated to the PBMonom the iterator is acting on. Python should keep track of the memory of BooleanMonomial, so python will deallocate that PBMonom by calling the __dealloc__ method of BooleanMonomial. Calling the destructor of PBMonom from any other place would lead to segfaults.

Do I understand you right: the patch is not correct, because the destructor of the PolyBoRi? monomial must not be called at that place? But, since there is a memleak, the destructor not called correctly. So why, does the destruction of the

Actually, it might be a good idea to add a reference to the BooleanMonomial object in the iterator as well. We wouldn't want python to garbage collect that while the iterator is still around.

The iterators (upstream at C++) carry all references, which are need by them. For iterators in lexicographical rings, this is very much like - for instance - std::vector-iterators, which do not have knowledge of the vector they belong to. (The degree-lexicographical have internal references to the polynomial structure.) But using "isZero()" avoid the problem anyway.

Another problem you might want to ponder: We don't explicitly call the constructor of any iterator other than that of BooleSet, yet we still call their destructors. If we need to call the constructor, how do we get away without it most of the time? If we don't, why is BooleSet special?

Good question, the iterators, which obey the ring ordering, like the iterators of polynomials, are wrapped by a shread-pointer construction, which is not the case for BooleSet? iterators. Does that make any difference? What actually happens, if you use code like the one from new_BPI_from_PBPolyIter for new_BSI_from_PBSetIter (including the function arguments)?

I still have few experience in pyx code, but any kind of differences between the iterator types should not make any problems, because in both cases constructors, copy-constructors and destructors care for the memory management on c++-side.

Best regards,

Alexander

[PolyBoRi]

Sorry, to late:

So why, does the destruction of the

-> So why, is the destruction of the not called by the object itself?

shread-pointer

-> shared pointer Best regards,

Alexander

[malb]

So the strategy is either of the following?

• get rid of the reference to the C++ object alltogether since the iterators have a method to check for the end anyway
• add a reference to the Python Object rather than the C++ object and let Python do the refcounting?
• apply this patch since either assignment causes a copy and we need to destruct or it adds a reference and increases the refcount, right?

[malb]

The attached patch removes all .obj members from the iterators and replaces them with ._end members. Happy reviewing.

[burcin]

remove object references from BooleanPolynomialIterator?

[burcin]

attachment:trac3283_bpi_object_reference.patch removes the object reference from BooleanPolynomialIterator as well. It should be applied after attachment:pbori_memleak_nex_attempt.patch .

I give a positive review to Martin's patch, now someone needs to review mine. :)

[PolyBoRi]

Hm, I wouldn't have added this _end member, because one could check for equality using that iZero() member from the original PolyBoRi?-Iterator. If access to this member function is too complicated, the results of end() could be generated on the fly, as they are the same as result from the default constructors. (But indeed, this could change somewhen...)

Off-Trac Michael B. mentioned, that removing the object completely might cause problems. The object may be deleted meanwhile, and hence, the (C++-) iterator may become invalid.

Best regards,

Alexander

[malb]

So we remove the _end members and add a Python level reference to the original object (polynomial, monomial, set). This should prevent the GC from killing the original object.

[burcin]

Replying to PolyBoRi:

Hm, I wouldn't have added this _end member, because one could check for equality using that iZero() member from the original PolyBoRi?-Iterator. If access to this member function is too complicated, the results of end() could be generated on the fly, as they are the same as result from the default constructors. (But indeed, this could change somewhen...)

Which iterators have an isZero() function? I tried using that for BooleanMonomialIterator and BooleanMonomialVariableIterator while I was reviewing Martin's patch. (The PolyBoRi equivalent of) BooleanMonomialIterator had an isEmpty() method, because of it's base class CCuddNavigator. However, the variable iterator didn't. I didn't test to see if BooleanMonomialIterator worked as intended with the isEmpty() method.

[PolyBoRi]

You are right, my postings were isleading. In my last comment, I wanted to say that BoolePolynomial::const_iterator (BooleanPolynomialIterator) has the isZero() functionality, which could already be used in attachment:trac3283_bpi_object_reference.patch . (For BooleanMonomialIterator isValid() does the job, but BooleanMonomialVariableIterator, does not own it.)

In comment 3, I had meant, that I could add such some kind of isEnd() function upstream, for instance using isZero() for BoolePolynomial::const_iterator.

But now - after all these confusions - I must admit, that I first have to add this functionality to have a consistent interface for this test on all PolyBoRi? iterators.

I apologize for the trouble caused,

Alexander

[malb]

state of affairs for editoral meeting 26/06/08

I need to fix the code, possibly by next week.

[malb]

Next attempt attached:

• Python references to parent object
• some docstrings added
• I checked for memleaks with this patch and didn't find any in my short tests

Burcin, can you review my patch?

[burcin]

Two small problems:

• adding equal to ctypedef struct PBPolyVectorIter seems to be redundant.
• BooleanPolynomialVectorIterator does not deallocate self._end

Other than these a very positive review. Thanks for sorting out the iterators in the polybori wrapper malb. :)

[malb]

new version addresses burcin's review

[malb]

Since the pending changes are in the updated patch, I give the patch a positive review.

[mabshoff]

Merged pbori_iterators.patch in Sage 3.0.4.alpha2