[IMP-dev] Actual patches

First, the patch fixes a bug in remove_always and removes the workaround in Particle.

The second fixes a bug in reference counting and removes the workaround in the test code. In addition, for some reason all the reference counting tests were disabled so that fact that they all were failing didn't come up :-) Anyway, now things work and are tested.

The containers now should deal with exceptions better (for example adding a non-reference counted Object to two containers and catching the resulting exception doesn't result in memory corruption any more). Not that the rest of the IMP code is all exception safe, but these changes should handle most errors.

Index: kernel/include/IMP/internal/AttributeTable.h =================================================================== --- kernel/include/IMP/internal/AttributeTable.h (revision 584) +++ kernel/include/IMP/internal/AttributeTable.h (working copy) @@ -199,7 +199,9 @@ }

void remove_always(Key k) { - map_[k.get_index()]= Traits::get_invalid(); + if (k.get_index() < size_) { + map_[k.get_index()]= Traits::get_invalid(); + } }

Index: kernel/include/IMP/Particle.h =================================================================== --- kernel/include/IMP/Particle.h (revision 584) +++ kernel/include/IMP/Particle.h (working copy) @@ -265,6 +265,9 @@ \return true it the particle is active. */ bool get_is_active() const { + IMP_IF_CHECK(EXPENSIVE) { + assert_is_valid(); + } return is_active_; }

@@ -543,9 +546,7 @@ { floats_.remove(name); derivatives_.remove(name); - if (optimizeds_.contains(name)) { - optimizeds_.remove_always(name); - } + optimizeds_.remove_always(name); }

Index: kernel/include/IMP/internal/ObjectContainer.h =================================================================== --- kernel/include/IMP/internal/ObjectContainer.h (revision 584) +++ kernel/include/IMP/internal/ObjectContainer.h (working copy) @@ -9,6 +9,7 @@ #define __IMP_OBJECT_CONTAINER_H

#include "Object.h" +#include "RefCountedObject.h"

#include <boost/iterator/filter_iterator.hpp>

@@ -31,8 +32,9 @@ couldn't get the casts to work out right. */ template <class O, class I> -class ObjectContainer: public std::vector<O*> +class ObjectContainer { + std::vector<O*> data_; std::vector<int> free_; struct OK { bool operator()(const O*a) const { @@ -44,108 +46,111 @@ unsigned int get_index(II i) const {return i.get_index();} unsigned int get_index(unsigned int i) const {return i;}

- void ref(O*o) { - if (o) o->ref(); - } - - void unref(O* o) { - if (o) { - o->unref(); - if (!o->get_has_ref()) { - delete o; - } - } - } // hide it - void erase(){} + typedef std::vector<O*> Vector; public: - typedef std::vector<O*> Vector; - using Vector::size; - using Vector::empty; + ObjectContainer(){} ~ObjectContainer() { clear(); }

+ bool empty() const { + return data_.empty() || data_.size() == free_.size(); + } + unsigned int size() const { + return data_.size() - free_.size(); + } + void clear() { - for (typename Vector::iterator it= Vector::begin(); - it != Vector::end(); ++it) { - unref(*it); + for (typename Vector::iterator it= data_.begin(); + it != data_.end(); ++it) { + O* t= *it; + *it=NULL; + if (t) disown(t); } free_.clear(); - Vector::clear(); + data_.clear(); }

typedef boost::filter_iterator<OK, typename Vector::iterator> iterator; - iterator begin() {return iterator(Vector::begin(), Vector::end());} - iterator end() {return iterator(Vector::end(), Vector::end());} + iterator begin() {return iterator(data_.begin(), data_.end());} + iterator end() {return iterator(data_.end(), data_.end());}

typedef boost::filter_iterator<OK, typename Vector::const_iterator> const_iterator; const_iterator begin() const { - return const_iterator(Vector::begin(), Vector::end()); + return const_iterator(data_.begin(), data_.end()); } const_iterator end() const { - return const_iterator(Vector::end(), Vector::end()); + return const_iterator(data_.end(), data_.end()); }

void remove(I i) { unsigned int id= get_index(i); - IMP_assert(Vector::operator[](id) != NULL, "Nothing there to remove"); - unref(Vector::operator[](id)); - Vector::operator[](id)=NULL; + IMP_assert(id < data_.size(), + "Trying to remove invalid element in container"); + IMP_assert(data_[id] != NULL, "Nothing there to remove"); + O* t= data_[id]; free_.push_back(id); + data_[id]=NULL; + disown(t); }

O* operator[](I i) const { - IMP_check(get_index(i) < Vector::size(), + IMP_check(get_index(i) < data_.size(), "Index " << i << " out of range", IndexException("Out of range")); - IMP_assert(Vector::operator[](get_index(i)) != NULL, + IMP_assert(data_.operator[](get_index(i)) != NULL, "Attempting to access invalid slot in container"); - return Vector::operator[](get_index(i)); + return data_[get_index(i)]; } + I push_back(O* d) { IMP_CHECK_OBJECT(d); - ref(d); + own(d); IMP_IF_CHECK(EXPENSIVE) { - for (typename Vector::const_iterator it= Vector::begin(); - it != Vector::end(); ++it) { + for (typename Vector::const_iterator it= data_.begin(); + it != data_.end(); ++it) { IMP_assert(*it != d, "IMP Containers can only have one copy of " << " each object"); } } if (free_.empty()) { - Vector::push_back(d); - unsigned int idx= Vector::size()-1; + data_.push_back(d); + unsigned int idx= data_.size()-1; return I(idx); } else { unsigned int i= free_.back(); free_.pop_back(); - Vector::operator[](i)= d; + data_[i]= d; return I(i); } } + template <class It> void insert(iterator c, It b, It e) { + IMP_assert(c== end(), "Insert position is ignored in ObjectContainer"); IMP_IF_CHECK(EXPENSIVE) { for (It cc= b; cc != e; ++cc) { IMP_CHECK_OBJECT(*cc); - for (typename Vector::const_iterator it= Vector::begin(); - it != Vector::end(); ++it) { + for (typename Vector::const_iterator it= data_.begin(); + it != data_.end(); ++it) { IMP_assert(*it != *cc, "IMP Containers can only have one copy of " << " each object"); } } } for (It cc= b; cc != e; ++cc) { - ref(*cc); + own(*cc); } while (!free_.empty()) { - push_back(*b); + int i= free_.back(); + free_.pop_back(); + data_[i]= *b; ++b; } - Vector::insert(c.base(), b, e); + data_.insert(data_.end(), b, e); }

}; Index: kernel/include/IMP/internal/Object.h =================================================================== --- kernel/include/IMP/internal/Object.h (revision 584) +++ kernel/include/IMP/internal/Object.h (working copy) @@ -9,7 +9,6 @@ #ifndef __IMP_OBJECT_H #define __IMP_OBJECT_H

-#include "../log.h" #include "../exception.h"

namespace IMP @@ -42,24 +41,27 @@ void assert_is_valid() const;

bool get_has_ref() const {return count_ != 0;} + void ref() { assert_is_valid(); - IMP_assert(count_== 0, - "Non-reference counted objects can only belong to " \ - "one container."); ++count_; } + void unref() { assert_is_valid(); - IMP_assert(count_ ==1, "Too many unrefs on object"); + IMP_assert(count_ !=0, "Too many unrefs on object"); --count_; } - protected: - int count_; -private: + + unsigned int get_ref_count() const { + return count_; + } + + private: Object(const Object &o){} const internal::Object& operator=(const Object &o) {return *this;}

+ int count_; double check_value_; };

Index: kernel/include/IMP/internal/ObjectPointer.h =================================================================== --- kernel/include/IMP/internal/ObjectPointer.h (revision 584) +++ kernel/include/IMP/internal/ObjectPointer.h (working copy) @@ -11,6 +11,7 @@

#include "../log.h" #include "Object.h" +#include "RefCountedObject.h" #include "../macros.h" #include "../exception.h"

@@ -35,6 +36,16 @@ typedef ObjectPointer<O, RC> This; O* o_;

+ void set_pointer(O* p) { + if (RC) { + if (o_) disown(o_); + if (p) own(p); + o_=p; + } else { + o_=p; + } + } + // Enforce that ref counted objects are ref counted BOOST_STATIC_ASSERT((RC || !boost::is_base_of<RefCountedObject, O>::value));

@@ -48,34 +59,21 @@ } typedef bool (This::*unspecified_bool)() const;

- void ref() { - if (RC && o_) o_->ref(); - } - - void unref() { - if (RC && o_) { - o_->unref(); - if (!o_->get_has_ref()) delete o_; - } - } - public: - ObjectPointer(const ObjectPointer &o): o_(o.o_) { - ref(); + ObjectPointer(const ObjectPointer &o): o_(NULL) { + set_pointer(o.o_); } ObjectPointer& operator=(const ObjectPointer &o){ - unref(); - o_=o.o_; - ref(); + set_pointer(o.o_); return *this; } ObjectPointer(): o_(NULL) {} - explicit ObjectPointer(O* o): o_(o) { - IMP_assert(o != NULL, "Can't initialize with NULL pointer"); - ref(); + explicit ObjectPointer(O* o): o_(NULL) { + IMP_assert(o, "Can't initialize with NULL pointer"); + set_pointer(o); } ~ObjectPointer(){ - unref(); + set_pointer(NULL); } const O& operator*() const { audit(); @@ -98,9 +96,7 @@ return o_; } void operator=(O* o) { - unref(); - o_=o; - ref(); + set_pointer(o); } IMP_COMPARISONS_1(o_);

Index: kernel/include/IMP/internal/RefCountedObject.h =================================================================== --- kernel/include/IMP/internal/RefCountedObject.h (revision 584) +++ kernel/include/IMP/internal/RefCountedObject.h (working copy) @@ -22,12 +22,8 @@

//! Common base class for ref counted objects. /** Currently the only ref counted objects are particles. + This class acts as a tag rather than providing any functionality.

- \note Due to weirdness in SWIG, the external objects - are responsible for deleting the ref counted object when - the ref count goes to zero. This will change once we have - a real solution for Python. - \internal */ class IMPDLLEXPORT RefCountedObject: public Object @@ -43,21 +39,12 @@ public:

virtual ~RefCountedObject() { - IMP_assert(count_==0, "Deleting object which still has references"); + IMP_assert(!get_has_ref(), "Deleting object which still has references"); --live_objects_; }

- void ref() { - assert_is_valid(); - ++P::count_; - } - void unref() { - assert_is_valid(); - IMP_assert(count_ != 0, "Too many unrefs on object"); - --P::count_; - } - static unsigned int get_number_of_live_objects() { + // for debugging purposes only return live_objects_; } }; @@ -68,9 +55,9 @@ struct Ref { template <class O> - static void eval(O) { - BOOST_STATIC_ASSERT((!boost::is_pointer<O>::value - || !boost::is_base_of<RefCountedObject, O >::value)); + static void eval(O* o) { + BOOST_STATIC_ASSERT((!boost::is_base_of<RefCountedObject, O >::value)); + IMP_LOG(VERBOSE, "Not refing particle " << o << std::endl); } };

@@ -79,7 +66,10 @@ { template <class O> static void eval(O* o) { - if (o) o->ref(); + IMP_LOG(VERBOSE, "Refing particle " << o->get_index() + << o->get_ref_count() << std::endl); + o->assert_is_valid(); + o->ref(); } };

@@ -87,9 +77,9 @@ struct UnRef { template <class O> - static void eval(O) { - BOOST_STATIC_ASSERT((!boost::is_pointer<O>::value - || !boost::is_base_of<RefCountedObject, O >::value)); + static void eval(O* o) { + BOOST_STATIC_ASSERT((!boost::is_base_of<RefCountedObject, O >::value)); + IMP_LOG(VERBOSE, "Not Unrefing object " << o << std::endl); } };

@@ -98,22 +88,24 @@ { template <class O> static void eval(O *o) { - if (o) { - o->unref(); - if (!o->get_has_ref()) { - delete o; - } + IMP_LOG(VERBOSE, "Unrefing particle " << o->get_index() + << " " << o->get_ref_count() << std::endl); + o->assert_is_valid(); + o->unref(); + if (!o->get_has_ref()) { + delete o; } } -}; + };

//! Can be called on any object and will only unref it if appropriate template <class O> void unref(O o) { - UnRef<(boost::is_pointer<O>::value - && boost::is_base_of<RefCountedObject, O >::value)>::eval(o); + BOOST_STATIC_ASSERT(!boost::is_pointer<O>::value); + /*IMP_LOG(VERBOSE, "NonUnRef called with nonpointer for " + << o << std::endl);*/ }

@@ -121,11 +113,69 @@ template <class O> void ref(O o) { - Ref<(boost::is_pointer<O>::value - && boost::is_base_of<RefCountedObject, O >::value)>::eval(o); + BOOST_STATIC_ASSERT(!boost::is_pointer<O>::value); + /*IMP_LOG(VERBOSE, "NonRef count called with nonpointer for " + << o << std::endl);*/ }

+//! Can be called on any object and will only unref it if appropriate +template <class O> +void unref(O* o) +{ + /*IMP_LOG(VERBOSE, "Unref count called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << std::endl);*/ + UnRef<(boost::is_base_of<RefCountedObject, O >::value)>::eval(o); +}

+ +//! Can be called on any object and will only ref it if appropriate +template <class O> +void ref(O* o) +{ + /*IMP_LOG(VERBOSE, "Ref called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << std::endl);*/ + Ref<(boost::is_base_of<RefCountedObject, O >::value)>::eval(o); +} + + +//! Can be called on any object and will only unref it if appropriate +template <class O> +void disown(O* o) +{ + /*IMP_LOG(VERBOSE, "Disown called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << " " << o->get_ref_count() << std::endl);*/ + o->unref(); + if (!o->get_has_ref()) { + delete o; + } +} + + +//! Can be called on any object and will only ref it if appropriate +template <class O> +void own(O* o) +{ + /*IMP_LOG(VERBOSE, "Own called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o + << " " << o->get_ref_count() << std::endl);*/ + if (boost::is_base_of<RefCountedObject, O >::value) { + // no checks + } else { + IMP_check(!o->get_has_ref(), "Trying to own already owned but " + << "non-reference-counted object.", + ValueException("Already owned object")); + } + o->ref(); +} + + + + + } // namespace internal

} // namespace IMP Index: kernel/test/states/test_nonbonded_list.py =================================================================== --- kernel/test/states/test_nonbonded_list.py (revision 584) +++ kernel/test/states/test_nonbonded_list.py (working copy) @@ -165,7 +165,7 @@ score= m.evaluate(False) self.assertEqual(score, 2*190, "Wrong score")

- ps[3].set_is_active(False) + m.remove_particle(ps[3].get_index()) self.assert_(not ps[3].get_is_active(), "Particle not inactive") ps=None score= m.evaluate(False) Index: kernel/test/particles/test_refcount.py =================================================================== --- kernel/test/particles/test_refcount.py (revision 584) +++ kernel/test/particles/test_refcount.py (working copy) @@ -7,15 +7,23 @@ class RefCountTests(IMP.test.TestCase): """Test refcounting of particles"""

+ def setUp(self): + IMP.test.TestCase.setUp(self) + IMP.set_log_level(IMP.VERBOSE) + self.basenum= IMP.RefCountedObject.get_number_of_live_objects() + print "The base number of objects is " + str(self.basenum) + def _check_number(self, expected): print "Expected "+str(expected)\ - + " got " + str(IMP.RefCountedObject.get_number_of_live_objects()) - self.assertEqual(IMP.RefCountedObject.get_number_of_live_objects(), + + " got " + str(IMP.RefCountedObject.get_number_of_live_objects() + - self.basenum) + self.assertEqual(IMP.RefCountedObject.get_number_of_live_objects() - self.basenum, expected, "wrong number of particles")

- def _test_simple(self): + def __test_simple(self): """Check that ref counting of particles works within python""" + # swig is broken so this needs to be skipped self._check_number(0)

p= IMP.Particle() @@ -33,7 +41,7 @@ m=1 self._check_number(0)

- def _test_removal(self): + def test_removal(self): """Check that ref counting works with removing particles""" self._check_number(0) m= IMP.Model() @@ -47,7 +55,7 @@ self._check_number(0)

# This test does not work since swig refcounting is broken - def _test_round_trip(self): + def __test_round_trip(self): """test that tracking survives the round trip"""

print "test that the round trip object doesn't delete it" @@ -101,24 +109,30 @@ self._check_number(0)

- def _test_shared(self): - """Check that ref counting works shared particles""" + def test_shared(self): + """Check that ref counting works with shared particles""" print "max change" self._check_number(0) m= IMP.Model() p= IMP.Particle() + print "Add particle" pi= m.add_particle(p) + d= IMP.XYZDecorator.create(p) + d=0 mc= IMP.MaxChangeScoreState(IMP.XYZDecorator.get_xyz_keys()) + print "Add particle to mc" mc.add_particle(p) self._check_number(1) + print "Remove from model" m.remove_particle(pi) self._check_number(1) p=1 self._check_number(1) + print "Remove from mc" mc.clear_particles() self._check_number(0)

- def _test_skip(self): + def test_skip(self): """Check that removed particles are skipped""" print "skipped" m= IMP.Model()