From 3582fbbf4c3d2089a0af9bb20ba432530d6175e3 Mon Sep 17 00:00:00 2001 From: Steven Hahn Date: Tue, 4 Nov 2014 13:34:51 -0500 Subject: [PATCH] Refs #10456. Fix race condition --- .../MantidGeometry/Instrument/ParameterMap.h | 10 ++-- .../Geometry/src/Instrument/ParameterMap.cpp | 54 ++++++++----------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/Code/Mantid/Framework/Geometry/inc/MantidGeometry/Instrument/ParameterMap.h b/Code/Mantid/Framework/Geometry/inc/MantidGeometry/Instrument/ParameterMap.h index 603e267721fc..12bf75a0e1c6 100644 --- a/Code/Mantid/Framework/Geometry/inc/MantidGeometry/Instrument/ParameterMap.h +++ b/Code/Mantid/Framework/Geometry/inc/MantidGeometry/Instrument/ParameterMap.h @@ -271,7 +271,9 @@ namespace Geometry /// access iterators. end; pmap_it end(){return m_map.end();} pmap_cit end()const{return m_map.end();} - + ParameterMap( const ParameterMap& other) : m_map(other.m_map), m_cacheLocMap(other.m_cacheLocMap), m_cacheRotMap(other.m_cacheRotMap), m_boundingBoxMap(other.m_boundingBoxMap), + m_positionCache(), m_boundingBoxCache(), m_rotationCache(), m_mapAccess() {}; + private: ///Assignment operator ParameterMap& operator=(ParameterMap * rhs); @@ -279,8 +281,7 @@ namespace Geometry component_map_it positionOf(const IComponent* comp,const char *name, const char * type); ///const version of the internal function to get position of the parameter in the parameter map component_map_cit positionOf(const IComponent* comp,const char *name, const char * type) const; - - + /// internal parameter map instance pmap m_map; /// internal cache map instance for cached position values @@ -289,6 +290,9 @@ namespace Geometry mutable Kernel::Cache m_cacheRotMap; ///internal cache map for cached bounding boxes mutable Kernel::Cache m_boundingBoxMap; + + mutable Kernel::Mutex m_positionCache, m_boundingBoxCache, m_rotationCache, m_mapAccess; + }; /// ParameterMap shared pointer typedef diff --git a/Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp b/Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp index 21682630d74e..8b27637fd9f1 100644 --- a/Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp +++ b/Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp @@ -331,22 +331,19 @@ namespace Mantid // can not add null pointer if(!par)return; - PARALLEL_CRITICAL(m_mapAccess) + Kernel::Mutex::ScopedLock _lock(m_mapAccess); + auto existing_par = positionOf(comp,par->name().c_str(),""); + // As this is only an add method it should really throw if it already exists. + // However, this is old behaviour and many things rely on this actually be an + // add/replace-style function + if (existing_par != m_map.end()) + { + existing_par->second = par; + } + else { - auto existing_par = positionOf(comp,par->name().c_str(),""); - // As this is only an add method it should really throw if it already exists. - // However, this is old behaviour and many things rely on this actually be an - // add/replace-style function - if (existing_par != m_map.end()) - { - existing_par->second = par; - } - else - { - m_map.insert(std::make_pair(comp->getComponentID(),par)); - } + m_map.insert(std::make_pair(comp->getComponentID(),par)); } - } /** Create or adjust "pos" parameter for a component @@ -654,8 +651,8 @@ namespace Mantid Parameter_sptr result; if(!comp) return result; - PARALLEL_CRITICAL(m_mapAccess) { + Kernel::Mutex::ScopedLock _lock(m_mapAccess); auto itr = positionOf(comp,name, type); if (itr != m_map.end()) result = itr->second; @@ -739,8 +736,8 @@ namespace Mantid Parameter_sptr ParameterMap::getByType(const IComponent* comp, const std::string& type) const { Parameter_sptr result = Parameter_sptr(); - PARALLEL_CRITICAL(m_mapAccess) { + Kernel::Mutex::ScopedLock _lock(m_mapAccess); if( !m_map.empty() ) { const ComponentID id = comp->getComponentID(); @@ -763,7 +760,7 @@ namespace Mantid } // found->firdst } // it_found != m_map.end() } //!m_map.empty() - } // PARALLEL_CRITICAL(m_map_access) + } return result; } @@ -920,10 +917,8 @@ namespace Mantid void ParameterMap::setCachedLocation(const IComponent* comp, const V3D& location) const { // Call to setCachedLocation is a write so not thread-safe - PARALLEL_CRITICAL(positionCache) - { - m_cacheLocMap.setCache(comp->getComponentID(),location); - } + Kernel::Mutex::ScopedLock _lock(m_positionCache); + m_cacheLocMap.setCache(comp->getComponentID(),location); } ///Attempts to retrieve a location from the location cache @@ -933,8 +928,8 @@ namespace Mantid bool ParameterMap::getCachedLocation(const IComponent* comp, V3D& location) const { bool inMap(false); - PARALLEL_CRITICAL(positionCache) { + Kernel::Mutex::ScopedLock _lock(m_positionCache); inMap = m_cacheLocMap.getCache(comp->getComponentID(),location); } return inMap; @@ -946,10 +941,8 @@ namespace Mantid void ParameterMap::setCachedRotation(const IComponent* comp, const Quat& rotation) const { // Call to setCachedRotation is a write so not thread-safe - PARALLEL_CRITICAL(rotationCache) - { - m_cacheRotMap.setCache(comp->getComponentID(),rotation); - } + Kernel::Mutex::ScopedLock _lock(m_rotationCache); + m_cacheRotMap.setCache(comp->getComponentID(),rotation); } ///Attempts to retrieve a rotation from the rotation cache @@ -959,8 +952,8 @@ namespace Mantid bool ParameterMap::getCachedRotation(const IComponent* comp, Quat& rotation) const { bool inMap(false); - PARALLEL_CRITICAL(rotationCache) { + Kernel::Mutex::ScopedLock _lock(m_rotationCache); inMap = m_cacheRotMap.getCache(comp->getComponentID(),rotation); } return inMap; @@ -972,10 +965,8 @@ namespace Mantid void ParameterMap::setCachedBoundingBox(const IComponent *comp, const BoundingBox & box) const { // Call to setCachedRotation is a write so not thread-safe - PARALLEL_CRITICAL(boundingBoxCache) - { - m_boundingBoxMap.setCache(comp->getComponentID(), box); - } + Kernel::Mutex::ScopedLock _lock(m_boundingBoxCache); + m_boundingBoxMap.setCache(comp->getComponentID(), box); } ///Attempts to retrieve a bounding box from the cache @@ -984,6 +975,7 @@ namespace Mantid /// @returns true if the bounding is in the map, otherwise false bool ParameterMap::getCachedBoundingBox(const IComponent *comp, BoundingBox & box) const { + Kernel::Mutex::ScopedLock _lock(m_boundingBoxCache); return m_boundingBoxMap.getCache(comp->getComponentID(),box); }