From ac975bf79ae6d90e006be0b23960719baf6e0436 Mon Sep 17 00:00:00 2001 From: Robert Osfield Date: Tue, 14 Oct 2008 14:27:41 +0000 Subject: [PATCH] Added a Refrenced::getGlobalReferencedMutex, and OpenThreads::ScopedPointerLock() and use of this in add/removeParent() codes to avoid threading problems when using atomic ref counting. --- include/OpenThreads/ScopedLock | 23 +++++++++++++++++++++++ include/osg/Referenced | 5 ++++- src/osg/Drawable.cpp | 25 +++++-------------------- src/osg/Node.cpp | 25 +++++-------------------- src/osg/Referenced.cpp | 6 ++++++ src/osg/StateAttribute.cpp | 5 +++++ src/osg/StateSet.cpp | 25 +++++-------------------- 7 files changed, 53 insertions(+), 61 deletions(-) diff --git a/include/OpenThreads/ScopedLock b/include/OpenThreads/ScopedLock index fe84efa40..b665ecaa8 100644 --- a/include/OpenThreads/ScopedLock +++ b/include/OpenThreads/ScopedLock @@ -43,5 +43,28 @@ template class ReverseScopedLock ~ReverseScopedLock(){m_lock.lock();} }; + +template class ScopedPointerLock +{ + private: + M* m_lock; + ScopedPointerLock(const ScopedPointerLock&); // prevent copy + ScopedPointerLock& operator=(const ScopedPointerLock&); // prevent assign + public: + explicit ScopedPointerLock(M* m):m_lock(m) { if (m_lock) m_lock->lock();} + ~ScopedPointerLock(){ if (m_lock) m_lock->unlock();} +}; + +template class ReverseScopedPointerLock +{ + private: + M* m_lock; + ReverseScopedPointerLock(const ReverseScopedPointerLock&); // prevent copy + ReverseScopedPointerLock& operator=(const ReverseScopedPointerLock&); // prevent assign + public: + explicit ReverseScopedPointerLock(M* m):m_lock(m) { if (m_lock) m_lock->unlock();} + ~ReverseScopedPointerLock(){ if (m_lock) m_lock->lock();} +}; + } #endif diff --git a/include/osg/Referenced b/include/osg/Referenced index 0196c3d3d..6b4f83809 100644 --- a/include/osg/Referenced +++ b/include/osg/Referenced @@ -70,11 +70,14 @@ class OSG_EXPORT Referenced /** Get the mutex used to ensure thread safety of ref()/unref(). */ #if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - OpenThreads::Mutex* getRefMutex() const { return 0; } + OpenThreads::Mutex* getRefMutex() const { return getGlobalReferencedMutex(); } #else OpenThreads::Mutex* getRefMutex() const { return _refMutex; } #endif + /** Get the optional global Referenced mutex, this can be shared between all osg::Referenced.*/ + static OpenThreads::Mutex* getGlobalReferencedMutex(); + /** Increment the reference count by one, indicating that this object has another pointer which is referencing it.*/ inline void ref() const; diff --git a/src/osg/Drawable.cpp b/src/osg/Drawable.cpp index e5ef9a7c1..8bb2b40e8 100644 --- a/src/osg/Drawable.cpp +++ b/src/osg/Drawable.cpp @@ -281,32 +281,17 @@ void Drawable::computeDataVariance() void Drawable::addParent(osg::Node* node) { - if (getRefMutex()) - { - OpenThreads::ScopedLock lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - _parents.push_back(node); - } - else - { - _parents.push_back(node); - } + _parents.push_back(node); } void Drawable::removeParent(osg::Node* node) { - if (getRefMutex()) - { - OpenThreads::ScopedLock lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); - if (pitr!=_parents.end()) _parents.erase(pitr); - } - else - { - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); - if (pitr!=_parents.end()) _parents.erase(pitr); - } + ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); + if (pitr!=_parents.end()) _parents.erase(pitr); } diff --git a/src/osg/Node.cpp b/src/osg/Node.cpp index c5f945ef6..098a49f65 100644 --- a/src/osg/Node.cpp +++ b/src/osg/Node.cpp @@ -95,32 +95,17 @@ Node::~Node() void Node::addParent(osg::Group* node) { - if (getRefMutex()) - { - OpenThreads::ScopedLock lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - _parents.push_back(node); - } - else - { - _parents.push_back(node); - } + _parents.push_back(node); } void Node::removeParent(osg::Group* node) { - if (getRefMutex()) - { - OpenThreads::ScopedLock lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); - if (pitr!=_parents.end()) _parents.erase(pitr); - } - else - { - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); - if (pitr!=_parents.end()) _parents.erase(pitr); - } + ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),node); + if (pitr!=_parents.end()) _parents.erase(pitr); } void Node::accept(NodeVisitor& nv) diff --git a/src/osg/Referenced.cpp b/src/osg/Referenced.cpp index 7e8fce9aa..0ee55ae5d 100644 --- a/src/osg/Referenced.cpp +++ b/src/osg/Referenced.cpp @@ -79,6 +79,12 @@ struct DeleteHandlerPointer DeleteHandler* _ptr; }; +OpenThreads::Mutex* Referenced::getGlobalReferencedMutex() +{ + return 0; +// static OpenThreads::Mutex s_ReferencedGlobalMutext; +// return &s_ReferencedGlobalMutext; +} typedef std::set ObserverSet; diff --git a/src/osg/StateAttribute.cpp b/src/osg/StateAttribute.cpp index 61b18dbab..80370a9ac 100644 --- a/src/osg/StateAttribute.cpp +++ b/src/osg/StateAttribute.cpp @@ -27,11 +27,16 @@ StateAttribute::StateAttribute() void StateAttribute::addParent(osg::StateSet* object) { + osg::notify(osg::INFO)<<"Adding parent"< lock(getRefMutex()); + _parents.push_back(object); } void StateAttribute::removeParent(osg::StateSet* object) { + OpenThreads::ScopedPointerLock lock(getRefMutex()); + ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),object); if (pitr!=_parents.end()) _parents.erase(pitr); } diff --git a/src/osg/StateSet.cpp b/src/osg/StateSet.cpp index 260057cd4..f1dc5061d 100644 --- a/src/osg/StateSet.cpp +++ b/src/osg/StateSet.cpp @@ -252,32 +252,17 @@ void StateSet::computeDataVariance() void StateSet::addParent(osg::Object* object) { // osg::notify(osg::INFO)<<"Adding parent"< lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - _parents.push_back(object); - } - else - { - _parents.push_back(object); - } + _parents.push_back(object); } void StateSet::removeParent(osg::Object* object) { - if (getRefMutex()) - { - OpenThreads::ScopedLock lock(*getRefMutex()); + OpenThreads::ScopedPointerLock lock(getRefMutex()); - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),object); - if (pitr!=_parents.end()) _parents.erase(pitr); - } - else - { - ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),object); - if (pitr!=_parents.end()) _parents.erase(pitr); - } + ParentList::iterator pitr = std::find(_parents.begin(),_parents.end(),object); + if (pitr!=_parents.end()) _parents.erase(pitr); } int StateSet::compare(const StateSet& rhs,bool compareAttributeContents) const