diff --git a/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx b/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx index ab4fdd13108b309b65e22e4f4acb391a8f886601..f48d4e692b44df9a43e254a77e0114940c180982 100644 --- a/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx +++ b/Event/xAOD/xAODCore/Root/AuxContainerBase.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ // $Id: AuxContainerBase.cxx 793746 2017-01-24 21:23:52Z ssnyder $ @@ -17,6 +17,8 @@ #include "xAODCore/tools/IOStats.h" #include "xAODCore/tools/ReadStats.h" +#include "CxxUtils/checker_macros.h" + using namespace std; namespace xAOD { @@ -66,11 +68,11 @@ namespace xAOD { /// /// @param store Another store that should be wrapped, but not owned /// - AuxContainerBase::AuxContainerBase( const SG::IAuxStore* store ) + AuxContainerBase::AuxContainerBase( SG::IAuxStore* store ) : SG::IAuxStore(), m_selection(), m_auxids(), m_vecs(), - m_store( const_cast< SG::IAuxStore* >( store ) ), + m_store( store ), m_storeIO( 0 ), m_ownsStore( false ), m_locked( false), m_name( "UNKNOWN" ) { @@ -195,10 +197,11 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); const void* result = m_store->getData( auxid ); - if( result && nids != m_store->getAuxIDs().size() ) { - m_auxids.insert( auxid ); + if( result ) { + auxid_set_t& auxids_nc ATLAS_THREAD_SAFE = + const_cast<auxid_set_t&> (m_auxids); + auxids_nc.insert( auxid ); } return result; } else { @@ -243,9 +246,8 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { // If not, but we have a dynamic store, push it in there: if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); void* result = m_store->getDecoration( auxid, size, capacity ); - if( result && ( nids != m_store->getAuxIDs().size() ) ) { + if( result ) { m_auxids.insert( auxid ); } return result; @@ -365,9 +367,8 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); void* result = m_store->getData( auxid, size, capacity ); - if( result && nids != m_store->getAuxIDs().size() ) { + if( result ) { m_auxids.insert( auxid ); } return result; diff --git a/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx b/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx index 786ce799788d63eefd26cfb6f6dadffc029f30f5..b935cc7881134b086630738e6ff96d1a5f3263a4 100644 --- a/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx +++ b/Event/xAOD/xAODCore/Root/AuxInfoBase.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ // $Id: AuxInfoBase.cxx 793737 2017-01-24 20:11:10Z ssnyder $ @@ -19,6 +19,8 @@ #include "xAODCore/tools/IOStats.h" #include "xAODCore/tools/ReadStats.h" +#include "CxxUtils/checker_macros.h" + namespace xAOD { AuxInfoBase::AuxInfoBase( bool allowDynamicVars ) @@ -67,11 +69,11 @@ namespace xAOD { /// /// @param store Another store that should be wrapped, but not owned /// - AuxInfoBase::AuxInfoBase( const SG::IAuxStore* store ) + AuxInfoBase::AuxInfoBase( SG::IAuxStore* store ) : SG::IAuxStore(), m_selection(), m_auxids(), m_vecs(), - m_store( const_cast< SG::IAuxStore* >( store ) ), + m_store( store ), m_storeIO( 0 ), m_ownsStore( false ), m_locked( false ), m_name( "UNKNOWN" ) { @@ -197,10 +199,11 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); const void* result = m_store->getData( auxid ); - if( result && ( nids != m_store->getAuxIDs().size() ) ) { - m_auxids.insert( auxid ); + if( result ) { + auxid_set_t& auxids_nc ATLAS_THREAD_SAFE = + const_cast<auxid_set_t&> (m_auxids); + auxids_nc.insert( auxid ); } return result; } else { @@ -245,9 +248,8 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { // If not, but we have a dynamic store, push it in there: if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); void* result = m_store->getDecoration( auxid, size, capacity ); - if( result && ( nids != m_store->getAuxIDs().size() ) ) { + if( result ) { m_auxids.insert( auxid ); } return result; @@ -365,9 +367,8 @@ namespace xAOD { if( ( auxid >= m_vecs.size() ) || ( ! m_vecs[ auxid ] ) ) { if( m_store ) { - size_t nids = m_store->getAuxIDs().size(); void* result = m_store->getData( auxid, size, capacity ); - if( result && ( nids != m_store->getAuxIDs().size() ) ) { + if( result ) { m_auxids.insert( auxid ); } return result; diff --git a/Event/xAOD/xAODCore/Root/AuxSelection.cxx b/Event/xAOD/xAODCore/Root/AuxSelection.cxx index ec608f67f6ef069ad3ad8931f382e8cdfe47b984..78312dd41a315f88b840027eadeb998eb98b0c30 100644 --- a/Event/xAOD/xAODCore/Root/AuxSelection.cxx +++ b/Event/xAOD/xAODCore/Root/AuxSelection.cxx @@ -13,11 +13,15 @@ // Local include(s): #include "xAODCore/AuxSelection.h" +#include "CxxUtils/checker_macros.h" +#include <mutex> namespace { - /// Helper variable to only print warning about missing variables once - static std::set< std::string > mentionedVariableNames; + /// Helper variable to only print warning about missing variables once, + /// with a mutex. + std::set< std::string > mentionedVariableNames ATLAS_THREAD_SAFE; + std::mutex mentionedMutex; } // private namespace @@ -103,6 +107,7 @@ namespace xAOD { } } else { // Check if a warning should be printed at this time or not: + std::lock_guard<std::mutex> lock (mentionedMutex); if( ::mentionedVariableNames.insert( *name_itr ).second ) { // Apparently we didn't complain about this name yet... std::cerr << "xAOD::AuxSelection WARNING Selected dynamic " diff --git a/Event/xAOD/xAODCore/Root/IOStats.cxx b/Event/xAOD/xAODCore/Root/IOStats.cxx index 374c86f3b8e4a80fc6e2a23b149796bd9386210e..5cc40ecc22ba4f6817a3d7d4909a29613eb44d5c 100644 --- a/Event/xAOD/xAODCore/Root/IOStats.cxx +++ b/Event/xAOD/xAODCore/Root/IOStats.cxx @@ -1,15 +1,16 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration */ // Local include(s): #include "xAODCore/tools/IOStats.h" +#include "CxxUtils/checker_macros.h" namespace xAOD { IOStats& IOStats::instance() { - static IOStats obj; + static IOStats obj ATLAS_THREAD_SAFE; return obj; } diff --git a/Event/xAOD/xAODCore/Root/PerfStats.cxx b/Event/xAOD/xAODCore/Root/PerfStats.cxx index 252a629929db1597dd3be48a1a9e2e1d0c256477..2de4c34890a5ffc1488ce662503cbf98606d8291 100644 --- a/Event/xAOD/xAODCore/Root/PerfStats.cxx +++ b/Event/xAOD/xAODCore/Root/PerfStats.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration */ // $Id: PerfStats.cxx 634033 2014-12-05 14:46:38Z krasznaa $ @@ -20,18 +20,20 @@ ClassImp( xAOD::PerfStats ) namespace xAOD { // Initialize the static variable(s): - PerfStats* PerfStats::m_instance = 0; + PerfStats* PerfStats::s_instance = 0; + std::mutex PerfStats::s_mutex; /// The destructor is a quite important function in this class. - /// it makes sure that the static m_instance variable gets reset, + /// it makes sure that the static s_instance variable gets reset, /// and that all TVirtualPerfStats objects really get deleted. /// PerfStats::~PerfStats() { + lock_t lock (s_mutex); // Since this object can only be deleted by deleting the global // gPerfStats object, make sure that all the objects get deleted // if the user asked for it... - m_instance = 0; + s_instance = 0; if( m_otherPerfStats ) { delete m_otherPerfStats; } @@ -44,16 +46,13 @@ namespace xAOD { /// PerfStats& PerfStats::instance() { + lock_t lock (s_mutex); // Construct the object if it is now available at the moment: - if( ! m_instance ) { - m_instance = new PerfStats(); + if( ! s_instance ) { + s_instance = new PerfStats(); } - // Make sure that this is still the object that receives - // monitoring information: - gPerfStats = m_instance; - - return *m_instance; + return *s_instance; } /// The user is supposed to call this function after the initialization of @@ -63,6 +62,7 @@ namespace xAOD { /// void PerfStats::start( bool clear ) { + lock_t lock (s_mutex); // Return right away if we are running already: if( m_running ) return; @@ -85,6 +85,7 @@ namespace xAOD { /// void PerfStats::stop() { + lock_t lock (s_mutex); // Return right away if we are not running: if( ! m_running ) return; @@ -349,6 +350,8 @@ namespace xAOD { : m_otherPerfStats( 0 ), m_running( false ), m_startTime( 0.0 ), m_tree( 0 ), m_file( 0 ), m_treeWarningPrinted( false ) { + // locked via instance(). + // Remember a possible former performance monitoring object: if( gPerfStats && ( gPerfStats != this ) ) { m_otherPerfStats = gPerfStats; diff --git a/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx b/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx index 8cbb01c34713b59847e883bb323b12e05f591753..37471b35b7f61eb43f59540fff154006456ccd77 100644 --- a/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx +++ b/Event/xAOD/xAODCore/Root/ShallowAuxContainer.cxx @@ -62,9 +62,7 @@ namespace xAOD { m_name( "UNKNOWN" ) { m_storeIO = dynamic_cast< SG::IAuxStoreIO* >( m_store ); - const SG::IAuxStoreIO* temp = - dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); - m_parentIO = const_cast< SG::IAuxStoreIO* >( temp ); + m_parentIO = dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); } ShallowAuxContainer::~ShallowAuxContainer() { @@ -115,9 +113,7 @@ namespace xAOD { guard_t guard (m_mutex); m_parentLink = link; - const SG::IAuxStoreIO* temp = - dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); - m_parentIO = const_cast< SG::IAuxStoreIO* >( temp ); + m_parentIO = dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); m_auxidsValid = false; return; } @@ -417,15 +413,8 @@ namespace xAOD { } // Do we have a parent that has it? - if( m_parentLink.isValid() ) { - if( ! m_parentIO ) { - const SG::IAuxStoreIO* temp = - dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); - m_parentIO = const_cast< SG::IAuxStoreIO* >( temp ); - } - if( m_parentIO ) { - return m_parentIO->getIOData( auxid ); - } + if( m_parentIO ) { + return m_parentIO->getIOData( auxid ); } // If not, then where did this variable come from?!? @@ -445,15 +434,8 @@ namespace xAOD { } // Do we have a parent that has it? - if( m_parentLink.isValid() ) { - if( ! m_parentIO ) { - const SG::IAuxStoreIO* temp = - dynamic_cast< const SG::IAuxStoreIO* >( m_parentLink.cptr() ); - m_parentIO = const_cast< SG::IAuxStoreIO* >( temp ); - } - if( m_parentIO ) { - return m_parentIO->getIOType( auxid ); - } + if( m_parentIO ) { + return m_parentIO->getIOType( auxid ); } // If not, then where did this variable come from?!? diff --git a/Event/xAOD/xAODCore/Root/TDVCollectionProxy.cxx b/Event/xAOD/xAODCore/Root/TDVCollectionProxy.cxx index e3442a1b6ab10de2bcf8b394d4a1b4dad7d632ce..94225360fb2fae406281fd1bf9d0e89ceae60ffb 100644 --- a/Event/xAOD/xAODCore/Root/TDVCollectionProxy.cxx +++ b/Event/xAOD/xAODCore/Root/TDVCollectionProxy.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration */ // $Id: TDVCollectionProxy.cxx 660500 2015-04-14 14:04:12Z krasznaa $ @@ -19,6 +19,7 @@ // EDM include(s): #include "AthContainers/DataVector.h" #include "CxxUtils/no_sanitize_undefined.h" +#include "CxxUtils/checker_macros.h" // Local include(s): #include "xAODCore/tools/TDVCollectionProxy.h" @@ -416,7 +417,9 @@ namespace xAOD { // Find the underlying vector. const std::vector< char* >& vec = dv->stdcont(); // And store its address. - buff.fCont = const_cast< std::vector< char* >* >( &vec ); + std::vector<char*>* vptr ATLAS_THREAD_SAFE = + const_cast< std::vector< char* >* >( &vec ); + buff.fCont = vptr; return; } diff --git a/Event/xAOD/xAODCore/xAODCore/ATLAS_CHECK_THREAD_SAFETY b/Event/xAOD/xAODCore/xAODCore/ATLAS_CHECK_THREAD_SAFETY new file mode 100644 index 0000000000000000000000000000000000000000..5478f780847034afce3ea946b82802754d2baa85 --- /dev/null +++ b/Event/xAOD/xAODCore/xAODCore/ATLAS_CHECK_THREAD_SAFETY @@ -0,0 +1 @@ +Event/xAOD/xAODCore diff --git a/Event/xAOD/xAODCore/xAODCore/AuxContainerBase.h b/Event/xAOD/xAODCore/xAODCore/AuxContainerBase.h index ee546a68d731741a53152a1c8107fe43245e93da..6ed29e139859f8b831ff333259114a0967288cdf 100644 --- a/Event/xAOD/xAODCore/xAODCore/AuxContainerBase.h +++ b/Event/xAOD/xAODCore/xAODCore/AuxContainerBase.h @@ -1,7 +1,7 @@ // Dear emacs, this is -*- c++ -*- /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ // $Id: AuxContainerBase.h 793737 2017-01-24 20:11:10Z ssnyder $ @@ -65,7 +65,7 @@ namespace xAOD { /// Copy constructor AuxContainerBase( const AuxContainerBase& parent ); /// Constructor receiving a "dynamic auxiliary store" - AuxContainerBase( const SG::IAuxStore* store ); + AuxContainerBase( SG::IAuxStore* store ); /// Destructor ~AuxContainerBase(); @@ -193,7 +193,7 @@ namespace xAOD { /// Dynamic attributes selection implementation AuxSelection m_selection; /// Internal list of all available variables - mutable auxid_set_t m_auxids; + auxid_set_t m_auxids; /// Internal list of all managed variables std::vector< SG::IAuxTypeVector* > m_vecs; diff --git a/Event/xAOD/xAODCore/xAODCore/AuxInfoBase.h b/Event/xAOD/xAODCore/xAODCore/AuxInfoBase.h index de14da4549d074884c6a9bc640155a56c69f454b..1ffea6b8eedea356a59a3d3a6c7e1e51b102670c 100644 --- a/Event/xAOD/xAODCore/xAODCore/AuxInfoBase.h +++ b/Event/xAOD/xAODCore/xAODCore/AuxInfoBase.h @@ -1,7 +1,7 @@ // Dear emacs, this is -*- c++ -*- /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ // $Id: AuxInfoBase.h 793737 2017-01-24 20:11:10Z ssnyder $ @@ -62,7 +62,7 @@ namespace xAOD { /// Copy constructor AuxInfoBase( const AuxInfoBase& parent ); /// Constructor receiving a "dynamic auxiliary store" - AuxInfoBase( const SG::IAuxStore* store ); + AuxInfoBase( SG::IAuxStore* store ); /// Destructor ~AuxInfoBase(); @@ -179,7 +179,7 @@ namespace xAOD { /// Dynamic attributes selection implementation AuxSelection m_selection; /// Internal list of all available variables - mutable auxid_set_t m_auxids; + auxid_set_t m_auxids; /// Internal list of all managed variables std::vector< SG::IAuxTypeVector* > m_vecs; diff --git a/Event/xAOD/xAODCore/xAODCore/ShallowAuxContainer.h b/Event/xAOD/xAODCore/xAODCore/ShallowAuxContainer.h index 10f14bc1c3670646deb2750068d31efd40dea4b2..785feb3ea586eacb79d36a82a812fbc09d716d67 100644 --- a/Event/xAOD/xAODCore/xAODCore/ShallowAuxContainer.h +++ b/Event/xAOD/xAODCore/xAODCore/ShallowAuxContainer.h @@ -1,7 +1,7 @@ // Dear emacs, this is -*- c++ -*- /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ // $Id: ShallowAuxContainer.h 793737 2017-01-24 20:11:10Z ssnyder $ @@ -17,6 +17,7 @@ #ifndef XAOD_STANDALONE # include "AthenaKernel/ILockable.h" #endif // not XAOD_STANDALONE +#include "CxxUtils/checker_macros.h" // Local include(s): #include "xAODCore/AuxSelection.h" @@ -189,7 +190,7 @@ namespace xAOD { /// Link to the parent object DataLink< SG::IConstAuxStore > m_parentLink; /// Optional pointer to the IO interface of the parent object - mutable SG::IAuxStoreIO* m_parentIO; + const SG::IAuxStoreIO* m_parentIO; /// Flag for whether to do "shallow IO" or not bool m_shallowIO; @@ -198,8 +199,10 @@ namespace xAOD { typedef AthContainers_detail::lock_guard<mutex_t> guard_t; mutable mutex_t m_mutex; - mutable auxid_set_t m_auxids; - mutable bool m_auxidsValid; + // Mutable is thread-safe since we lock access, and the object + // is a ConcurrentBitset. + mutable auxid_set_t m_auxids ATLAS_THREAD_SAFE; + mutable bool m_auxidsValid ATLAS_THREAD_SAFE; /// Name of the container in memory. Set externally. std::string m_name; diff --git a/Event/xAOD/xAODCore/xAODCore/tools/PerfStats.h b/Event/xAOD/xAODCore/xAODCore/tools/PerfStats.h index 2aee4a340abe1db635105dcb317484ef091b47f0..010c83453a5a38d083efc50509032efc67711129 100644 --- a/Event/xAOD/xAODCore/xAODCore/tools/PerfStats.h +++ b/Event/xAOD/xAODCore/xAODCore/tools/PerfStats.h @@ -1,7 +1,7 @@ // Dear emacs, this is -*- c++ -*- /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration */ // $Id: PerfStats.h 634033 2014-12-05 14:46:38Z krasznaa $ @@ -11,6 +11,9 @@ // ROOT include(s): #include <TVirtualPerfStats.h> +#include "CxxUtils/checker_macros.h" +#include <mutex> + // Forward declaration(s): class TTree; @@ -125,7 +128,11 @@ namespace xAOD { private: /// The single instance of the object - static PerfStats* m_instance; + static PerfStats* s_instance ATLAS_THREAD_SAFE; + + /// Lock controlling access to the singleton. + static std::mutex s_mutex; + typedef std::lock_guard<std::mutex> lock_t; /// Another performance monitoring object ::TVirtualPerfStats* m_otherPerfStats;