diff --git a/Event/EventContainers/EventContainers/ATLAS_CHECK_THREAD_SAFETY b/Event/EventContainers/EventContainers/ATLAS_CHECK_THREAD_SAFETY new file mode 100644 index 0000000000000000000000000000000000000000..c286eecf711851e6c48b3024af07511433741bba --- /dev/null +++ b/Event/EventContainers/EventContainers/ATLAS_CHECK_THREAD_SAFETY @@ -0,0 +1 @@ +Event/EventContainers diff --git a/Event/EventContainers/EventContainers/IIdentifiableCont.h b/Event/EventContainers/EventContainers/IIdentifiableCont.h index 1d644f5e094998ad1e1752c351b173f47c978965..5a5ceef93f53393635a109829492f1e39243ca31 100644 --- a/Event/EventContainers/EventContainers/IIdentifiableCont.h +++ b/Event/EventContainers/EventContainers/IIdentifiableCont.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration */ @@ -18,6 +18,7 @@ public: virtual std::vector<IdentifierHash> GetAllCurrentHashes() const =0; virtual StatusCode addOrDelete(std::unique_ptr<T> ptr, IdentifierHash hashId) =0; + virtual StatusCode addOrDelete(std::unique_ptr<const T> ptr, IdentifierHash hashId) =0; virtual size_t fullSize() const =0; diff --git a/Event/EventContainers/EventContainers/IdentifiableContTemp.h b/Event/EventContainers/EventContainers/IdentifiableContTemp.h index 305bdb2e053348fd52a06ecdbcf29b744df05453..a8efd3605d7bf8ca71ae9e660d2db2c971bd7a99 100644 --- a/Event/EventContainers/EventContainers/IdentifiableContTemp.h +++ b/Event/EventContainers/EventContainers/IdentifiableContTemp.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2018 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration */ @@ -67,6 +67,15 @@ public: return StatusCode::SUCCESS; } + virtual StatusCode addOrDelete(std::unique_ptr<const T> ptr, IdentifierHash hashId) override{ + if(hashId >= m_randomcont.size()) return StatusCode::FAILURE; + if(m_randomcont[hashId] == nullptr){ + return addCollection(ptr.release(), hashId); + } + ptr.reset(); + return StatusCode::SUCCESS; + } + virtual size_t fullSize() const override{ return m_randomcont.size(); } @@ -86,7 +95,7 @@ public: return m_hasExternalCache; } - virtual StatusCode naughtyRetrieve(IdentifierHash hashId, T* &collToRetrieve) const override{ + virtual StatusCode naughtyRetrieve ATLAS_NOT_THREAD_SAFE (IdentifierHash hashId, T* &collToRetrieve) const override{ if(hashId >= m_randomcont.size()) return StatusCode::FAILURE; collToRetrieve = const_cast<T*>( m_randomcont[hashId]); return StatusCode::SUCCESS; @@ -94,7 +103,7 @@ public: StatusCode MergeToRealContainer(IIdentifiableCont<T> *real){ for(auto &x : m_usedhashes){ - auto ptr = std::unique_ptr<T>(const_cast<T*>( x.second)); + auto ptr = std::unique_ptr<const T>(x.second); auto sc = real->addOrDelete(std::move(ptr), x.first); if(sc.isFailure()) { return StatusCode::FAILURE; } m_randomcont[x.first] = nullptr; diff --git a/Event/EventContainers/EventContainers/IdentifiableContainerMT.h b/Event/EventContainers/EventContainers/IdentifiableContainerMT.h index 35174e751c92bd268998a66c3cc52a03b3bb99a1..b253004bbcfee565365c6cffee71baa7fa04e15e 100644 --- a/Event/EventContainers/EventContainers/IdentifiableContainerMT.h +++ b/Event/EventContainers/EventContainers/IdentifiableContainerMT.h @@ -168,6 +168,7 @@ public: /// Tries to add the item to the cache, if the item already exists then it is deleted /// This is a convenience method for online multithreaded scenarios virtual StatusCode addOrDelete(std::unique_ptr<T>, IdentifierHash hashId) override final; + virtual StatusCode addOrDelete(std::unique_ptr<const T>, IdentifierHash hashId) override final; ///identical to previous excepts allows counting of deletions StatusCode addOrDelete(std::unique_ptr<T>, IdentifierHash hashId, bool &deleted); @@ -186,7 +187,7 @@ public: StatusCode fetchOrCreate(const std::vector<IdentifierHash> &hashId); //This is a method to support bad behaviour in old code. Remove ASAP - virtual StatusCode naughtyRetrieve(IdentifierHash hashId, T* &collToRetrieve) const override final; + virtual StatusCode naughtyRetrieve ATLAS_NOT_THREAD_SAFE (IdentifierHash hashId, T* &collToRetrieve) const override final; #ifdef IdentifiableCacheBaseRemove @@ -354,6 +355,17 @@ IdentifiableContainerMT<T>::addOrDelete(std::unique_ptr<T> uptr, IdentifierHash return StatusCode::SUCCESS; } +template < class T> +StatusCode +IdentifiableContainerMT<T>::addOrDelete(std::unique_ptr<const T> uptr, IdentifierHash hashId) +{ + if(ATH_UNLIKELY(hashId >= m_link->fullSize())) return StatusCode::FAILURE; + auto ptr = uptr.release(); + bool b = IdentifiableContainerBase::insert(hashId, ptr); + if(!b) delete ptr; + return StatusCode::SUCCESS; +} + template < class T> StatusCode IdentifiableContainerMT<T>::addLock(std::unique_ptr<T> ptr, IdentifierHash hashId) diff --git a/Event/EventContainers/EventContainers/InternalOfflineFast.h b/Event/EventContainers/EventContainers/InternalOfflineFast.h index 16cf509c7300bc9e1ddb5b9425b9f7666b74796e..511519ee931e2e4ce5b3a6db4109d1f1b5c35296 100644 --- a/Event/EventContainers/EventContainers/InternalOfflineFast.h +++ b/Event/EventContainers/EventContainers/InternalOfflineFast.h @@ -39,7 +39,7 @@ public: virtual void* removeCollection( IdentifierHash hashId ) override; virtual void destructor(deleter_f*) noexcept override; private: - mutable std::vector<I_InternalIDC::hashPair> m_map; + mutable std::vector<I_InternalIDC::hashPair> m_map ATLAS_THREAD_SAFE; std::vector<const void*> m_fullMap; mutable std::mutex m_waitMutex ATLAS_THREAD_SAFE; mutable std::atomic<bool> m_needsupdate ATLAS_THREAD_SAFE; //These mutables are carefully thought out, do not change diff --git a/Event/EventContainers/EventContainers/InternalOfflineMap.h b/Event/EventContainers/EventContainers/InternalOfflineMap.h index dbd9b57180f08f80783eb6b7a168cd1fb6bb7edc..6ff20cbc72dd2eb1d0cbbcbe94e62924f7fc5c28 100644 --- a/Event/EventContainers/EventContainers/InternalOfflineMap.h +++ b/Event/EventContainers/EventContainers/InternalOfflineMap.h @@ -40,7 +40,7 @@ public: virtual void* removeCollection( IdentifierHash hashId ) override; virtual void destructor(deleter_f*) noexcept override; private: - mutable std::vector<I_InternalIDC::hashPair> m_map; + mutable std::vector<I_InternalIDC::hashPair> m_map ATLAS_THREAD_SAFE; std::unordered_map<IdentifierHash::value_type, const void*> m_fullMap; mutable std::mutex m_waitMutex ATLAS_THREAD_SAFE; mutable std::atomic<bool> m_needsupdate ATLAS_THREAD_SAFE; //These mutables are carefully thought out, do not change diff --git a/Event/EventContainers/src/InternalOfflineFast.cxx b/Event/EventContainers/src/InternalOfflineFast.cxx index ff34cefbfca8ad76743bcf7a024ed3caa263fb00..e3ddab2e9513cc6d15d138063a30867998eaa8f9 100644 --- a/Event/EventContainers/src/InternalOfflineFast.cxx +++ b/Event/EventContainers/src/InternalOfflineFast.cxx @@ -105,7 +105,7 @@ StatusCode InternalOfflineFast::addLock(IdentifierHash hashId, const void* ptr) } void* InternalOfflineFast::removeCollection( IdentifierHash hashId ) { - void* ptr = const_cast< void* > (m_fullMap[hashId]); + void* ptr ATLAS_THREAD_SAFE = const_cast< void* > (m_fullMap[hashId]); m_fullMap[hashId] = nullptr; m_needsupdate.store(true, std::memory_order_relaxed); return ptr; diff --git a/Event/EventContainers/test/IDC_Realistic_Test.cxx b/Event/EventContainers/test/IDC_Realistic_Test.cxx index c4ca7f963af1c6283fdcd3ec97fd75e6142075fc..4d314ac2062dd7760e34256204b8cdd704c72a22 100644 --- a/Event/EventContainers/test/IDC_Realistic_Test.cxx +++ b/Event/EventContainers/test/IDC_Realistic_Test.cxx @@ -16,7 +16,6 @@ namespace IDC_TEST class MyCollection; } -std::vector<IDC_TEST::MyCollection*> initialdata; namespace IDC_TEST { constexpr int nthreads=10; @@ -25,7 +24,6 @@ constexpr int Ncontainers = 5000; constexpr int Nevents = 50; std::mutex abortedlock; -std::set<size_t> abortedhashes; class MyID @@ -169,12 +167,14 @@ public: size_t RoIEnd; int threads; counters c; + const std::set<size_t>& m_abortedhashes; + const std::vector<IDC_TEST::MyCollection*>& m_initialdata; void Check(MyCollectionContainer &container) { //Collections filled c.aborted=0; - for(auto x : abortedhashes) { + for(auto x : m_abortedhashes) { c.aborted+= x >=RoIStart && x<RoIEnd; } @@ -194,7 +194,7 @@ public: auto p = container.indexFindPtr(x); int j =0; for(auto q : *p) { - if(q->val() != (initialdata[x.value()]->at(j++))->val()) wrong++; + if(q->val() != (m_initialdata[x.value()]->at(j++))->val()) wrong++; } if(j!=ndigits) { std::cout << "n digits wrong"<<std::endl; @@ -207,7 +207,7 @@ public: for(auto p : container){ int j =0; for(auto q : *p) { - if(q->val() != (initialdata[hashes[orig]]->at(j++))->val()) wrong++; + if(q->val() != (m_initialdata[hashes[orig]]->at(j++))->val()) wrong++; } if(j!=ndigits) { std::cout << "n digits wrong"<<std::endl; @@ -225,7 +225,7 @@ public: std::abort(); } for(auto q : *ptr) { - if(q->val() != (initialdata[hashes[orig2]]->at(j++))->val()) { + if(q->val() != (m_initialdata[hashes[orig2]]->at(j++))->val()) { std::cout << "directaccess broke " << std::endl; std::abort(); } @@ -245,8 +245,12 @@ public: ExecuteFill(container); Check(container); } - PseudoView(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i) : IDC(inIDC), RoIStart(s), RoIEnd(r), - threads(i), c() {} + PseudoView(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i, + const std::set<size_t>& abortedhashes, + const std::vector<IDC_TEST::MyCollection*>& initialdata) : IDC(inIDC), RoIStart(s), RoIEnd(r), + threads(i), c(), + m_abortedhashes (abortedhashes), + m_initialdata (initialdata) {} virtual ~PseudoView() = default; }; @@ -254,7 +258,10 @@ public: class PseudoViewNoLock : public PseudoView { public: - PseudoViewNoLock(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i) : PseudoView(s, r, inIDC, i ) { } + PseudoViewNoLock(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i, + const std::set<size_t>& abortedhashes, + const std::vector<IDC_TEST::MyCollection*>& initialdata) + : PseudoView(s, r, inIDC, i, abortedhashes, initialdata ) { } virtual void ExecuteFill(MyCollectionContainer &container) override { @@ -271,7 +278,7 @@ public: for(int j=0; j<ndigits; j++) { dcoll->add(new MyDigit(dis(gen))); } - if(abortedhashes.count(i)) { //testing aborting collections + if(m_abortedhashes.count(i)) { //testing aborting collections continue; } // std::this_thread::sleep_for(0.005s); @@ -290,7 +297,10 @@ public: class PseudoViewLock : public PseudoView { public: - PseudoViewLock(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i) : PseudoView(s, r, inIDC, i ) { } + PseudoViewLock(int s, int r, EventContainers::IdentifiableCache<MyCollection>* inIDC, int i, + const std::set<size_t>& abortedhashes, + const std::vector<IDC_TEST::MyCollection*>& initialdata) + : PseudoView(s, r, inIDC, i, abortedhashes, initialdata ) { } virtual void ExecuteFill(MyCollectionContainer &container) override { @@ -307,7 +317,7 @@ public: for(int j=0; j<ndigits; j++) { dcoll->add(new MyDigit(dis(gen))); } - if(abortedhashes.count(i)) { //testing aborting collections + if(m_abortedhashes.count(i)) { //testing aborting collections continue; } bool deleted = false; @@ -330,11 +340,12 @@ public: std::vector< T > m_views; EventContainers::IdentifiableCache<MyCollection> *IDCache; - void Execute() { + void Execute(const std::set<size_t>& abortedhashes, + const std::vector<IDC_TEST::MyCollection*>& initialdata) { int x = 0; IDCache = new EventContainers::IdentifiableCache<MyCollection>(IdentifierHash(Ncontainers), nullptr); for(int i=0; i<nthreads; i++) { - m_views.emplace_back(x, x+1000,IDCache, i); + m_views.emplace_back(x, x+1000,IDCache, i, abortedhashes, initialdata); x+=50; } std::vector<std::thread> threads; @@ -373,10 +384,13 @@ int main() { std::mt19937 genabort(0); std::uniform_int_distribution<> abort(0, highestvalue); + std::set<size_t> abortedhashes; for(int i =0; i<20; i++) { abortedhashes.insert(abort(genabort)); } + std::vector<IDC_TEST::MyCollection*> initialdata; + MyDigit::s_total=0; for(int i =0; i<Ncontainers; i++) { std::mt19937 gen(i); //Standard mersenne_twister_engine seeded with rd() @@ -396,7 +410,7 @@ int main() { for(int i =0; i<Nevents; i++) { if(i%10==0) std::cout << i << "/" << Nevents << std::endl; PseudoEvent<PseudoViewNoLock> event; - event.Execute(); + event.Execute(abortedhashes, initialdata); c.Add(event.getcounters()); } std::cout << "NoLock\n"; @@ -413,7 +427,7 @@ int main() { for(int i =0; i<Nevents; i++) { if(i%10==0) std::cout << i << "/" << Nevents << std::endl; PseudoEvent<PseudoViewLock> event; - event.Execute(); + event.Execute(abortedhashes, initialdata); c.Add(event.getcounters()); } std::cout << "Lock\n"; diff --git a/Event/EventContainers/test/IDMT_ContainerTest.cxx b/Event/EventContainers/test/IDMT_ContainerTest.cxx index 82038c470bbfca42e753476abd8a4d007e75d74c..a9a5fc1ff69656561f68066fdd6aaa1906095a19 100644 --- a/Event/EventContainers/test/IDMT_ContainerTest.cxx +++ b/Event/EventContainers/test/IDMT_ContainerTest.cxx @@ -11,10 +11,11 @@ #include "GaudiKernel/System.h" #include "AthenaKernel/CLASS_DEF.h" +#include <atomic> + // define a bunch of fake data classes using namespace std; -static EventContainers::Mode s_mode; namespace IDC_TEST { @@ -36,11 +37,11 @@ namespace IDC_TEST MyDigit(float d) :m_digit(d) {s_total++;} float val() const { return m_digit ;} ~MyDigit(){ s_total--; } - static int s_total; + static std::atomic<int> s_total; private: float m_digit; }; - int MyDigit::s_total = 0; + std::atomic<int> MyDigit::s_total = 0; static const CLID CLID_MYCOLLECTION=10000; class MyCollection @@ -134,11 +135,11 @@ ID_ContainerTest::ID_ContainerTest() } // ------ initialize() -int ID_ContainerTest::initialize() +int ID_ContainerTest::initialize(EventContainers::Mode mode) { // we own the Container - m_container = new MyCollectionContainer(m_ncollections, s_mode); + m_container = new MyCollectionContainer(m_ncollections, mode); std::cout <<" Collection, Skip = " << m_ncollections<<" "<<m_nskip<<std::endl; std::cout <<" Test level = " << m_test<<std::endl; @@ -155,7 +156,7 @@ return 0;} //------ execute() -int ID_ContainerTest::execute(){ +int ID_ContainerTest::execute(EventContainers::Mode mode){ typedef SelectAllObject<MyCollectionContainer,MyDigit> SELECTOR ; typedef SELECTOR::const_iterator digit_const_iterator; @@ -181,15 +182,8 @@ int ID_ContainerTest::execute(){ std::string key("MyDIgitCont"); - static bool first=true; - int skip= m_nskip; - if(first) { - first = false; - // skip = 0 ; - } - std::vector< MyCollection* > vColl; std::vector< const MyCollection* > vCollRem; @@ -504,7 +498,7 @@ int ID_ContainerTest::execute(){ { - auto container2 = new MyCollectionContainer(m_ncollections, s_mode); + auto container2 = new MyCollectionContainer(m_ncollections, mode); int itemsadded=0; for (int coll =0; coll <hfmax; coll=coll+(1+skip) ){ MyID id(coll); @@ -538,7 +532,7 @@ int ID_ContainerTest::execute(){ } delete container2; container2 = nullptr; //Test Empty - MyCollectionContainer* dempty = new MyCollectionContainer(100, s_mode); + MyCollectionContainer* dempty = new MyCollectionContainer(100, mode); if(dempty->begin() != dempty->end()){ std::cout << __FILE__ << " empty container not working see LINE " << __LINE__ << std::endl; std::abort(); } @@ -549,7 +543,7 @@ int ID_ContainerTest::execute(){ delete dempty; dempty = nullptr; //Test Online IDC - static const DefaultMaker* maker= new DefaultMaker(); + static const DefaultMaker* const maker= new DefaultMaker(); auto cache = new EventContainers::IdentifiableCache<MyCollection>(IdentifierHash(100), maker); cache->add(IdentifierHash(0), new MyCollection(MyID(0))); cache->add(IdentifierHash(3), new MyCollection(MyID(3)));//Some pre cached collections @@ -618,7 +612,7 @@ int ID_ContainerTest::execute(){ delete cache; - auto containerordertest = new MyCollectionContainer(500, s_mode); + auto containerordertest = new MyCollectionContainer(500, mode); containerordertest->addCollection(new MyCollection, 4).ignore(); containerordertest->addCollection(new MyCollection, 3).ignore(); containerordertest->addCollection(new MyCollection, 2).ignore(); @@ -664,10 +658,9 @@ int main (int /*argc*/, char** /*argv[]*/) ID_ContainerTest test; for(auto x : { EventContainers::Mode::OfflineLowMemory, EventContainers::Mode::OfflineFast, EventContainers::Mode::OfflineMap }){ - s_mode = x; - std::cout <<" container mode " << static_cast<int>(s_mode) << std::endl; - test.initialize(); - for (unsigned int i = 0; i < 5; i++) test.execute(); + std::cout <<" container mode " << static_cast<int>(x) << std::endl; + test.initialize(x); + for (unsigned int i = 0; i < 5; i++) test.execute(x); test.finalize(); } EventContainers::IdentifiableContTemp<MyCollection> emptyContContainer(10); //Put here to test compilation of IdentifiableContTemp diff --git a/Event/EventContainers/test/ID_ContainerTest.cxx b/Event/EventContainers/test/ID_ContainerTest.cxx index 8cdc688d646cbdc6b983717651c7ff16c2852277..86204e92b04ee651ebd4025ed320b16d1eaa0ee2 100644 --- a/Event/EventContainers/test/ID_ContainerTest.cxx +++ b/Event/EventContainers/test/ID_ContainerTest.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration */ // This is a test cxx file for IdentifiableContainer. @@ -112,7 +112,7 @@ ID_ContainerTest::ID_ContainerTest() } // ------ initialize() -int ID_ContainerTest::initialize() +int ID_ContainerTest::initialize(EventContainers::Mode) { // we own the Container @@ -133,7 +133,7 @@ return 0;} //------ execute() -int ID_ContainerTest::execute(){ +int ID_ContainerTest::execute(EventContainers::Mode){ typedef SelectAllObject<MyCollectionContainer,MyDigit> SELECTOR ; typedef SELECTOR::const_iterator digit_const_iterator; @@ -159,15 +159,8 @@ int ID_ContainerTest::execute(){ std::string key("MyDIgitCont"); - static bool first=true; - int skip= m_nskip; - if(first) { - first = false; - // skip = 0 ; - } - std::vector< MyCollection* > vColl; std::vector< const MyCollection* > vCollRem; @@ -422,8 +415,8 @@ int main (int /*argc*/, char** /*argv[]*/) { ID_ContainerTest test; - test.initialize(); - for (unsigned int i = 0; i < 5; i++) test.execute(); + test.initialize(EventContainers::Mode::OfflineFast); + for (unsigned int i = 0; i < 5; i++) test.execute(EventContainers::Mode::OfflineFast); test.finalize(); } diff --git a/Event/EventContainers/test/ID_ContainerTest.h b/Event/EventContainers/test/ID_ContainerTest.h index 6bab14949a0ed8f4a5d3ee9f151cefc1ec9cca7f..49550c16aa9be7ca0b7252821029c9fdd9688f09 100755 --- a/Event/EventContainers/test/ID_ContainerTest.h +++ b/Event/EventContainers/test/ID_ContainerTest.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration */ // @@ -15,10 +15,10 @@ class ID_ContainerTest { public: ID_ContainerTest(); - virtual int initialize() ; + virtual int initialize(EventContainers::Mode mode) ; virtual int finalize() ; - virtual int execute(); + virtual int execute(EventContainers::Mode mode); private: int m_ncollections;