From 3bbb272ad50ba7b9e88bfd6c4a733055b9603b08 Mon Sep 17 00:00:00 2001 From: Thomas Geymayer Date: Mon, 23 Jun 2014 00:27:41 +0200 Subject: [PATCH] nasal::Ghost: improve intrusive pointer storage and weak references. - Just increment/decrement reference count for intrusive smart pointers. No need to create an additional object on the heap. - Keep strong reference for weak pointer based ghosts to prevent destroying objects while beeing used. --- simgear/nasal/cppbind/Ghost.hxx | 206 ++++++++++++------ simgear/nasal/cppbind/cppbind_test_ghost.cxx | 52 +++++ simgear/nasal/cppbind/detail/nasal_traits.hxx | 164 +++++++++++++- 3 files changed, 343 insertions(+), 79 deletions(-) diff --git a/simgear/nasal/cppbind/Ghost.hxx b/simgear/nasal/cppbind/Ghost.hxx index b7d1d849..6dcb5e15 100644 --- a/simgear/nasal/cppbind/Ghost.hxx +++ b/simgear/nasal/cppbind/Ghost.hxx @@ -50,12 +50,25 @@ inline T* get_pointer(SGWeakPtr const& p) return p.lock().get(); } -template -inline T* get_pointer(osg::observer_ptr const& p) +namespace osg { - osg::ref_ptr ref; - p.lock(ref); - return ref.get(); + template + inline T* get_pointer(observer_ptr const& p) + { + ref_ptr ref; + p.lock(ref); + return ref.get(); + } +} + +template +inline typename boost::enable_if< + boost::is_pointer, + T +>::type +get_pointer(T ptr) +{ + return ptr; } // Both ways of retrieving the address of a static member function @@ -292,9 +305,26 @@ namespace nasal if( !holder ) throw std::runtime_error("holder has expired"); + // Keep reference for duration of call to prevent expiring + // TODO not needed for strong referenced ghost + strong_ref ref = fromNasal(c, me); + if( !ref ) + { + naGhostType* ghost_type = naGhost_type(me); + naRuntimeError + ( + c, + "method called on object of wrong type: " + "is '%s' expected '%s'", + naIsNil(me) ? "nil" + : (ghost_type ? ghost_type->name : "unknown"), + _ghost_type_strong.name + ); + } + return holder->_method ( - requireObject(c, me), + *get_pointer(ref), CallContext(c, argc, args) ); } @@ -793,7 +823,9 @@ namespace nasal if( !ghost_type ) return naNil(); - return naNewGhost2(c, ghost_type, new RefType(ref_ptr)); + return naNewGhost2( c, + ghost_type, + shared_ptr_storage::ref(ref_ptr) ); } /** @@ -801,14 +833,8 @@ namespace nasal * Nasal objects has to be derived class of the target class (Either * derived in C++ or in Nasal using a 'parents' vector) */ - template - static - typename boost::enable_if_c< - boost::is_same::value - || boost::is_same::value, - RefType - >::type - fromNasal(naContext c, naRef me) + template + static Type fromNasal(naContext c, naRef me) { bool is_weak = false; @@ -816,8 +842,8 @@ namespace nasal if( isBaseOf(naGhost_type(me), is_weak) ) { void* ghost = naGhost_ptr(me); - return is_weak ? getPtr(ghost) - : getPtr(ghost); + return is_weak ? getPtr(ghost) + : getPtr(ghost); } // Now if it is derived from a ghost (hash with ghost in parent vector) @@ -825,7 +851,7 @@ namespace nasal { naRef na_parents = naHash_cget(me, const_cast("parents")); if( !naIsVector(na_parents) ) - return RefType(); + return Type(); typedef std::vector naRefs; naRefs parents = from_nasal(c, na_parents); @@ -833,13 +859,13 @@ namespace nasal parent != parents.end(); ++parent ) { - RefType ptr = fromNasal(c, *parent); + Type ptr = fromNasal(c, *parent); if( get_pointer(ptr) ) return ptr; } } - return RefType(); + return Type(); } static bool isBaseOf(naRef obj) @@ -881,8 +907,11 @@ namespace nasal >::type getPtr(void* ptr) { + typedef shared_ptr_storage storage_type; if( ptr ) - return RefPtr(*static_cast(ptr)); + return storage_type::template get( + static_cast(ptr) + ); else return RefPtr(); } @@ -895,8 +924,11 @@ namespace nasal >::type getPtr(void* ptr) { + typedef shared_ptr_storage storage_type; if( ptr ) - return RefPtr(*static_cast(ptr)); + return storage_type::template get( + static_cast(ptr) + ); else return RefPtr(); } @@ -990,25 +1022,6 @@ namespace nasal return getSingletonHolder().get(); } - static raw_type& requireObject(naContext c, naRef me) - { - raw_type* obj = get_pointer( fromNasal(c, me) ); - - if( !obj ) - { - naGhostType* ghost_type = naGhost_type(me); - naRuntimeError - ( - c, - "method called on object of wrong type: is '%s' expected '%s'", - naIsNil(me) ? "nil" : (ghost_type ? ghost_type->name : "unknown"), - _ghost_type_strong.name - ); - } - - return *obj; - } - template getter_t to_getter(Ret (raw_type::*getter)() const) { @@ -1138,10 +1151,8 @@ namespace nasal _ghost_type_strong.destroy = SG_GET_TEMPLATE_MEMBER(Ghost, queueDestroy); _ghost_type_strong.name = _name_strong.c_str(); - _ghost_type_strong.get_member = - SG_GET_TEMPLATE_MEMBER(Ghost, getMember); - _ghost_type_strong.set_member = - SG_GET_TEMPLATE_MEMBER(Ghost, setMember); + _ghost_type_strong.get_member = &Ghost::getMemberStrong; + _ghost_type_strong.set_member = &Ghost::setMemberStrong; _ghost_type_weak.destroy = SG_GET_TEMPLATE_MEMBER(Ghost, queueDestroy); @@ -1149,10 +1160,8 @@ namespace nasal if( supports_weak_ref::value ) { - _ghost_type_weak.get_member = - SG_GET_TEMPLATE_MEMBER(Ghost, getMember); - _ghost_type_weak.set_member = - SG_GET_TEMPLATE_MEMBER(Ghost, setMember); + _ghost_type_weak.get_member = &Ghost::getMemberWeak; + _ghost_type_weak.set_member = &Ghost::setMemberWeak; } else { @@ -1170,7 +1179,10 @@ namespace nasal template static void destroy(void *ptr) { - delete static_cast(ptr); + typedef shared_ptr_storage storage_type; + storage_type::unref( + static_cast(ptr) + ); } template @@ -1179,6 +1191,17 @@ namespace nasal _destroy_list.push_back( DestroyList::value_type(&destroy, ptr) ); } + static void raiseErrorExpired(naContext c, const char* str) + { + Ghost* ghost_info = getSingletonPtr(); + naRuntimeError( + c, + "Ghost::%s: ghost has expired '%s'", + str, + ghost_info ? ghost_info->_name_strong.c_str() : "unknown" + ); + } + /** * Callback for retrieving a ghost member. */ @@ -1218,14 +1241,25 @@ namespace nasal return ""; } - template - static const char* getMember( naContext c, - void* ghost, - naRef key, - naRef* out ) + static const char* + getMemberWeak(naContext c, void* ghost, naRef key, naRef* out) { - strong_ref const& ptr = getPtr(ghost); - return getMemberImpl(c, *get_pointer(ptr), key, out); + // Keep a strong reference while retrieving member, to prevent deleting + // object in between. + strong_ref ref = getPtr(ghost); + if( !ref ) + raiseErrorExpired(c, "getMember"); + + return getMemberImpl(c, *get_pointer(ref), key, out); + } + + static const char* + getMemberStrong(naContext c, void* ghost, naRef key, naRef* out) + { + // Just get a raw pointer as we are keeping a strong reference as ghost + // anyhow. + raw_type* ptr = getPtr(ghost); + return getMemberImpl(c, *ptr, key, out); } /** @@ -1256,14 +1290,25 @@ namespace nasal member->second.setter(obj, c, val); } - template - static void setMember( naContext c, - void* ghost, - naRef field, - naRef val ) + static void + setMemberWeak(naContext c, void* ghost, naRef field, naRef val) { - strong_ref const& ptr = getPtr(ghost); - return setMemberImpl(c, *get_pointer(ptr), field, val); + // Keep a strong reference while retrieving member, to prevent deleting + // object in between. + strong_ref ref = getPtr(ghost); + if( !ref ) + raiseErrorExpired(c, "setMember"); + + setMemberImpl(c, *get_pointer(ref), field, val); + } + + static void + setMemberStrong(naContext c, void* ghost, naRef field, naRef val) + { + // Just get a raw pointer as we are keeping a strong reference as ghost + // anyhow. + raw_type* ptr = getPtr(ghost); + setMemberImpl(c, *ptr, field, val); } }; @@ -1313,7 +1358,7 @@ from_nasal_helper(naContext c, naRef ref, const T*) } /** - * Convert any pointer to a SGReference based object to a ghost. + * Convert any pointer to a SGReferenced based object to a ghost. */ template typename boost::enable_if_c< @@ -1327,7 +1372,7 @@ to_nasal_helper(naContext c, T* ptr) } /** - * Convert nasal ghosts/hashes to pointer (of a SGReference based ghost). + * Convert nasal ghosts/hashes to pointer (of a SGReferenced based ghost). */ template typename boost::enable_if_c< @@ -1344,7 +1389,34 @@ typename boost::enable_if_c< from_nasal_helper(naContext c, naRef ref, const T*) { typedef SGSharedPtr::type> TypeRef; - return nasal::Ghost::template fromNasal(c, ref).release(); + return nasal::Ghost::template fromNasal(c, ref); +} + +/** + * Convert any pointer to a osg::Referenced based object to a ghost. + */ +template +typename boost::enable_if< + boost::is_base_of, + naRef +>::type +to_nasal_helper(naContext c, T* ptr) +{ + return nasal::Ghost >::makeGhost(c, osg::ref_ptr(ptr)); +} + +/** + * Convert nasal ghosts/hashes to pointer (of a osg::Referenced based ghost). + */ +template +typename boost::enable_if< + boost::is_base_of::type>, + T +>::type +from_nasal_helper(naContext c, naRef ref, const T*) +{ + typedef osg::ref_ptr::type> TypeRef; + return nasal::Ghost::template fromNasal(c, ref); } #endif /* SG_NASAL_GHOST_HXX_ */ diff --git a/simgear/nasal/cppbind/cppbind_test_ghost.cxx b/simgear/nasal/cppbind/cppbind_test_ghost.cxx index 569e8e7a..67a265bd 100644 --- a/simgear/nasal/cppbind/cppbind_test_ghost.cxx +++ b/simgear/nasal/cppbind/cppbind_test_ghost.cxx @@ -2,6 +2,8 @@ #include #include "Ghost.hxx" +#include +#include class Base1: public virtual SGVirtualWeakReferenced @@ -36,6 +38,7 @@ typedef SGSharedPtr SGReferencedPtr; CHECK_PTR_TRAIT_TYPE(weak, weak_ref, weak)\ CHECK_PTR_TRAIT(DerivedPtr, DerivedWeakPtr) +CHECK_PTR_TRAIT(boost::shared_ptr, boost::weak_ptr) #undef CHECK_PTR_TRAIT #undef CHECK_PTR_TRAIT_TYPE @@ -87,3 +90,52 @@ BOOST_AUTO_TEST_CASE( ghost_weak_strong_nasal_conversion ) BOOST_REQUIRE( !weak.lock() ); } + +#define CHECK_PTR_STORAGE_TRAIT_TYPE(ptr_t, storage)\ + BOOST_STATIC_ASSERT((boost::is_same<\ + nasal::shared_ptr_storage::storage_type,\ + storage\ + >::value)); + +CHECK_PTR_STORAGE_TRAIT_TYPE(DerivedPtr, Derived) +CHECK_PTR_STORAGE_TRAIT_TYPE(DerivedWeakPtr, DerivedWeakPtr) + +typedef boost::shared_ptr BoostDerivedPtr; +CHECK_PTR_STORAGE_TRAIT_TYPE(BoostDerivedPtr, BoostDerivedPtr) + +typedef boost::weak_ptr BoostDerivedWeakPtr; +CHECK_PTR_STORAGE_TRAIT_TYPE(BoostDerivedWeakPtr, BoostDerivedWeakPtr) + +#undef CHECK_PTR_STORAGE_TRAIT_TYPE + +BOOST_STATIC_ASSERT(( nasal::shared_ptr_traits::is_intrusive::value)); +BOOST_STATIC_ASSERT(( nasal::shared_ptr_traits::is_intrusive::value)); +BOOST_STATIC_ASSERT(( nasal::shared_ptr_traits::is_intrusive::value)); +BOOST_STATIC_ASSERT(( nasal::shared_ptr_traits::is_intrusive::value)); +BOOST_STATIC_ASSERT(( nasal::shared_ptr_traits::is_intrusive::value)); + +BOOST_STATIC_ASSERT((!nasal::shared_ptr_traits >::is_intrusive::value)); +BOOST_STATIC_ASSERT((!nasal::shared_ptr_traits >::is_intrusive::value)); + +BOOST_AUTO_TEST_CASE( storage_traits ) +{ + DerivedPtr d = new Derived(); + + Derived* d_raw = nasal::shared_ptr_storage::ref(d); + BOOST_REQUIRE_EQUAL(d_raw, d.get()); + BOOST_REQUIRE_EQUAL(d.getNumRefs(), 2); + + DerivedWeakPtr* d_weak = nasal::shared_ptr_storage::ref(d); + BOOST_REQUIRE_EQUAL( + nasal::shared_ptr_storage::get(d_weak), + d_raw + ); + + d.reset(); + BOOST_REQUIRE_EQUAL(Derived::count(d_raw), 1); + + nasal::shared_ptr_storage::unref(d_raw); + BOOST_REQUIRE(d_weak->expired()); + + nasal::shared_ptr_storage::unref(d_weak); +} diff --git a/simgear/nasal/cppbind/detail/nasal_traits.hxx b/simgear/nasal/cppbind/detail/nasal_traits.hxx index 62c4d302..f3126918 100644 --- a/simgear/nasal/cppbind/detail/nasal_traits.hxx +++ b/simgear/nasal/cppbind/detail/nasal_traits.hxx @@ -22,8 +22,10 @@ #include #include +#include // Forward declarations +class SGReferenced; class SGWeakReferenced; template class SGSharedPtr; template class SGWeakPtr; @@ -36,6 +38,7 @@ namespace boost } namespace osg { + class Referenced; template class ref_ptr; template class observer_ptr; @@ -77,25 +80,27 @@ SG_MAKE_TRAIT(<>, osg::Vec2s, is_vec2) public boost::integral_constant {}; -#define SG_MAKE_SHARED_PTR_TRAIT(ref, weak)\ +#define SG_MAKE_SHARED_PTR_TRAIT(strong, weak, intrusive)\ template\ - struct shared_ptr_traits >\ + struct shared_ptr_traits >\ {\ - typedef ref strong_ref;\ - typedef weak weak_ref;\ - typedef T element_type;\ + typedef strong strong_ref;\ + typedef weak weak_ref;\ + typedef T element_type;\ typedef boost::integral_constant is_strong;\ + typedef boost::integral_constant is_intrusive;\ };\ template\ struct shared_ptr_traits >\ {\ - typedef ref strong_ref;\ - typedef weak weak_ref;\ - typedef T element_type;\ + typedef strong strong_ref;\ + typedef weak weak_ref;\ + typedef T element_type;\ typedef boost::integral_constant is_strong;\ + typedef boost::integral_constant is_intrusive;\ };\ template\ - struct is_strong_ref >:\ + struct is_strong_ref >:\ public boost::integral_constant\ {};\ template\ @@ -103,9 +108,9 @@ SG_MAKE_TRAIT(<>, osg::Vec2s, is_vec2) public boost::integral_constant\ {}; - SG_MAKE_SHARED_PTR_TRAIT(SGSharedPtr, SGWeakPtr) - SG_MAKE_SHARED_PTR_TRAIT(osg::ref_ptr, osg::observer_ptr) - SG_MAKE_SHARED_PTR_TRAIT(boost::shared_ptr, boost::weak_ptr) + SG_MAKE_SHARED_PTR_TRAIT(SGSharedPtr, SGWeakPtr, true) + SG_MAKE_SHARED_PTR_TRAIT(osg::ref_ptr, osg::observer_ptr, true) + SG_MAKE_SHARED_PTR_TRAIT(boost::shared_ptr, boost::weak_ptr, false) #undef SG_MAKE_SHARED_PTR_TRAIT @@ -122,5 +127,140 @@ SG_MAKE_TRAIT(<>, osg::Vec2s, is_vec2) > {}; + template + struct shared_ptr_storage + { + typedef T storage_type; + typedef typename T::element_type element_type; + typedef typename shared_ptr_traits::strong_ref strong_ref; + typedef typename shared_ptr_traits::weak_ref weak_ref; + + template + static storage_type* ref(U ptr) + { + return new storage_type(ptr); + } + static void unref(storage_type* ptr) + { + delete ptr; + } + + template + static + typename boost::enable_if< + boost::is_same, + element_type* + >::type + get(storage_type* ptr) + { + return get_pointer(*ptr); + } + template + static + typename boost::enable_if_c< + boost::is_same::value + || (boost::is_same::value && supports_weak_ref::value), + U + >::type + get(storage_type* ptr) + { + return U(*ptr); + } + template + static + typename boost::enable_if_c< + boost::is_same::value + && !supports_weak_ref::value, + U + >::type + get(storage_type* ptr) + { + return U(); + } + }; + + namespace internal + { + template + struct intrusive_ptr_storage + { + typedef typename T::element_type storage_type; + typedef typename T::element_type element_type; + typedef typename shared_ptr_traits::strong_ref strong_ref; + typedef typename shared_ptr_traits::weak_ref weak_ref; + + template + static + typename boost::enable_if< + boost::is_same, + element_type* + >::type + get(storage_type* ptr) + { + return ptr; + } + template + static + typename boost::enable_if_c< + boost::is_same::value + || (boost::is_same::value && supports_weak_ref::value), + U + >::type + get(storage_type* ptr) + { + return U(ptr); + } + template + static + typename boost::enable_if_c< + boost::is_same::value + && !supports_weak_ref::value, + U + >::type + get(storage_type* ptr) + { + return U(); + } + }; + } + + template + struct shared_ptr_storage >: + public internal::intrusive_ptr_storage > + { + typedef T storage_type; + typedef T element_type; + + static storage_type* ref(element_type* ptr) + { + T::get(ptr); + return ptr; + } + static void unref(storage_type* ptr) + { + if( !T::put(ptr) ) + delete ptr; + } + }; + + template + struct shared_ptr_storage >: + public internal::intrusive_ptr_storage > + { + typedef T storage_type; + typedef T element_type; + + + static storage_type* ref(element_type* ptr) + { + ptr->ref(); + return ptr; + } + static void unref(storage_type* ptr) + { + ptr->unref(); + } + }; + } // namespace nasal #endif /* SG_NASAL_TRAITS_HXX_ */