Opened 7 years ago

Closed 6 years ago

#3283 closed defect (fixed)

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

Reported by: malb Owned by: malb
Priority: major Milestone: sage-3.0.4
Component: commutative algebra Keywords: PolyBoRi, memleak, editor_malb
Cc: burcin, PolyBoRi Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (4)

pbori_memleak.patch (1.3 KB) - added by malb 7 years ago.
pbori_memleak_nex_attempt.patch (8.9 KB) - added by malb 7 years ago.
trac3283_bpi_object_reference.patch (2.4 KB) - added by burcin 7 years ago.
remove object references from BooleanPolynomialIterator?
pbori_iterators.patch (11.7 KB) - added by malb 6 years ago.
new version addresses burcin's review

Download all attachments as: .zip

Change History (23)

Changed 7 years ago by malb

comment:1 Changed 7 years ago by mabshoff

  • Milestone changed from sage-3.0.2 to sage-3.0.3

comment:2 Changed 7 years ago by 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

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by 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

comment:5 in reply to: ↑ 4 Changed 7 years ago by 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

comment:6 Changed 7 years ago by 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?

Changed 7 years ago by malb

comment:7 Changed 7 years ago by malb

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

Changed 7 years ago by burcin

remove object references from BooleanPolynomialIterator?

comment:8 Changed 7 years ago by 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. :)

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

comment:10 Changed 7 years ago by 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.

comment:11 in reply to: ↑ 9 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by 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

comment:13 Changed 7 years ago by malb

  • Summary changed from [with patch, needs review] fix some memholes in PolyBoRi interface to [with patch, needs work] fix some memholes in PolyBoRi interface

comment:14 Changed 7 years ago by craigcitro

  • Keywords editor_malb added

comment:15 Changed 6 years ago by malb

state of affairs for editoral meeting 26/06/08

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

comment:16 Changed 6 years ago by malb

  • Summary changed from [with patch, needs work] fix some memholes in PolyBoRi interface to [with patch, needs review] fix some memholes in PolyBoRi interface

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?

comment:17 Changed 6 years ago by burcin

  • Summary changed from [with patch, needs review] fix some memholes in PolyBoRi interface to [with patch, positive review pending changes] fix some memholes in PolyBoRi interface

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

Changed 6 years ago by malb

new version addresses burcin's review

comment:18 Changed 6 years ago by malb

  • Summary changed from [with patch, positive review pending changes] fix some memholes in PolyBoRi interface to [with patch, positive review] fix some memholes in PolyBoRi interface

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

comment:19 Changed 6 years ago by mabshoff

  • Milestone changed from sage-3.1.1 to sage-3.0.4
  • Resolution set to fixed
  • Status changed from new to closed

Merged pbori_iterators.patch in Sage 3.0.4.alpha2

Note: See TracTickets for help on using tickets.