From d26a8474e75866ac99086955728b64e6cfd73ce3 Mon Sep 17 00:00:00 2001 From: Robert Osfield Date: Fri, 28 May 2010 08:57:48 +0000 Subject: [PATCH] Changed the ref_ptr observer_ptr<>::lock() method to be bool observer_ptr<>::lock(ref_ptr&) to avoid the temporary ref_ptr<>'s being created and destroyed on the stack along with the associated ref/unref() operations --- examples/osgfpdepth/osgfpdepth.cpp | 7 +- include/osg/observer_ptr | 334 +----------------- src/osg/ObserverNodePath.cpp | 3 +- src/osgDB/DatabasePager.cpp | 16 +- .../introspection/osgDB/DatabasePager.cpp | 6 +- .../osgGA/NodeTrackerManipulator.cpp | 6 +- .../introspection/osgWidget/Widget.cpp | 6 +- .../introspection/osgWidget/Window.cpp | 6 +- 8 files changed, 41 insertions(+), 343 deletions(-) diff --git a/examples/osgfpdepth/osgfpdepth.cpp b/examples/osgfpdepth/osgfpdepth.cpp index b86820617..c71634ad8 100644 --- a/examples/osgfpdepth/osgfpdepth.cpp +++ b/examples/osgfpdepth/osgfpdepth.cpp @@ -538,9 +538,10 @@ public: Object*, NodeVisitor* /*nv*/) { if (ea.getHandled()) return false; - ref_ptr depth = _depth.lock(); - if (!depth.valid()) - return false; + + ref_ptr depth; + if (!_depth.lock(depth)) return false; + switch(ea.getEventType()) { case(osgGA::GUIEventAdapter::KEYUP): diff --git a/include/osg/observer_ptr b/include/osg/observer_ptr index 8ea9abd58..174ce544e 100644 --- a/include/osg/observer_ptr +++ b/include/osg/observer_ptr @@ -23,8 +23,6 @@ namespace osg { -#if 1 - /** Smart pointer for observed objects, that automatically set pointers to them to null when they deleted. * To use the observer_ptr<> robustly in multi-threaded applications it is recommend to access the pointer via * the lock() method that passes back a ref_ptr<> that safely takes a reference to the object to prevent deletion @@ -96,17 +94,27 @@ public: } /** - * Create a ref_ptr from a observer_ptr. The ref_ptr will be valid if the + * Assign the observer_ptr to a ref_ptr. The ref_ptr will be valid if the * referenced object hasn't been deleted and has a ref count > 0. */ - ref_ptr lock() const + bool lock(ref_ptr& rptr) const { - if (!_reference) return ref_ptr(); + if (!_reference) + { + rptr = 0; + return false; + } + Referenced* obj = _reference->addRefLock(); - if (!obj) return ref_ptr(); - ref_ptr result(_ptr); + if (!obj) + { + rptr = 0; + return false; + } + + rptr = _ptr; obj->unref_nodelete(); - return result; + return rptr.valid(); } /** Comparison operators. These continue to work even after the @@ -146,316 +154,6 @@ protected: T* _ptr; }; -#else - -// Internal class used to hold the "naked" pointer to the weakly -// referenced object -template -struct WeakReference : public Observer, public Referenced -{ - WeakReference(const T* ptr) : _ptr(const_cast(ptr)) {} - - virtual void objectDeleted(void*) - { - bool deleteNeeded = false; - { - OpenThreads::ScopedLock lock(_mutex); - if (!_ptr) - { - // The last weak reference was deleted after the last - // reference, but the observer hasn't run yet. The - // observer can't be prevented from running, so it - // must delete itself. - deleteNeeded = true; - } - else - { - _ptr = 0; - } - } - if (deleteNeeded) delete this; - } - - /** - * "Lock" a Referenced object i.e., protect it from being deleted - * by incrementing its reference count. - * - * returns null if object doesn't exist anymore - */ - virtual T* addRefLock() - { - OpenThreads::ScopedLock lock(_mutex); - if (!_ptr) return 0; - int refCount = _ptr->ref(); - if (refCount == 1) - { - // The object is in the process of being deleted, but our - // objectDeleted() method hasn't been run yet (and we're - // blocking it -- and the final destruction -- with our lock). - _ptr->unref_nodelete(); - return 0; - } - return _ptr; - } - OpenThreads::Mutex _mutex; - T* _ptr; -}; - -// Reference object for observer_ptr that was initialized with an -// "unsafe" object e.g., a stack-allocated object. - -template -struct UnsafeWeakReference : public WeakReference -{ - UnsafeWeakReference(const T* ptr) : WeakReference(ptr) {} - - // Don't even touch the ref count of an unsafe object - T* addRefLock() - { - OSG_WARN << "tried to lock an unsafe observer_ptr."; - return 0; - } -}; - -/** - Delete the WeakReference object, if necessary. The WeakReference - should be deleted when the last observer_ptr pointing to it goes - out of scope; this avoids a memory leak. A race must be prevented - between this function and WeakReference::objectDeleted(). If the - referenced object is being deleted at the same time as this - function is called, we can't prevent objectDeleted() from being - run, so we need to leave the WeakReference in a valid state and let - objectDeleted() delete it. */ -template -void maybeDelete(WeakReference* weakRef) -{ - if (!weakRef || weakRef->unref_nodelete() > 0) - return; - // No other observer_ptrs hold weakRef, so clean up. - bool doDelete = false; - bool doRemove = false; - UnsafeWeakReference* unsafe = 0; - T* ptr = 0; - { - OpenThreads::ScopedLock lock(weakRef->_mutex); - if (!weakRef->_ptr) - { - // The object has already been deleted; it's OK to delete weakRef. - doDelete = true; - } - else - { - ptr = weakRef->_ptr; - // Carefully remove weakRef as an observer. - unsafe = dynamic_cast*>(weakRef); - if (!unsafe && weakRef->_ptr->ref() == 1) - { - // The referenced object is being deleted, so the - // Observer method must delete the weak reference. - weakRef->_ptr->unref_nodelete(); - weakRef->_ptr = 0; - } - else - { - // The referenced object won't be deleted until we - // decrement the reference count, so remove the - // observer. However, we need to give up the lock on - // weakRef first, to avoid an ordering deadlock; - // removeObserver takes a lock on the observer set. - doRemove = true; - doDelete = true; - } - } - } - if (doRemove) - { - ptr->removeObserver(weakRef); - if (!unsafe) - ptr->unref_nodelete(); - } - if (doDelete) delete weakRef; -} - -/** - * Get a weak reference from a Referenced. This returns a - * WeakReference object with the ref count bumped. We must already - * hold a reference to the Referenced object so it won't get - * deleted during this process. - */ -template WeakReference* getWeakReference(const T* object); - -/** Smart pointer for observed objects, that automatically set pointers to them to null when they deleted. - * To use the observer_ptr<> robustly in multi-threaded applications it is recommend to access the pointer via - * the lock() method that passes back a ref_ptr<> that safely takes a reference to the object to prevent deletion - * during usage of the object. In certain conditions it may be safe to use the pointer directly without using lock(), - * which will confer a perfomance advantage, the conditions are: - * 1) The data structure is only accessed/deleted in single threaded/serial way. - * 2) The data strucutre is guarenteed by high level management of data strucutures and threads which avoid - * possible situations where the observer_ptr<>'s object may be deleted by one thread whilst being accessed - * by another. - * If you are in any doubt about whether it is safe to access the object safe then use - * ref_ptr<> observer_ptr<>.lock() combination. */ -template -class observer_ptr : public Observer -{ -public: - typedef T element_type; - observer_ptr() : _reference(0) {} - /** - * Create a observer_ptr from a ref_ptr. - */ - observer_ptr(const ref_ptr& rp) - { - _reference = getWeakReference(rp.get()); - } - /** - * Create a observer_ptr from a raw pointer. For compatibility; - * the result might not be lockable. - */ - observer_ptr(T* rp) - { - _reference = getWeakReference(rp); - } - - observer_ptr(const observer_ptr& wp) : _reference(wp._reference) - { - if (_reference) _reference->ref(); - } - - ~observer_ptr() - { - maybeDelete(_reference); - } - - observer_ptr& operator = (const observer_ptr& wp) - { - if (wp._reference) wp._reference->ref(); - maybeDelete(_reference); - _reference = wp._reference; - return *this; - } - - observer_ptr& operator = (const ref_ptr& rp) - { - WeakReference* tmp = getWeakReference(rp.get()); - maybeDelete(_reference); - _reference = tmp; - return *this; - } - - observer_ptr& operator = (T* rp) - { - WeakReference* tmp = getWeakReference(rp); - maybeDelete(_reference); - _reference = tmp; - return *this; - } - /** - * Create a ref_ptr from a observer_ptr. The ref_ptr will be valid if the - * referenced object hasn't been deleted and has a ref count > 0. - */ - ref_ptr lock() const - { - if (!_reference) - return ref_ptr(); - T* obj = _reference->addRefLock(); - if (!obj) - return ref_ptr(); - ref_ptr result(obj); - obj->unref_nodelete(); - return result; - } - /** Comparison operators. These continue to work even after the - * observed object has been deleted. - */ - bool operator == (const observer_ptr& wp) const - { return _reference == wp._reference; } - bool operator != (const observer_ptr& wp) const - { return _reference != wp._reference; } - bool operator < (const observer_ptr& wp) const - { return _reference < wp._reference; } - bool operator > (const observer_ptr& wp) const - { return _reference > wp._reference; } - // Non-strict interface, for compatibility - // comparison operator for const T*. - inline bool operator == (const T* ptr) const - { - if (!_reference) - return !ptr; - else - return (_reference->_ptr==ptr); - } - inline bool operator != (const T* ptr) const - { - if (!_reference) - return ptr != 0; - else - return (_reference->_ptr!=ptr); - } - inline bool operator < (const T* ptr) const { return (_reference && _reference->_ptr (const T* ptr) const { return (_reference && _reference->ptr>ptr); } - // Convenience methods for operating on object, however, access is not automatically threadsafe. - // To make thread safe, one should either ensure at a high level - // that the object will not be deleted while operating on it, or - // by using the observer_ptr<>::lock() to get a ref_ptr<> that - // ensures the objects stay alive throughout all access to it. - - // Throw an error if _reference is null? - inline T& operator*() const { return *_reference->_ptr; } - inline T* operator->() const { return _reference? _reference->_ptr : 0; } - // get the raw C pointer - inline T* get() const { return _reference ? _reference->_ptr : 0; } - - inline bool operator!() const { return !_reference || !_reference->_ptr; } - inline bool valid() const { return _reference && _reference->_ptr; } -protected: - // The pointer to the WeakReference is not kept in a ref_ptr so that - // its deletion can be managed explicitly. - WeakReference* _reference; -}; - - -template -WeakReference* -getWeakReference(const T* object) -{ - if (!object) - return 0; - WeakReference* result = 0; - ObserverSet* setData = object->getOrCreateObserverSet(); - OpenThreads::ScopedLock lock(*setData->getObserverSetMutex()); - ObserverSet::Observers &observers = setData->getObservers(); - for (ObserverSet::Observers::iterator itr = observers.begin(), - end = observers.end(); - itr != end; - ++itr) - { - if ((result = dynamic_cast*>(*itr))) - { - int weakRefCount = result->ref(); - if (weakRefCount == 1) - { - // The last weak reference disappeared, but hasn't - // been removed from the observers list yet. - result->unref_nodelete(); - } - else - { - return result; - } - } - } - if (object->referenceCount() > 0) - result = new WeakReference(object); - else - result = new UnsafeWeakReference(object); - result->ref(); - observers.insert(result); - return result; -} - -#endif - } #endif diff --git a/src/osg/ObserverNodePath.cpp b/src/osg/ObserverNodePath.cpp index 912a2f9f5..5ca031e14 100644 --- a/src/osg/ObserverNodePath.cpp +++ b/src/osg/ObserverNodePath.cpp @@ -100,8 +100,7 @@ bool ObserverNodePath::getRefNodePath(RefNodePath& refNodePath) const refNodePath.resize(_nodePath.size()); for(unsigned int i=0; i<_nodePath.size(); ++i) { - refNodePath[i] = _nodePath[i].lock(); - if (!refNodePath[i].valid()) + if (!_nodePath[i].lock(refNodePath[i])) { OSG_INFO<<"ObserverNodePath::getRefNodePath() node has been invalidated"< plod = itr->lock(); - if (plod.valid()) + osg::ref_ptr plod; + if (itr->lock(plod)) { int delta = frameStamp.getFrameNumber() - plod->getFrameNumberOfLastTraversal(); if (delta>1) @@ -1643,8 +1643,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp) itr != _inactivePagedLODList.end(); ) { - osg::ref_ptr plod = itr->lock(); - if (plod.valid()) + osg::ref_ptr plod; + if (itr->lock(plod)) { int delta = frameStamp.getFrameNumber() - plod->getFrameNumberOfLastTraversal(); if (delta>1) @@ -1702,8 +1702,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp) itr!=_inactivePagedLODList.end() && countPagedLODsVisitor._numPagedLODs plod = itr->lock(); - if (plod.valid() && countPagedLODsVisitor._pagedLODs.count(plod.get())==0) + osg::ref_ptr plod; + if (itr->lock(plod) && countPagedLODsVisitor._pagedLODs.count(plod.get())==0) { countPagedLODsVisitor.removeExpiredChildrenAndCountPagedLODs(plod.get(), expiryTime, expiryFrame, childrenRemoved); @@ -1721,8 +1721,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp) itr!=_activePagedLODList.end() && countPagedLODsVisitor._numPagedLODs plod = itr->lock(); - if (plod.valid() && countPagedLODsVisitor._pagedLODs.count(plod.get())==0) + osg::ref_ptr plod; + if (itr->lock(plod) && countPagedLODsVisitor._pagedLODs.count(plod.get())==0) { countPagedLODsVisitor.removeExpiredChildrenAndCountPagedLODs(plod.get(), expiryTime, expiryFrame, childrenRemoved); diff --git a/src/osgWrappers/introspection/osgDB/DatabasePager.cpp b/src/osgWrappers/introspection/osgDB/DatabasePager.cpp index 6a1fcbde6..4eb7694df 100644 --- a/src/osgWrappers/introspection/osgDB/DatabasePager.cpp +++ b/src/osgWrappers/introspection/osgDB/DatabasePager.cpp @@ -561,10 +561,10 @@ BEGIN_OBJECT_REFLECTOR(osg::observer_ptr< osg::GraphicsContext >) ____observer_ptr__C5_observer_ptr_R1, "", ""); - I_Method0(osg::ref_ptr< osg::GraphicsContext >, lock, + I_Method1(bool, lock, IN, osg::ref_ptr< osg::GraphicsContext > &, rptr, Properties::NON_VIRTUAL, - __ref_ptrT1_T___lock, - "Create a ref_ptr from a observer_ptr. ", + __bool__lock__ref_ptrT1_T__R1, + "Assign the observer_ptr to a ref_ptr. ", "The ref_ptr will be valid if the referenced object hasn't been deleted and has a ref count > 0. "); I_Method0(osg::GraphicsContext *, get, Properties::NON_VIRTUAL, diff --git a/src/osgWrappers/introspection/osgGA/NodeTrackerManipulator.cpp b/src/osgWrappers/introspection/osgGA/NodeTrackerManipulator.cpp index 1d619e248..d68532f2a 100644 --- a/src/osgWrappers/introspection/osgGA/NodeTrackerManipulator.cpp +++ b/src/osgWrappers/introspection/osgGA/NodeTrackerManipulator.cpp @@ -251,10 +251,10 @@ BEGIN_OBJECT_REFLECTOR(osg::observer_ptr< osg::Node >) ____observer_ptr__C5_observer_ptr_R1, "", ""); - I_Method0(osg::ref_ptr< osg::Node >, lock, + I_Method1(bool, lock, IN, osg::ref_ptr< osg::Node > &, rptr, Properties::NON_VIRTUAL, - __ref_ptrT1_T___lock, - "Create a ref_ptr from a observer_ptr. ", + __bool__lock__ref_ptrT1_T__R1, + "Assign the observer_ptr to a ref_ptr. ", "The ref_ptr will be valid if the referenced object hasn't been deleted and has a ref count > 0. "); I_Method0(osg::Node *, get, Properties::NON_VIRTUAL, diff --git a/src/osgWrappers/introspection/osgWidget/Widget.cpp b/src/osgWrappers/introspection/osgWidget/Widget.cpp index b0deef432..5bea68f97 100644 --- a/src/osgWrappers/introspection/osgWidget/Widget.cpp +++ b/src/osgWrappers/introspection/osgWidget/Widget.cpp @@ -969,10 +969,10 @@ BEGIN_OBJECT_REFLECTOR(osg::observer_ptr< osgWidget::Widget >) ____observer_ptr__C5_observer_ptr_R1, "", ""); - I_Method0(osg::ref_ptr< osgWidget::Widget >, lock, + I_Method1(bool, lock, IN, osg::ref_ptr< osgWidget::Widget > &, rptr, Properties::NON_VIRTUAL, - __ref_ptrT1_T___lock, - "Create a ref_ptr from a observer_ptr. ", + __bool__lock__ref_ptrT1_T__R1, + "Assign the observer_ptr to a ref_ptr. ", "The ref_ptr will be valid if the referenced object hasn't been deleted and has a ref count > 0. "); I_Method0(osgWidget::Widget *, get, Properties::NON_VIRTUAL, diff --git a/src/osgWrappers/introspection/osgWidget/Window.cpp b/src/osgWrappers/introspection/osgWidget/Window.cpp index 80a62aaf3..a1631b128 100644 --- a/src/osgWrappers/introspection/osgWidget/Window.cpp +++ b/src/osgWrappers/introspection/osgWidget/Window.cpp @@ -971,10 +971,10 @@ BEGIN_OBJECT_REFLECTOR(osg::observer_ptr< osgWidget::Window >) ____observer_ptr__C5_observer_ptr_R1, "", ""); - I_Method0(osg::ref_ptr< osgWidget::Window >, lock, + I_Method1(bool, lock, IN, osg::ref_ptr< osgWidget::Window > &, rptr, Properties::NON_VIRTUAL, - __ref_ptrT1_T___lock, - "Create a ref_ptr from a observer_ptr. ", + __bool__lock__ref_ptrT1_T__R1, + "Assign the observer_ptr to a ref_ptr. ", "The ref_ptr will be valid if the referenced object hasn't been deleted and has a ref count > 0. "); I_Method0(osgWidget::Window *, get, Properties::NON_VIRTUAL,