From a10500b608fada11bfa3c7e7fc689552bca0a961 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 2 Oct 2013 18:13:21 -0500 Subject: [PATCH 01/12] The variable remainingEvents_ may be read by multiple threads simultanously so must be atomic --- FWCore/Framework/interface/OutputModule.h | 3 ++- FWCore/Framework/interface/one/OutputModuleBase.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/FWCore/Framework/interface/OutputModule.h b/FWCore/Framework/interface/OutputModule.h index 3acd20bf01a44..0af2e3a9ac691 100644 --- a/FWCore/Framework/interface/OutputModule.h +++ b/FWCore/Framework/interface/OutputModule.h @@ -27,6 +27,7 @@ output stream. #include #include #include +#include namespace edm { @@ -119,7 +120,7 @@ namespace edm { private: int maxEvents_; - int remainingEvents_; + std::atomic remainingEvents_; // TODO: Give OutputModule // an interface (protected?) that supplies client code with the diff --git a/FWCore/Framework/interface/one/OutputModuleBase.h b/FWCore/Framework/interface/one/OutputModuleBase.h index 47d5d944e172d..d830b86c535f3 100644 --- a/FWCore/Framework/interface/one/OutputModuleBase.h +++ b/FWCore/Framework/interface/one/OutputModuleBase.h @@ -23,6 +23,7 @@ #include #include #include +#include // user include files @@ -133,7 +134,7 @@ namespace edm { private: int maxEvents_; - int remainingEvents_; + std::atomic remainingEvents_; // TODO: Give OutputModule // an interface (protected?) that supplies client code with the From 89a6f14b70dc63c240d83daa77307585602613f7 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 3 Oct 2013 09:39:45 -0500 Subject: [PATCH 02/12] First version to actually process multiple events concurrently This version uses a TBB task per Stream to process Events with a mutex used to protect the Source in a critical section. This is sufficient to test the thread safety of the framework internals. --- FWCore/Framework/interface/EventProcessor.h | 7 ++ FWCore/Framework/src/EventProcessor.cc | 128 +++++++++++++++++++- 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/FWCore/Framework/interface/EventProcessor.h b/FWCore/Framework/interface/EventProcessor.h index 7fabd8dbada79..75d506bbc44d9 100644 --- a/FWCore/Framework/interface/EventProcessor.h +++ b/FWCore/Framework/interface/EventProcessor.h @@ -34,6 +34,7 @@ configured in the user's main() function, and is set running. #include #include #include +#include namespace statemachine { class Machine; @@ -230,6 +231,11 @@ namespace edm { void possiblyContinueAfterForkChildFailure(); + friend class StreamProcessingTask; + void processEventsForStreamAsync(unsigned int iStreamIndex, + std::atomic* finishedProcessingEvents); + + //read the next event using Stream iStreamIndex void readEvent(unsigned int iStreamIndex); @@ -262,6 +268,7 @@ namespace edm { std::unique_ptr fb_; boost::shared_ptr looper_; + std::mutex nextTransitionMutex_; PrincipalCache principalCache_; bool beginJobCalled_; bool shouldWeStop_; diff --git a/FWCore/Framework/src/EventProcessor.cc b/FWCore/Framework/src/EventProcessor.cc index a91820ac66b44..490ce1afa68bf 100644 --- a/FWCore/Framework/src/EventProcessor.cc +++ b/FWCore/Framework/src/EventProcessor.cc @@ -71,6 +71,8 @@ #include #include +#include "tbb/task.h" + //Used for forking #include #include @@ -1245,7 +1247,7 @@ namespace edm { bool returnValue = false; // Look for a shutdown signal - if(shutdown_flag.load(std::memory_order_relaxed)) { + if(shutdown_flag.load(std::memory_order_acquire)) { returnValue = true; returnCode = epSignal; } @@ -1773,34 +1775,148 @@ namespace edm { FDEBUG(1) << "\tdeleteLumiFromCache " << run << "/" << lumi << "\n"; } + class StreamProcessingTask : public tbb::task { + public: + StreamProcessingTask(EventProcessor* iProc, + unsigned int iStreamIndex, + std::atomic* iFinishedProcessingEvents, + tbb::task* iWaitTask): + m_proc(iProc), + m_streamID(iStreamIndex), + m_finishedProcessingEvents(iFinishedProcessingEvents), + m_waitTask(iWaitTask){} + + tbb::task* execute() { + m_proc->processEventsForStreamAsync(m_streamID,m_finishedProcessingEvents); + m_waitTask->decrement_ref_count(); + return nullptr; + } + private: + EventProcessor* m_proc; + unsigned int m_streamID; + std::atomic* m_finishedProcessingEvents; + tbb::task* m_waitTask; + }; + + void EventProcessor::processEventsForStreamAsync(unsigned int iStreamIndex, + std::atomic* finishedProcessingEvents) { + try { + // make the services available + ServiceRegistry::Operate operate(serviceToken_); + + if(iStreamIndex==0) { + processEvent(0); + } + do { + if(shouldWeStop()) { + break; + } + { + { + //nextItemType and readEvent need to be in same critical section + std::lock_guard sourceGuard(nextTransitionMutex_); + + if(finishedProcessingEvents->load(std::memory_order_acquire)) { + std::cerr<<"finishedProcessingEvents\n"; + break; + } + InputSource::ItemType itemType = input_->nextItemType(); + if (InputSource::IsEvent !=itemType) { + nextItemTypeFromProcessingEvents_ = itemType; + finishedProcessingEvents->store(true,std::memory_order_release); + std::cerr<<"next item type "<store(true,std::memory_order_release); + std::cerr<<"task caught edm::Exception\n"<store(true,std::memory_order_release); + std::cerr<<"task caught exception\n"; + } + } + void EventProcessor::readAndProcessEvent() { if(numberOfForkedChildren_>0) { readEvent(0); processEvent(0); return; } - InputSource::ItemType itemType = InputSource::IsEvent; + nextItemTypeFromProcessingEvents_ = InputSource::IsEvent; //needed for looper + asyncStopRequestedWhileProcessingEvents_ = false; + + std::atomic finishedProcessingEvents{false}; + + //Task assumes Stream 0 has already read the event that caused us to go here + readEvent(0); + + //To wait, the ref count has to b 1+#streams + tbb::task* eventLoopWaitTask{new (tbb::task::allocate_root()) tbb::empty_task{}}; + eventLoopWaitTask->increment_ref_count(); + + const unsigned int kNumStreams = preallocations_.numberOfStreams(); + unsigned int iStreamIndex = 0; + for(; iStreamIndexincrement_ref_count(); + tbb::task::enqueue( *(new (tbb::task::allocate_root()) StreamProcessingTask{this,iStreamIndex, &finishedProcessingEvents, eventLoopWaitTask})); + } + eventLoopWaitTask->increment_ref_count(); + eventLoopWaitTask->spawn_and_wait_for_all(*(new (tbb::task::allocate_root()) StreamProcessingTask{this,iStreamIndex,&finishedProcessingEvents,eventLoopWaitTask})); + tbb::task::destroy(*eventLoopWaitTask); + + /* + InputSource::ItemType itemType = InputSource::IsEvent; + nextItemTypeFromProcessingEvents_ = itemType; //needed for looper //While all the following item types are isEvent, process them right here asyncStopRequestedWhileProcessingEvents_ = false; //We will round-robin which stream to use unsigned int nextStreamIndex=0; const unsigned int kNumStreams = preallocations_.numberOfStreams(); + + //we know there is something to do + readEvent(nextStreamIndex); + processEvent(nextStreamIndex); + bool finishedProcessingEvents = false; do { - readEvent(nextStreamIndex); - processEvent(nextStreamIndex); nextStreamIndex = (nextStreamIndex+1) % kNumStreams; if(shouldWeStop()) { break; } - itemType = input_->nextItemType(); + { + std::lock_guard sourceGuard(nextTransitionMutex_); + if(not finishedProcessingEvents) { + itemType = input_->nextItemType(); + if (InputSource::IsEvent !=itemType) { + finishedProcessingEvents = true; + nextItemTypeFromProcessingEvents_ = itemType; + break; + } + } else { + break; + } + } if((asyncStopRequestedWhileProcessingEvents_=checkForAsyncStopRequest(asyncStopStatusCodeFromProcessingEvents_))) { break; } + readEvent(nextStreamIndex); + processEvent(nextStreamIndex); } while (itemType == InputSource::IsEvent); - nextItemTypeFromProcessingEvents_ = itemType; + */ } void EventProcessor::readEvent(unsigned int iStreamIndex) { //TODO this will have to become per stream From 8c03a48f882973c4b90106449b17ee77d87e13fe Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Fri, 4 Oct 2013 15:19:58 -0500 Subject: [PATCH 03/12] Propagate exceptions thrown during processing to the main thread The EventProcessor now uses the new C++11 std::exception_ptr, std::current_exception() and std:rethrow_exception() to capture a thrown exception in a thread processing an event and then transfer that exception to the main thread to be rethrown. We use an std::atomic to provide thread safe update of the std::exception_ptr member data in the case of multiple processing threads having an exception. In such a case, only one of the exceptions will be kept. --- FWCore/Framework/interface/EventProcessor.h | 5 + FWCore/Framework/src/EventProcessor.cc | 107 ++++++++------------ 2 files changed, 45 insertions(+), 67 deletions(-) diff --git a/FWCore/Framework/interface/EventProcessor.h b/FWCore/Framework/interface/EventProcessor.h index 75d506bbc44d9..d8f406f107619 100644 --- a/FWCore/Framework/interface/EventProcessor.h +++ b/FWCore/Framework/interface/EventProcessor.h @@ -35,6 +35,7 @@ configured in the user's main() function, and is set running. #include #include #include +#include namespace statemachine { class Machine; @@ -268,6 +269,10 @@ namespace edm { std::unique_ptr fb_; boost::shared_ptr looper_; + //The atomic protects concurrent access of deferredExceptionPtr_ + std::atomic deferredExceptionPtrIsSet_; + std::exception_ptr deferredExceptionPtr_; + std::mutex nextTransitionMutex_; PrincipalCache principalCache_; bool beginJobCalled_; diff --git a/FWCore/Framework/src/EventProcessor.cc b/FWCore/Framework/src/EventProcessor.cc index 490ce1afa68bf..7f0d3b2a5de7e 100644 --- a/FWCore/Framework/src/EventProcessor.cc +++ b/FWCore/Framework/src/EventProcessor.cc @@ -220,6 +220,7 @@ namespace edm { historyAppender_(new HistoryAppender), fb_(), looper_(), + deferredExceptionPtrIsSet_(false), principalCache_(), beginJobCalled_(false), shouldWeStop_(false), @@ -260,6 +261,7 @@ namespace edm { historyAppender_(new HistoryAppender), fb_(), looper_(), + deferredExceptionPtrIsSet_(false), principalCache_(), beginJobCalled_(false), shouldWeStop_(false), @@ -303,6 +305,7 @@ namespace edm { historyAppender_(new HistoryAppender), fb_(), looper_(), + deferredExceptionPtrIsSet_(false), principalCache_(), beginJobCalled_(false), shouldWeStop_(false), @@ -342,6 +345,7 @@ namespace edm { historyAppender_(new HistoryAppender), fb_(), looper_(), + deferredExceptionPtrIsSet_(false), principalCache_(), beginJobCalled_(false), shouldWeStop_(false), @@ -1811,39 +1815,44 @@ namespace edm { if(shouldWeStop()) { break; } + if(deferredExceptionPtrIsSet_.load(std::memory_order_acquire)) { + //another thread hit an exception + std::cerr<<"another thread saw an exception\n"; + break; + } { - { - //nextItemType and readEvent need to be in same critical section - std::lock_guard sourceGuard(nextTransitionMutex_); - - if(finishedProcessingEvents->load(std::memory_order_acquire)) { - std::cerr<<"finishedProcessingEvents\n"; - break; - } - InputSource::ItemType itemType = input_->nextItemType(); - if (InputSource::IsEvent !=itemType) { - nextItemTypeFromProcessingEvents_ = itemType; - finishedProcessingEvents->store(true,std::memory_order_release); - std::cerr<<"next item type "< sourceGuard(nextTransitionMutex_); + + if(finishedProcessingEvents->load(std::memory_order_acquire)) { + std::cerr<<"finishedProcessingEvents\n"; + break; + } + InputSource::ItemType itemType = input_->nextItemType(); + if (InputSource::IsEvent !=itemType) { + nextItemTypeFromProcessingEvents_ = itemType; + finishedProcessingEvents->store(true,std::memory_order_release); + std::cerr<<"next item type "<store(true,std::memory_order_release); - std::cerr<<"task caught edm::Exception\n"<store(true,std::memory_order_release); + bool expected =false; + if(deferredExceptionPtrIsSet_.compare_exchange_strong(expected,true)) { + deferredExceptionPtr_ = std::current_exception(); + } std::cerr<<"task caught exception\n"; } } @@ -1877,46 +1886,10 @@ namespace edm { eventLoopWaitTask->spawn_and_wait_for_all(*(new (tbb::task::allocate_root()) StreamProcessingTask{this,iStreamIndex,&finishedProcessingEvents,eventLoopWaitTask})); tbb::task::destroy(*eventLoopWaitTask); - /* - InputSource::ItemType itemType = InputSource::IsEvent; - nextItemTypeFromProcessingEvents_ = itemType; //needed for looper - //While all the following item types are isEvent, process them right here - asyncStopRequestedWhileProcessingEvents_ = false; - - //We will round-robin which stream to use - unsigned int nextStreamIndex=0; - const unsigned int kNumStreams = preallocations_.numberOfStreams(); - - //we know there is something to do - readEvent(nextStreamIndex); - processEvent(nextStreamIndex); - bool finishedProcessingEvents = false; - do { - nextStreamIndex = (nextStreamIndex+1) % kNumStreams; - - if(shouldWeStop()) { - break; - } - { - std::lock_guard sourceGuard(nextTransitionMutex_); - if(not finishedProcessingEvents) { - itemType = input_->nextItemType(); - if (InputSource::IsEvent !=itemType) { - finishedProcessingEvents = true; - nextItemTypeFromProcessingEvents_ = itemType; - break; - } - } else { - break; - } - } - if((asyncStopRequestedWhileProcessingEvents_=checkForAsyncStopRequest(asyncStopStatusCodeFromProcessingEvents_))) { - break; - } - readEvent(nextStreamIndex); - processEvent(nextStreamIndex); - } while (itemType == InputSource::IsEvent); - */ + //One of the processing threads saw an exception + if(deferredExceptionPtrIsSet_) { + std::rethrow_exception(deferredExceptionPtr_); + } } void EventProcessor::readEvent(unsigned int iStreamIndex) { //TODO this will have to become per stream From 9a8d8d94300e53c45f34ab1568065c6181691f96 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 7 Oct 2013 10:23:18 -0500 Subject: [PATCH 04/12] Added classes to managed resources shared by multiple modules The SharedResourcesRegistry is the centralized place where resources are registered and where the framework can get SharedResourcesAcquirer instances. These two classes work together to allow safe sharing of non-thread safe resources between modules. A unit test is included to test the two classes. --- .../Framework/src/SharedResourcesAcquirer.cc | 32 +++++ .../Framework/src/SharedResourcesAcquirer.h | 69 +++++++++++ .../Framework/src/SharedResourcesRegistry.cc | 57 +++++++++ .../Framework/src/SharedResourcesRegistry.h | 70 +++++++++++ FWCore/Framework/test/BuildFile.xml | 2 +- .../test/sharedresourcesregistry_t.cppunit.cc | 114 ++++++++++++++++++ 6 files changed, 343 insertions(+), 1 deletion(-) create mode 100644 FWCore/Framework/src/SharedResourcesAcquirer.cc create mode 100644 FWCore/Framework/src/SharedResourcesAcquirer.h create mode 100644 FWCore/Framework/src/SharedResourcesRegistry.cc create mode 100644 FWCore/Framework/src/SharedResourcesRegistry.h create mode 100644 FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc diff --git a/FWCore/Framework/src/SharedResourcesAcquirer.cc b/FWCore/Framework/src/SharedResourcesAcquirer.cc new file mode 100644 index 0000000000000..f0590fe5deb08 --- /dev/null +++ b/FWCore/Framework/src/SharedResourcesAcquirer.cc @@ -0,0 +1,32 @@ +// -*- C++ -*- +// +// Package: Subsystem/Package +// Class : SharedResourcesAcquirer +// +// Implementation: +// [Notes on implementation] +// +// Original Author: Chris Jones +// Created: Sun, 06 Oct 2013 19:43:28 GMT +// + +// system include files + +// user include files +#include "SharedResourcesAcquirer.h" + + +namespace edm { + void SharedResourcesAcquirer::lock() { + for(auto m : m_resources) { + m->lock(); + } + } + + void SharedResourcesAcquirer::unlock() { + for(auto it = m_resources.rbegin(), itEnd = m_resources.rend(); + it != itEnd; ++it) { + (*it)->unlock(); + } + } +} \ No newline at end of file diff --git a/FWCore/Framework/src/SharedResourcesAcquirer.h b/FWCore/Framework/src/SharedResourcesAcquirer.h new file mode 100644 index 0000000000000..027ad28e719a0 --- /dev/null +++ b/FWCore/Framework/src/SharedResourcesAcquirer.h @@ -0,0 +1,69 @@ +#ifndef Subsystem_Package_SharedResourcesAcquirer_h +#define Subsystem_Package_SharedResourcesAcquirer_h +// -*- C++ -*- +// +// Package: Subsystem/Package +// Class : SharedResourcesAcquirer +// +/**\class SharedResourcesAcquirer SharedResourcesAcquirer.h "SharedResourcesAcquirer.h" + + Description: Handles acquiring and releasing a group of resources shared between modules + + Usage: + + +*/ +// +// Original Author: Chris Jones +// Created: Sun, 06 Oct 2013 19:43:26 GMT +// + +// system include files + +// user include files +#include +#include +#include + +// forward declarations +class testSharedResourcesRegistry; +namespace edm { + class SharedResourcesAcquirer + { + public: + friend class ::testSharedResourcesRegistry; + + SharedResourcesAcquirer() = default; + explicit SharedResourcesAcquirer(std::vector&& iResources): + m_resources(iResources){} + + SharedResourcesAcquirer(SharedResourcesAcquirer&&) = default; + SharedResourcesAcquirer(const SharedResourcesAcquirer&) = default; + SharedResourcesAcquirer& operator=(const SharedResourcesAcquirer&) = default; + + ~SharedResourcesAcquirer() = default; + + // ---------- member functions --------------------------- + void lock(); + void unlock(); + + ///Used by the framework to temporarily unlock a resource in the case where a module is temporarily suspended, + /// e.g. when a Event::getByLabel call launches a Producer via unscheduled processing + template + void temporaryUnlock(FUNC iFunc) { + std::shared_ptr guard(this,[](SharedResourcesAcquirer* iThis) {iThis->lock();}); + this->unlock(); + iFunc(); + } + + ///The number returned may be less than the number of resources requested if a resource is only used by one module and therefore is not being shared. + size_t numberOfResources() const { return m_resources.size();} + private: + + // ---------- member data -------------------------------- + std::vector m_resources; + }; +} + + +#endif diff --git a/FWCore/Framework/src/SharedResourcesRegistry.cc b/FWCore/Framework/src/SharedResourcesRegistry.cc new file mode 100644 index 0000000000000..0eaf2d795ef44 --- /dev/null +++ b/FWCore/Framework/src/SharedResourcesRegistry.cc @@ -0,0 +1,57 @@ +// -*- C++ -*- +// +// Package: FWCore/Framework +// Class : SharedResourcesRegistry +// +// Implementation: +// [Notes on implementation] +// +// Original Author: Chris Jones +// Created: Sun, 06 Oct 2013 15:48:50 GMT +// + +// system include files +#include + +// user include files +#include "SharedResourcesRegistry.h" +#include "SharedResourcesAcquirer.h" + +namespace edm { + + const std::string SharedResourcesRegistry::kLegacyModuleResourceName{"__legacy__"}; + + void + SharedResourcesRegistry::registerSharedResource(const std::string& iString){ + auto& values = resourceMap_[iString]; + + if(values.second ==1) { + //only need to make the resource if more than 1 module wants it + values.first = std::shared_ptr( new std::mutex ); + } + ++(values.second); + } + + SharedResourcesAcquirer + SharedResourcesRegistry::createAcquirer(std::vector const & iNames) const { + //Sort by how often used and then by name + std::map, std::mutex*> sortedResources; + + for(auto const& name: iNames) { + auto itFound = resourceMap_.find(name); + assert(itFound != resourceMap_.end()); + //If only one module wants it, it really isn't shared + if(itFound->second.second>1) { + sortedResources.insert(std::make_pair(std::make_pair(itFound->second.second, itFound->first),itFound->second.first.get())); + } + } + std::vector mutexes; + mutexes.reserve(sortedResources.size()); + for(auto const& resource: sortedResources) { + mutexes.push_back(resource.second); + } + + return SharedResourcesAcquirer(std::move(mutexes)); + } + +} diff --git a/FWCore/Framework/src/SharedResourcesRegistry.h b/FWCore/Framework/src/SharedResourcesRegistry.h new file mode 100644 index 0000000000000..4d353b8b6f526 --- /dev/null +++ b/FWCore/Framework/src/SharedResourcesRegistry.h @@ -0,0 +1,70 @@ +#ifndef FWCore_Framework_SharedResourcesRegistry_h +#define FWCore_Framework_SharedResourcesRegistry_h +// -*- C++ -*- +// +// Package: FWCore/Framework +// Class : SharedResourcesRegistry +// +/**\class SharedResourcesRegistry SharedResourcesRegistry.h "SharedResourcesRegistry.h" + + Description: Manages the Acquirers used to take temporary control of a resource shared between modules + + Usage: + + +*/ +// +// Original Author: Chris Jones +// Created: Sun, 06 Oct 2013 15:48:44 GMT +// + +// system include files +#include +#include +#include +#include +#include + +// user include files + +// forward declarations +class testSharedResourcesRegistry; + +namespace edm { + class SharedResourcesAcquirer; + + class SharedResourcesRegistry + { + + public: + //needed for testing + friend class ::testSharedResourcesRegistry; + + // ---------- const member functions --------------------- + SharedResourcesAcquirer createAcquirer(std::vector const&) const; + + // ---------- static member functions -------------------- + static SharedResourcesRegistry* instance(); + + ///All legacy modules share this resource + static const std::string kLegacyModuleResourceName; + + // ---------- member functions --------------------------- + ///A resource name must be registered before it can be used in the createAcquirer call + void registerSharedResource(const std::string&); + + private: + SharedResourcesRegistry()=default; + ~SharedResourcesRegistry()=default; + + SharedResourcesRegistry(const SharedResourcesRegistry&) = delete; // stop default + + const SharedResourcesRegistry& operator=(const SharedResourcesRegistry&) = delete; // stop default + + // ---------- member data -------------------------------- + std::map,unsigned int>> resourceMap_; + + }; +} + +#endif diff --git a/FWCore/Framework/test/BuildFile.xml b/FWCore/Framework/test/BuildFile.xml index 2f50443bde50a..468ec3da176c9 100644 --- a/FWCore/Framework/test/BuildFile.xml +++ b/FWCore/Framework/test/BuildFile.xml @@ -175,7 +175,7 @@ - + diff --git a/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc b/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc new file mode 100644 index 0000000000000..af120f7cb6a15 --- /dev/null +++ b/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc @@ -0,0 +1,114 @@ +/* + * sharedresourcesregistry_t.cppunit.cc + * CMSSW + * + * Created by Chris Jones on 8/8/05. + * + */ + +#include "cppunit/extensions/HelperMacros.h" + +#include "FWCore/Framework/src/SharedResourcesRegistry.h" +#include "FWCore/Framework/src/SharedResourcesAcquirer.h" + +using namespace edm; + +class testSharedResourcesRegistry: public CppUnit::TestFixture +{ + CPPUNIT_TEST_SUITE(testSharedResourcesRegistry); + + CPPUNIT_TEST(oneTest); + CPPUNIT_TEST(legacyTest); + CPPUNIT_TEST(multipleTest); + + CPPUNIT_TEST_SUITE_END(); +public: + void setUp(){} + void tearDown(){} + + void oneTest(); + void legacyTest(); + void multipleTest(); +}; + +///registration of the test so that the runner can find it +CPPUNIT_TEST_SUITE_REGISTRATION(testSharedResourcesRegistry); + + +void testSharedResourcesRegistry::oneTest() +{ + edm::SharedResourcesRegistry reg; + + reg.registerSharedResource("foo"); + reg.registerSharedResource("bar"); + reg.registerSharedResource("zoo"); + + { + std::vector res{"foo","bar","zoo"}; + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(0 == tester.numberOfResources()); + } + { + std::vector res{"foo"}; + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(0 == tester.numberOfResources()); + } +} + +void testSharedResourcesRegistry::legacyTest() +{ + std::vector res{edm::SharedResourcesRegistry::kLegacyModuleResourceName}; + { + edm::SharedResourcesRegistry reg; + + reg.registerSharedResource(edm::SharedResourcesRegistry::kLegacyModuleResourceName); + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(0 == tester.numberOfResources()); + } + { + edm::SharedResourcesRegistry reg; + + reg.registerSharedResource(edm::SharedResourcesRegistry::kLegacyModuleResourceName); + reg.registerSharedResource(edm::SharedResourcesRegistry::kLegacyModuleResourceName); + + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(1 == tester.numberOfResources()); + + } +} + +void testSharedResourcesRegistry::multipleTest() +{ + edm::SharedResourcesRegistry reg; + + reg.registerSharedResource("foo"); + reg.registerSharedResource("bar"); + reg.registerSharedResource("zoo"); + reg.registerSharedResource("zoo"); + reg.registerSharedResource("bar"); + reg.registerSharedResource("zoo"); + + { + std::vector res{"foo","bar","zoo"}; + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(2 == tester.numberOfResources()); + } + { + std::vector res{"foo"}; + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(0 == tester.numberOfResources()); + } + { + std::vector res{"bar"}; + auto tester = reg.createAcquirer(res); + + CPPUNIT_ASSERT(1 == tester.numberOfResources()); + } + +} From 683b3e96f87c9c2e3c23ef473647ecec08f48cf2 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 7 Oct 2013 15:26:14 -0500 Subject: [PATCH 05/12] Switched to using recursive_mutex and moved SharedResourcesAcquirer.h to interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to more easily accommodate unscheduled execution, I switched from using a mutex to a recursive_mutex. This will be less performant since the locks will be held longer than might be needed but it is easier to implement initially. A more performant way would be to release the locks temporarily while we are doing a ‘edm::Event::getBy*’ call. --- .../{src => interface}/SharedResourcesAcquirer.h | 4 ++-- FWCore/Framework/src/SharedResourcesAcquirer.cc | 2 +- FWCore/Framework/src/SharedResourcesRegistry.cc | 14 ++++++++++---- FWCore/Framework/src/SharedResourcesRegistry.h | 2 +- .../test/sharedresourcesregistry_t.cppunit.cc | 2 +- 5 files changed, 15 insertions(+), 9 deletions(-) rename FWCore/Framework/{src => interface}/SharedResourcesAcquirer.h (93%) diff --git a/FWCore/Framework/src/SharedResourcesAcquirer.h b/FWCore/Framework/interface/SharedResourcesAcquirer.h similarity index 93% rename from FWCore/Framework/src/SharedResourcesAcquirer.h rename to FWCore/Framework/interface/SharedResourcesAcquirer.h index 027ad28e719a0..f509d74323126 100644 --- a/FWCore/Framework/src/SharedResourcesAcquirer.h +++ b/FWCore/Framework/interface/SharedResourcesAcquirer.h @@ -34,7 +34,7 @@ namespace edm { friend class ::testSharedResourcesRegistry; SharedResourcesAcquirer() = default; - explicit SharedResourcesAcquirer(std::vector&& iResources): + explicit SharedResourcesAcquirer(std::vector&& iResources): m_resources(iResources){} SharedResourcesAcquirer(SharedResourcesAcquirer&&) = default; @@ -61,7 +61,7 @@ namespace edm { private: // ---------- member data -------------------------------- - std::vector m_resources; + std::vector m_resources; }; } diff --git a/FWCore/Framework/src/SharedResourcesAcquirer.cc b/FWCore/Framework/src/SharedResourcesAcquirer.cc index f0590fe5deb08..f1310454a2d1f 100644 --- a/FWCore/Framework/src/SharedResourcesAcquirer.cc +++ b/FWCore/Framework/src/SharedResourcesAcquirer.cc @@ -13,7 +13,7 @@ // system include files // user include files -#include "SharedResourcesAcquirer.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" namespace edm { diff --git a/FWCore/Framework/src/SharedResourcesRegistry.cc b/FWCore/Framework/src/SharedResourcesRegistry.cc index 0eaf2d795ef44..bef8fbfde9486 100644 --- a/FWCore/Framework/src/SharedResourcesRegistry.cc +++ b/FWCore/Framework/src/SharedResourcesRegistry.cc @@ -15,19 +15,25 @@ // user include files #include "SharedResourcesRegistry.h" -#include "SharedResourcesAcquirer.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" namespace edm { const std::string SharedResourcesRegistry::kLegacyModuleResourceName{"__legacy__"}; + SharedResourcesRegistry* + SharedResourcesRegistry::instance() { + static SharedResourcesRegistry s_instance; + return &s_instance; + } + void SharedResourcesRegistry::registerSharedResource(const std::string& iString){ auto& values = resourceMap_[iString]; if(values.second ==1) { //only need to make the resource if more than 1 module wants it - values.first = std::shared_ptr( new std::mutex ); + values.first = std::shared_ptr( new std::recursive_mutex ); } ++(values.second); } @@ -35,7 +41,7 @@ namespace edm { SharedResourcesAcquirer SharedResourcesRegistry::createAcquirer(std::vector const & iNames) const { //Sort by how often used and then by name - std::map, std::mutex*> sortedResources; + std::map, std::recursive_mutex*> sortedResources; for(auto const& name: iNames) { auto itFound = resourceMap_.find(name); @@ -45,7 +51,7 @@ namespace edm { sortedResources.insert(std::make_pair(std::make_pair(itFound->second.second, itFound->first),itFound->second.first.get())); } } - std::vector mutexes; + std::vector mutexes; mutexes.reserve(sortedResources.size()); for(auto const& resource: sortedResources) { mutexes.push_back(resource.second); diff --git a/FWCore/Framework/src/SharedResourcesRegistry.h b/FWCore/Framework/src/SharedResourcesRegistry.h index 4d353b8b6f526..650b86133347b 100644 --- a/FWCore/Framework/src/SharedResourcesRegistry.h +++ b/FWCore/Framework/src/SharedResourcesRegistry.h @@ -62,7 +62,7 @@ namespace edm { const SharedResourcesRegistry& operator=(const SharedResourcesRegistry&) = delete; // stop default // ---------- member data -------------------------------- - std::map,unsigned int>> resourceMap_; + std::map,unsigned int>> resourceMap_; }; } diff --git a/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc b/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc index af120f7cb6a15..4ac10c1a5e252 100644 --- a/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc +++ b/FWCore/Framework/test/sharedresourcesregistry_t.cppunit.cc @@ -9,7 +9,7 @@ #include "cppunit/extensions/HelperMacros.h" #include "FWCore/Framework/src/SharedResourcesRegistry.h" -#include "FWCore/Framework/src/SharedResourcesAcquirer.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" using namespace edm; From 4828ba433996bff7bc4a52692eba7195b59373ac Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Mon, 7 Oct 2013 15:28:44 -0500 Subject: [PATCH 06/12] Made legacy modules thread safe Now uses a std::mutex to guarantee a module instance is only processing one event at a time. Also uses a edm::SharedResourcesAcquirer to get the legacy resource and therefore guarantee only one legacy module is running at a time. --- FWCore/Framework/interface/EDAnalyzer.h | 6 +++++- FWCore/Framework/interface/EDFilter.h | 9 +++++--- FWCore/Framework/interface/EDProducer.h | 4 ++++ FWCore/Framework/interface/OutputModule.h | 6 ++++++ FWCore/Framework/src/EDAnalyzer.cc | 18 ++++++++++++++-- FWCore/Framework/src/EDFilter.cc | 24 +++++++++++++++++++--- FWCore/Framework/src/EDProducer.cc | 19 ++++++++++++++--- FWCore/Framework/src/OutputModule.cc | 25 ++++++++++++++++++----- 8 files changed, 94 insertions(+), 17 deletions(-) diff --git a/FWCore/Framework/interface/EDAnalyzer.h b/FWCore/Framework/interface/EDAnalyzer.h index 4721777a62f65..5200cb6e13158 100644 --- a/FWCore/Framework/interface/EDAnalyzer.h +++ b/FWCore/Framework/interface/EDAnalyzer.h @@ -5,8 +5,10 @@ #include "DataFormats/Provenance/interface/ModuleDescription.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" #include "FWCore/Framework/interface/EDConsumerBase.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include +#include // EDAnalyzer is the base class for all analyzer "modules". @@ -25,7 +27,7 @@ namespace edm { template friend class WorkerT; typedef EDAnalyzer ModuleType; - EDAnalyzer() : moduleDescription_() {} + EDAnalyzer(); virtual ~EDAnalyzer(); std::string workerType() const {return "WorkerT";} @@ -75,6 +77,8 @@ namespace edm { moduleDescription_ = md; } ModuleDescription moduleDescription_; + SharedResourcesAcquirer resourceAcquirer_; + std::mutex mutex_; std::function callWhenNewProductsRegistered_; }; diff --git a/FWCore/Framework/interface/EDFilter.h b/FWCore/Framework/interface/EDFilter.h index 4159119270556..d9813990d4a43 100644 --- a/FWCore/Framework/interface/EDFilter.h +++ b/FWCore/Framework/interface/EDFilter.h @@ -14,11 +14,14 @@ These products should be informational products about the filter decision. #include "FWCore/Framework/interface/ProducerBase.h" #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/Frameworkfwd.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" + #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" #include "DataFormats/Provenance/interface/ModuleDescription.h" #include #include +#include namespace edm { namespace maker { @@ -34,9 +37,7 @@ namespace edm { template friend class WorkerT; typedef EDFilter ModuleType; - EDFilter() : ProducerBase() , moduleDescription_(), - previousParentage_(), previousParentageId_() { - } + EDFilter(); virtual ~EDFilter(); static void fillDescriptions(ConfigurationDescriptions& descriptions); @@ -90,6 +91,8 @@ namespace edm { } ModuleDescription moduleDescription_; std::vector previousParentage_; + SharedResourcesAcquirer resourceAcquirer_; + std::mutex mutex_; ParentageID previousParentageId_; }; } diff --git a/FWCore/Framework/interface/EDProducer.h b/FWCore/Framework/interface/EDProducer.h index 27b016b705d2d..5bad51b5ffd1c 100644 --- a/FWCore/Framework/interface/EDProducer.h +++ b/FWCore/Framework/interface/EDProducer.h @@ -11,12 +11,14 @@ EDProducts into an Event. #include "FWCore/Framework/interface/ProducerBase.h" #include "FWCore/Framework/interface/EDConsumerBase.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include "FWCore/Framework/interface/Frameworkfwd.h" #include "DataFormats/Provenance/interface/ModuleDescription.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" #include #include +#include namespace edm { @@ -85,6 +87,8 @@ namespace edm { } ModuleDescription moduleDescription_; std::vector previousParentage_; + SharedResourcesAcquirer resourceAcquirer_; + std::mutex mutex_; ParentageID previousParentageId_; }; } diff --git a/FWCore/Framework/interface/OutputModule.h b/FWCore/Framework/interface/OutputModule.h index 0af2e3a9ac691..bd27d2564d6f8 100644 --- a/FWCore/Framework/interface/OutputModule.h +++ b/FWCore/Framework/interface/OutputModule.h @@ -21,6 +21,8 @@ output stream. #include "FWCore/Framework/interface/ProductSelector.h" #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/getAllTriggerNames.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" + #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" #include @@ -28,6 +30,7 @@ output stream. #include #include #include +#include namespace edm { @@ -162,6 +165,9 @@ namespace edm { BranchChildren branchChildren_; + SharedResourcesAcquirer resourceAcquirer_; + std::mutex mutex_; + //------------------------------------------------------------------ // private member functions //------------------------------------------------------------------ diff --git a/FWCore/Framework/src/EDAnalyzer.cc b/FWCore/Framework/src/EDAnalyzer.cc index 2f0fd31bfda3d..fc8f5a47aaba2 100644 --- a/FWCore/Framework/src/EDAnalyzer.cc +++ b/FWCore/Framework/src/EDAnalyzer.cc @@ -8,6 +8,9 @@ #include "FWCore/Framework/interface/LuminosityBlock.h" #include "FWCore/Framework/interface/Run.h" #include "FWCore/Framework/src/edmodule_mightGet_config.h" +#include "FWCore/Framework/interface/ConstProductRegistry.h" + +#include "SharedResourcesRegistry.h" #include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h" #include "FWCore/ParameterSet/interface/ParameterSetDescription.h" @@ -15,23 +18,34 @@ #include "DataFormats/Provenance/interface/ProductRegistry.h" #include "FWCore/ServiceRegistry/interface/Service.h" -#include "FWCore/Framework/interface/ConstProductRegistry.h" namespace edm { EDAnalyzer::~EDAnalyzer() { } + EDAnalyzer::EDAnalyzer() : moduleDescription_() { + SharedResourcesRegistry::instance()->registerSharedResource( + SharedResourcesRegistry::kLegacyModuleResourceName); + } + bool EDAnalyzer::doEvent(EventPrincipal const& ep, EventSetup const& c, ModuleCallingContext const* mcc) { Event e(const_cast(ep), moduleDescription_, mcc); e.setConsumer(this); - this->analyze(e, c); + { + std::lock_guard guard(mutex_); + std::lock_guard guardAcq(resourceAcquirer_); + this->analyze(e, c); + } return true; } void EDAnalyzer::doBeginJob() { + std::vector res = {SharedResourcesRegistry::kLegacyModuleResourceName}; + resourceAcquirer_ = SharedResourcesRegistry::instance()->createAcquirer(res); + this->beginJob(); } diff --git a/FWCore/Framework/src/EDFilter.cc b/FWCore/Framework/src/EDFilter.cc index c66f8afeabef7..a352ab08c6cbe 100644 --- a/FWCore/Framework/src/EDFilter.cc +++ b/FWCore/Framework/src/EDFilter.cc @@ -9,10 +9,19 @@ #include "FWCore/Framework/interface/Run.h" #include "FWCore/Framework/src/edmodule_mightGet_config.h" +#include "SharedResourcesRegistry.h" + #include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h" #include "FWCore/ParameterSet/interface/ParameterSetDescription.h" namespace edm { + + EDFilter::EDFilter() : ProducerBase() , moduleDescription_(), + previousParentage_(), previousParentageId_() { + SharedResourcesRegistry::instance()->registerSharedResource( + SharedResourcesRegistry::kLegacyModuleResourceName); + } + EDFilter::~EDFilter() { } @@ -22,13 +31,22 @@ namespace edm { bool rc = false; Event e(ep, moduleDescription_, mcc); e.setConsumer(this); - rc = this->filter(e, c); - commit_(e,&previousParentage_, &previousParentageId_); + { + std::lock_guard guard(mutex_); + { + std::lock_guard guardAcq(resourceAcquirer_); + rc = this->filter(e, c); + } + commit_(e,&previousParentage_, &previousParentageId_); + } return rc; } void - EDFilter::doBeginJob() { + EDFilter::doBeginJob() { + std::vector res = {SharedResourcesRegistry::kLegacyModuleResourceName}; + resourceAcquirer_ = SharedResourcesRegistry::instance()->createAcquirer(res); + this->beginJob(); } diff --git a/FWCore/Framework/src/EDProducer.cc b/FWCore/Framework/src/EDProducer.cc index 966b02030d53c..57dd29315666f 100644 --- a/FWCore/Framework/src/EDProducer.cc +++ b/FWCore/Framework/src/EDProducer.cc @@ -12,12 +12,17 @@ #include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h" #include "FWCore/ParameterSet/interface/ParameterSetDescription.h" +#include "SharedResourcesRegistry.h" + namespace edm { EDProducer::EDProducer() : ProducerBase(), moduleDescription_(), previousParentage_(), - previousParentageId_() { } + previousParentageId_() { + SharedResourcesRegistry::instance()->registerSharedResource( + SharedResourcesRegistry::kLegacyModuleResourceName); + } EDProducer::~EDProducer() { } @@ -26,13 +31,21 @@ namespace edm { ModuleCallingContext const* mcc) { Event e(ep, moduleDescription_, mcc); e.setConsumer(this); - this->produce(e, c); - commit_(e, &previousParentage_, &previousParentageId_); + { + std::lock_guard guard(mutex_); + { + std::lock_guard guardAcq(resourceAcquirer_); + this->produce(e, c); + } + commit_(e, &previousParentage_, &previousParentageId_); + } return true; } void EDProducer::doBeginJob() { + std::vector res = {SharedResourcesRegistry::kLegacyModuleResourceName}; + resourceAcquirer_ = SharedResourcesRegistry::instance()->createAcquirer(res); this->beginJob(); } diff --git a/FWCore/Framework/src/OutputModule.cc b/FWCore/Framework/src/OutputModule.cc index 300da1164fb79..25f4e049fa634 100644 --- a/FWCore/Framework/src/OutputModule.cc +++ b/FWCore/Framework/src/OutputModule.cc @@ -19,6 +19,8 @@ #include "FWCore/ServiceRegistry/interface/Service.h" #include "FWCore/Utilities/interface/DebugMacros.h" +#include "SharedResourcesRegistry.h" + #include namespace edm { @@ -57,6 +59,9 @@ namespace edm { process_name_, getAllTriggerNames(), selectors_); + SharedResourcesRegistry::instance()->registerSharedResource( + SharedResourcesRegistry::kLegacyModuleResourceName); + } void OutputModule::configure(OutputModuleDescription const& desc) { @@ -154,6 +159,9 @@ namespace edm { OutputModule::~OutputModule() { } void OutputModule::doBeginJob() { + std::vector res = {SharedResourcesRegistry::kLegacyModuleResourceName}; + resourceAcquirer_ = SharedResourcesRegistry::instance()->createAcquirer(res); + this->beginJob(); } @@ -176,13 +184,20 @@ namespace edm { FDEBUG(2) << "writeEvent called\n"; - if(!wantAllEvents_) { - if(!selectors_.wantEvent(ep, mcc)) { - return true; + { + std::lock_guard guard(mutex_); + if(!wantAllEvents_) { + if(!selectors_.wantEvent(ep, mcc)) { + return true; + } + } + + { + std::lock_guard guardAcq(resourceAcquirer_); + write(ep, mcc); } + updateBranchParents(ep); } - write(ep, mcc); - updateBranchParents(ep); if(remainingEvents_ > 0) { --remainingEvents_; } From a3787d777575723e7179e064495201ec45fd66c8 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 8 Oct 2013 10:56:35 -0500 Subject: [PATCH 07/12] =?UTF-8?q?Use=20the=20SharedResourcesAcquirer=20whe?= =?UTF-8?q?n=20a=20=E2=80=98one=E2=80=99=20type=20module=20declares=20a=20?= =?UTF-8?q?SharedResource?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All ‘one’ style base classes now hold a SharedResourcesAcquirer and use a new virtual function ‘createAcquirer()’ which by default returns an empty SharedResourcesAcquirer instance. If the module declares the use of a ‘SharedResource’ then the ‘createAcquirer()’ method will be overridden and return an instance with the needed resources. To facility this, edm::one::imll::SharedResourcesUser had to be changed to a templated class. --- .../Framework/interface/one/EDAnalyzerBase.h | 6 +++++ FWCore/Framework/interface/one/EDFilterBase.h | 6 ++++- .../Framework/interface/one/EDProducerBase.h | 4 +++ .../interface/one/OutputModuleBase.h | 6 +++++ .../one/analyzerAbilityToImplementor.h | 2 +- .../one/filterAbilityToImplementor.h | 2 +- FWCore/Framework/interface/one/implementors.h | 15 ++++++++--- .../one/outputmoduleAbilityToImplementor.h | 2 +- .../one/producerAbilityToImplementor.h | 2 +- FWCore/Framework/src/one/EDAnalyzerBase.cc | 12 ++++++++- FWCore/Framework/src/one/EDFilterBase.cc | 17 ++++++++++-- FWCore/Framework/src/one/EDProducerBase.cc | 16 ++++++++++-- FWCore/Framework/src/one/OutputModuleBase.cc | 21 +++++++++++---- .../Framework/src/one/SharedResourcesUser.cc | 26 ------------------- .../Framework/src/one/implementorsMethods.h | 20 ++++++++++++++ 15 files changed, 112 insertions(+), 45 deletions(-) delete mode 100644 FWCore/Framework/src/one/SharedResourcesUser.cc diff --git a/FWCore/Framework/interface/one/EDAnalyzerBase.h b/FWCore/Framework/interface/one/EDAnalyzerBase.h index 4e47fd0c1d969..bc17b4ce0f1fe 100644 --- a/FWCore/Framework/interface/one/EDAnalyzerBase.h +++ b/FWCore/Framework/interface/one/EDAnalyzerBase.h @@ -23,6 +23,7 @@ // user include files #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/Frameworkfwd.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include "DataFormats/Provenance/interface/ModuleDescription.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" @@ -97,11 +98,16 @@ namespace edm { virtual void doBeginLuminosityBlock_(LuminosityBlock const& lbp, EventSetup const& c); virtual void doEndLuminosityBlock_(LuminosityBlock const& lbp, EventSetup const& c); + virtual SharedResourcesAcquirer createAcquirer(); + void setModuleDescription(ModuleDescription const& md) { moduleDescription_ = md; } ModuleDescription moduleDescription_; std::function callWhenNewProductsRegistered_; + + SharedResourcesAcquirer resourcesAcquirer_; + std::mutex mutex_; }; } } diff --git a/FWCore/Framework/interface/one/EDFilterBase.h b/FWCore/Framework/interface/one/EDFilterBase.h index 3244e1173f1a2..78990f8345574 100644 --- a/FWCore/Framework/interface/one/EDFilterBase.h +++ b/FWCore/Framework/interface/one/EDFilterBase.h @@ -24,6 +24,7 @@ #include "FWCore/Framework/interface/ProducerBase.h" #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/Frameworkfwd.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include "DataFormats/Provenance/interface/ModuleDescription.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" @@ -99,8 +100,9 @@ namespace edm { virtual void doEndRunProduce_(Run& rp, EventSetup const& c); virtual void doBeginLuminosityBlockProduce_(LuminosityBlock& lbp, EventSetup const& c); virtual void doEndLuminosityBlockProduce_(LuminosityBlock& lbp, EventSetup const& c); - + virtual SharedResourcesAcquirer createAcquirer(); + void setModuleDescription(ModuleDescription const& md) { moduleDescription_ = md; } @@ -108,6 +110,8 @@ namespace edm { std::vector previousParentage_; ParentageID previousParentageId_; + SharedResourcesAcquirer resourcesAcquirer_; + std::mutex mutex_; }; } diff --git a/FWCore/Framework/interface/one/EDProducerBase.h b/FWCore/Framework/interface/one/EDProducerBase.h index 058144bc7ff7d..4aa7b368645c8 100644 --- a/FWCore/Framework/interface/one/EDProducerBase.h +++ b/FWCore/Framework/interface/one/EDProducerBase.h @@ -24,6 +24,7 @@ #include "FWCore/Framework/interface/ProducerBase.h" #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/Frameworkfwd.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include "DataFormats/Provenance/interface/ModuleDescription.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" @@ -99,6 +100,7 @@ namespace edm { virtual void doBeginLuminosityBlockProduce_(LuminosityBlock& lbp, EventSetup const& c); virtual void doEndLuminosityBlockProduce_(LuminosityBlock& lbp, EventSetup const& c); + virtual SharedResourcesAcquirer createAcquirer(); void setModuleDescription(ModuleDescription const& md) { moduleDescription_ = md; @@ -107,6 +109,8 @@ namespace edm { std::vector previousParentage_; ParentageID previousParentageId_; + SharedResourcesAcquirer resourcesAcquirer_; + std::mutex mutex_; }; } diff --git a/FWCore/Framework/interface/one/OutputModuleBase.h b/FWCore/Framework/interface/one/OutputModuleBase.h index d830b86c535f3..1e3e7a9d17030 100644 --- a/FWCore/Framework/interface/one/OutputModuleBase.h +++ b/FWCore/Framework/interface/one/OutputModuleBase.h @@ -40,6 +40,7 @@ #include "FWCore/Framework/interface/ProductSelector.h" #include "FWCore/Framework/interface/EDConsumerBase.h" #include "FWCore/Framework/interface/getAllTriggerNames.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" #include "FWCore/ParameterSet/interface/ParameterSetfwd.h" // forward declarations @@ -176,9 +177,14 @@ namespace edm { BranchChildren branchChildren_; + SharedResourcesAcquirer resourcesAcquirer_; + std::mutex mutex_; //------------------------------------------------------------------ // private member functions //------------------------------------------------------------------ + + virtual SharedResourcesAcquirer createAcquirer(); + void doWriteRun(RunPrincipal const& rp, ModuleCallingContext const*); void doWriteLuminosityBlock(LuminosityBlockPrincipal const& lbp, ModuleCallingContext const*); void doOpenFile(FileBlock const& fb); diff --git a/FWCore/Framework/interface/one/analyzerAbilityToImplementor.h b/FWCore/Framework/interface/one/analyzerAbilityToImplementor.h index 60102a8b62cc7..6de56c259712a 100644 --- a/FWCore/Framework/interface/one/analyzerAbilityToImplementor.h +++ b/FWCore/Framework/interface/one/analyzerAbilityToImplementor.h @@ -35,7 +35,7 @@ namespace edm { template<> struct AbilityToImplementor { - typedef edm::one::impl::SharedResourcesUser Type; + typedef edm::one::impl::SharedResourcesUser Type; }; template<> diff --git a/FWCore/Framework/interface/one/filterAbilityToImplementor.h b/FWCore/Framework/interface/one/filterAbilityToImplementor.h index 160e9c3fd7c7b..ca851ebb5386f 100644 --- a/FWCore/Framework/interface/one/filterAbilityToImplementor.h +++ b/FWCore/Framework/interface/one/filterAbilityToImplementor.h @@ -35,7 +35,7 @@ namespace edm { template<> struct AbilityToImplementor { - typedef edm::one::impl::SharedResourcesUser Type; + typedef edm::one::impl::SharedResourcesUser Type; }; template<> diff --git a/FWCore/Framework/interface/one/implementors.h b/FWCore/Framework/interface/one/implementors.h index 0750408850d7f..e87d0398a1bcc 100644 --- a/FWCore/Framework/interface/one/implementors.h +++ b/FWCore/Framework/interface/one/implementors.h @@ -20,6 +20,7 @@ // system include files #include +#include // user include files #include "FWCore/Framework/interface/Frameworkfwd.h" @@ -27,22 +28,28 @@ // forward declarations namespace edm { + class SharedResourcesAcquirer; + namespace one { namespace impl { - class SharedResourcesUser { + template + class SharedResourcesUser : public virtual T { public: template< typename... Args> - SharedResourcesUser(Args...) {} + SharedResourcesUser(Args... args) : T(args...) {} SharedResourcesUser(SharedResourcesUser const&) = delete; SharedResourcesUser& operator=(SharedResourcesUser const&) = delete; virtual ~SharedResourcesUser() {} protected: - static const std::string kUnknownResource; - void usesResource(std::string const& iName = kUnknownResource); + void usesResource(std::string const& iName); + void usesResource(); + private: + SharedResourcesAcquirer createAcquirer() override; + std::set resourceNames_; }; template diff --git a/FWCore/Framework/interface/one/outputmoduleAbilityToImplementor.h b/FWCore/Framework/interface/one/outputmoduleAbilityToImplementor.h index 8f7fe4a9cf876..033f965da7eb8 100644 --- a/FWCore/Framework/interface/one/outputmoduleAbilityToImplementor.h +++ b/FWCore/Framework/interface/one/outputmoduleAbilityToImplementor.h @@ -81,7 +81,7 @@ namespace edm { template<> struct AbilityToImplementor { - typedef edm::one::impl::SharedResourcesUser Type; + typedef edm::one::impl::SharedResourcesUser Type; }; template<> diff --git a/FWCore/Framework/interface/one/producerAbilityToImplementor.h b/FWCore/Framework/interface/one/producerAbilityToImplementor.h index 0311834f475f6..4a5cce2c08f61 100644 --- a/FWCore/Framework/interface/one/producerAbilityToImplementor.h +++ b/FWCore/Framework/interface/one/producerAbilityToImplementor.h @@ -35,7 +35,7 @@ namespace edm { template<> struct AbilityToImplementor { - typedef edm::one::impl::SharedResourcesUser Type; + typedef edm::one::impl::SharedResourcesUser Type; }; template<> diff --git a/FWCore/Framework/src/one/EDAnalyzerBase.cc b/FWCore/Framework/src/one/EDAnalyzerBase.cc index e7fe9441cc72f..a44e0d3bebfec 100644 --- a/FWCore/Framework/src/one/EDAnalyzerBase.cc +++ b/FWCore/Framework/src/one/EDAnalyzerBase.cc @@ -50,12 +50,22 @@ namespace edm { ModuleCallingContext const* mcc) { Event e(ep, moduleDescription_, mcc); e.setConsumer(this); - this->analyze(e, c); + { + std::lock_guard guard(mutex_); + std::lock_guard guardResources(resourcesAcquirer_); + this->analyze(e, c); + } return true; } + SharedResourcesAcquirer EDAnalyzerBase::createAcquirer() { + return SharedResourcesAcquirer{}; + } + void EDAnalyzerBase::doBeginJob() { + resourcesAcquirer_ = createAcquirer(); + this->beginJob(); } diff --git a/FWCore/Framework/src/one/EDFilterBase.cc b/FWCore/Framework/src/one/EDFilterBase.cc index 63705c302b218..1b3a2efddd30e 100644 --- a/FWCore/Framework/src/one/EDFilterBase.cc +++ b/FWCore/Framework/src/one/EDFilterBase.cc @@ -52,13 +52,26 @@ namespace edm { ModuleCallingContext const* mcc) { Event e(ep, moduleDescription_, mcc); e.setConsumer(this); - bool returnValue = this->filter(e, c); - commit_(e,&previousParentage_, &previousParentageId_); + bool returnValue =true; + { + std::lock_guard guard(mutex_); + { + std::lock_guard guard(resourcesAcquirer_); + returnValue = this->filter(e, c); + } + commit_(e,&previousParentage_, &previousParentageId_); + } return returnValue; } + SharedResourcesAcquirer EDFilterBase::createAcquirer() { + return SharedResourcesAcquirer{}; + } + void EDFilterBase::doBeginJob() { + resourcesAcquirer_ = createAcquirer(); + this->beginJob(); } diff --git a/FWCore/Framework/src/one/EDProducerBase.cc b/FWCore/Framework/src/one/EDProducerBase.cc index 0cd3208d4c817..718d593cf90ed 100644 --- a/FWCore/Framework/src/one/EDProducerBase.cc +++ b/FWCore/Framework/src/one/EDProducerBase.cc @@ -51,13 +51,25 @@ namespace edm { ModuleCallingContext const* mcc) { Event e(ep, moduleDescription_, mcc); e.setConsumer(this); - this->produce(e, c); - commit_(e,&previousParentage_, &previousParentageId_); + { + std::lock_guard guard(mutex_); + { + std::lock_guard guard(resourcesAcquirer_); + this->produce(e, c); + } + commit_(e,&previousParentage_, &previousParentageId_); + } return true; } + SharedResourcesAcquirer EDProducerBase::createAcquirer() { + return SharedResourcesAcquirer{}; + } + void EDProducerBase::doBeginJob() { + resourcesAcquirer_ = createAcquirer(); + this->beginJob(); } diff --git a/FWCore/Framework/src/one/OutputModuleBase.cc b/FWCore/Framework/src/one/OutputModuleBase.cc index b8fedd2bf6e6a..fd5c7687cbfcc 100644 --- a/FWCore/Framework/src/one/OutputModuleBase.cc +++ b/FWCore/Framework/src/one/OutputModuleBase.cc @@ -165,7 +165,12 @@ namespace edm { OutputModuleBase::~OutputModuleBase() { } + SharedResourcesAcquirer OutputModuleBase::createAcquirer() { + return SharedResourcesAcquirer{}; + } + void OutputModuleBase::doBeginJob() { + resourcesAcquirer_ = createAcquirer(); this->beginJob(); } @@ -184,13 +189,19 @@ namespace edm { ModuleCallingContext const* mcc) { detail::TRBESSentry products_sentry(selectors_); - if(!wantAllEvents_) { - if(!selectors_.wantEvent(ep, mcc)) { - return true; + { + std::lock_guard guard(mutex_); + if(!wantAllEvents_) { + if(!selectors_.wantEvent(ep, mcc)) { + return true; + } + } + { + std::lock_guard guard(resourcesAcquirer_); + write(ep, mcc); } + updateBranchParents(ep); } - write(ep, mcc); - updateBranchParents(ep); if(remainingEvents_ > 0) { --remainingEvents_; } diff --git a/FWCore/Framework/src/one/SharedResourcesUser.cc b/FWCore/Framework/src/one/SharedResourcesUser.cc deleted file mode 100644 index 32322f28a2439..0000000000000 --- a/FWCore/Framework/src/one/SharedResourcesUser.cc +++ /dev/null @@ -1,26 +0,0 @@ -// -*- C++ -*- -// -// Package: Package -// Class : SharedResourcesUser -// -// Implementation: -// [Notes on implementation] -// -// Original Author: Chris Jones -// Created: Thu, 09 May 2013 20:39:03 GMT -// - -// system include files - -// user include files -#include "FWCore/Framework/interface/one/implementors.h" - - -const std::string -edm::one::impl::SharedResourcesUser::kUnknownResource; - -void -edm::one::impl::SharedResourcesUser::usesResource(std::string const& iName) -{ - //Don't do anything yet -} \ No newline at end of file diff --git a/FWCore/Framework/src/one/implementorsMethods.h b/FWCore/Framework/src/one/implementorsMethods.h index 037961815c6c3..8998dfbdfe220 100644 --- a/FWCore/Framework/src/one/implementorsMethods.h +++ b/FWCore/Framework/src/one/implementorsMethods.h @@ -22,12 +22,32 @@ // user include files #include "FWCore/Framework/interface/one/implementors.h" +#include "FWCore/Framework/src/SharedResourcesRegistry.h" +#include "FWCore/Framework/interface/SharedResourcesAcquirer.h" // forward declarations namespace edm { namespace one { namespace impl { + template + void SharedResourcesUser::usesResource(std::string const& iName) { + resourceNames_.insert(iName); + SharedResourcesRegistry::instance()->registerSharedResource(iName); + } + template + void SharedResourcesUser::usesResource() { + this->usesResource(SharedResourcesRegistry::kLegacyModuleResourceName); + + } + + template + SharedResourcesAcquirer SharedResourcesUser::createAcquirer() { + std::vector v(resourceNames_.begin(),resourceNames_.end()); + return SharedResourcesRegistry::instance()->createAcquirer(v); + } + + template< typename T> void RunWatcher::doBeginRun_(Run const& rp, EventSetup const& c) { this->beginRun(rp,c); From 711c510fc7e5fdd26bce999e892f374f6dca1d61 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 8 Oct 2013 12:43:09 -0500 Subject: [PATCH 08/12] Need to explicitly instantiate the SharedResourcesUser templated class The SharedResourcesUser class was just changed to a template. As with the other implementor types, this needs to be explicitly instantiated. --- FWCore/Framework/src/one/analyzerImplementors.cc | 1 + FWCore/Framework/src/one/filterImplementors.cc | 1 + FWCore/Framework/src/one/outputmoduleImplementors.cc | 3 +++ FWCore/Framework/src/one/producerImplementors.cc | 1 + 4 files changed, 6 insertions(+) diff --git a/FWCore/Framework/src/one/analyzerImplementors.cc b/FWCore/Framework/src/one/analyzerImplementors.cc index 7398edd1e8632..2a55dc4325503 100644 --- a/FWCore/Framework/src/one/analyzerImplementors.cc +++ b/FWCore/Framework/src/one/analyzerImplementors.cc @@ -19,6 +19,7 @@ namespace edm { namespace one { namespace impl { + template class SharedResourcesUser; template class RunWatcher; template class LuminosityBlockWatcher; } diff --git a/FWCore/Framework/src/one/filterImplementors.cc b/FWCore/Framework/src/one/filterImplementors.cc index c322c51c1c36e..fbd21a91fe8b2 100644 --- a/FWCore/Framework/src/one/filterImplementors.cc +++ b/FWCore/Framework/src/one/filterImplementors.cc @@ -19,6 +19,7 @@ namespace edm { namespace one { namespace impl { + template class SharedResourcesUser; template class RunWatcher; template class LuminosityBlockWatcher; template class BeginRunProducer; diff --git a/FWCore/Framework/src/one/outputmoduleImplementors.cc b/FWCore/Framework/src/one/outputmoduleImplementors.cc index 4b140936432fc..6c4300170acd8 100644 --- a/FWCore/Framework/src/one/outputmoduleImplementors.cc +++ b/FWCore/Framework/src/one/outputmoduleImplementors.cc @@ -21,6 +21,9 @@ namespace edm { class ModuleCallingContext; namespace one { + namespace impl { + template class SharedResourcesUser; + } namespace outputmodule { void RunWatcher::doBeginRun_(RunPrincipal const& rp, ModuleCallingContext const* mcc) { beginRun(rp, mcc); diff --git a/FWCore/Framework/src/one/producerImplementors.cc b/FWCore/Framework/src/one/producerImplementors.cc index 5bdc3835782ee..324f3fbd441af 100644 --- a/FWCore/Framework/src/one/producerImplementors.cc +++ b/FWCore/Framework/src/one/producerImplementors.cc @@ -19,6 +19,7 @@ namespace edm { namespace one { namespace impl { + template class SharedResourcesUser; template class RunWatcher; template class LuminosityBlockWatcher; template class BeginRunProducer; From 08e556141d9044e6cc93c92075984148aabba05b Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 8 Oct 2013 15:44:12 -0500 Subject: [PATCH 09/12] Added missing mutex includes --- FWCore/Framework/interface/one/EDAnalyzerBase.h | 1 + FWCore/Framework/interface/one/EDFilterBase.h | 1 + FWCore/Framework/interface/one/EDProducerBase.h | 1 + FWCore/Framework/interface/one/OutputModuleBase.h | 2 +- 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/FWCore/Framework/interface/one/EDAnalyzerBase.h b/FWCore/Framework/interface/one/EDAnalyzerBase.h index bc17b4ce0f1fe..78b6b587e8477 100644 --- a/FWCore/Framework/interface/one/EDAnalyzerBase.h +++ b/FWCore/Framework/interface/one/EDAnalyzerBase.h @@ -19,6 +19,7 @@ // // system include files +#include // user include files #include "FWCore/Framework/interface/EDConsumerBase.h" diff --git a/FWCore/Framework/interface/one/EDFilterBase.h b/FWCore/Framework/interface/one/EDFilterBase.h index 78990f8345574..259b28bbb9685 100644 --- a/FWCore/Framework/interface/one/EDFilterBase.h +++ b/FWCore/Framework/interface/one/EDFilterBase.h @@ -19,6 +19,7 @@ // // system include files +#include // user include files #include "FWCore/Framework/interface/ProducerBase.h" diff --git a/FWCore/Framework/interface/one/EDProducerBase.h b/FWCore/Framework/interface/one/EDProducerBase.h index 4aa7b368645c8..6a5da792b703d 100644 --- a/FWCore/Framework/interface/one/EDProducerBase.h +++ b/FWCore/Framework/interface/one/EDProducerBase.h @@ -19,6 +19,7 @@ // // system include files +#include // user include files #include "FWCore/Framework/interface/ProducerBase.h" diff --git a/FWCore/Framework/interface/one/OutputModuleBase.h b/FWCore/Framework/interface/one/OutputModuleBase.h index 1e3e7a9d17030..d043a4f48bfa3 100644 --- a/FWCore/Framework/interface/one/OutputModuleBase.h +++ b/FWCore/Framework/interface/one/OutputModuleBase.h @@ -24,7 +24,7 @@ #include #include #include - +#include // user include files #include "DataFormats/Provenance/interface/BranchChildren.h" From db850754614e5fcbc3b172e2218129e04036bfef Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 8 Oct 2013 17:18:08 -0500 Subject: [PATCH 10/12] commented out debug printouts --- FWCore/Framework/src/EventProcessor.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/FWCore/Framework/src/EventProcessor.cc b/FWCore/Framework/src/EventProcessor.cc index 7f0d3b2a5de7e..95d54fac1ecd1 100644 --- a/FWCore/Framework/src/EventProcessor.cc +++ b/FWCore/Framework/src/EventProcessor.cc @@ -1817,7 +1817,7 @@ namespace edm { } if(deferredExceptionPtrIsSet_.load(std::memory_order_acquire)) { //another thread hit an exception - std::cerr<<"another thread saw an exception\n"; + //std::cerr<<"another thread saw an exception\n"; break; } { @@ -1825,25 +1825,25 @@ namespace edm { std::lock_guard sourceGuard(nextTransitionMutex_); if(finishedProcessingEvents->load(std::memory_order_acquire)) { - std::cerr<<"finishedProcessingEvents\n"; + //std::cerr<<"finishedProcessingEvents\n"; break; } InputSource::ItemType itemType = input_->nextItemType(); if (InputSource::IsEvent !=itemType) { nextItemTypeFromProcessingEvents_ = itemType; finishedProcessingEvents->store(true,std::memory_order_release); - std::cerr<<"next item type "< Date: Wed, 9 Oct 2013 13:07:49 -0500 Subject: [PATCH 11/12] Make access to EventSetup thread safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a global mutex to serialize access to all EventSetup modules and sources. This is overkill but guarantees that if any two modules/sources are sharing a non-thread safe resource we don’t have a problem. Changed two of the mutable to std::atomics since they are read/written outside of the critical section guarded by the mutex. --- FWCore/Framework/interface/DataProxy.h | 8 ++++---- FWCore/Framework/src/DataProxy.cc | 24 ++++++++++++------------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/FWCore/Framework/interface/DataProxy.h b/FWCore/Framework/interface/DataProxy.h index 9832bb4a759cf..04ad3ac543182 100644 --- a/FWCore/Framework/interface/DataProxy.h +++ b/FWCore/Framework/interface/DataProxy.h @@ -20,6 +20,7 @@ // // system include files +#include // user include files @@ -85,12 +86,11 @@ namespace edm { DataProxy(DataProxy const&); // stop default DataProxy const& operator=(DataProxy const&); // stop default - void setCacheIsValidAndAccessType(bool iTransientAccessOnly) const; // ---------- member data -------------------------------- - mutable void const* cache_; - mutable bool cacheIsValid_; - mutable bool nonTransientAccessRequested_; + mutable void const* cache_; //protected by a global mutex + mutable std::atomic cacheIsValid_; + mutable std::atomic nonTransientAccessRequested_; ComponentDescription const* description_; }; } diff --git a/FWCore/Framework/src/DataProxy.cc b/FWCore/Framework/src/DataProxy.cc index 1291305edf25f..345642b085259 100644 --- a/FWCore/Framework/src/DataProxy.cc +++ b/FWCore/Framework/src/DataProxy.cc @@ -11,6 +11,7 @@ // // system include files +#include // user include files #include "FWCore/Framework/interface/DataProxy.h" @@ -24,6 +25,7 @@ // namespace edm { namespace eventsetup { + static std::recursive_mutex s_esGlobalMutex; // // static data member definitions // @@ -69,15 +71,7 @@ DataProxy::~DataProxy() // // member functions // -void -DataProxy::setCacheIsValidAndAccessType(bool iTransientAccessOnly) const { - cacheIsValid_ = true; - if(!iTransientAccessOnly) { - nonTransientAccessRequested_ = true; - } -} - -void DataProxy::clearCacheIsValid() { +void DataProxy::clearCacheIsValid() { cacheIsValid_ = false; nonTransientAccessRequested_ = false; cache_ = 0; @@ -110,13 +104,19 @@ const void* DataProxy::get(const EventSetupRecord& iRecord, const DataKey& iKey, bool iTransiently) const { if(!cacheIsValid()) { - cache_ = const_cast(this)->getImpl(iRecord, iKey); + std::lock_guard guard(s_esGlobalMutex); + if(!cacheIsValid()) { + cache_ = const_cast(this)->getImpl(iRecord, iKey); + cacheIsValid_ = true; + } } - //It is safe to always set cache to valid. //We need to set the AccessType for each request so this can't be called in the if block above. //This also must be before the cache_ check since we want to setCacheIsValid before a possible // exception throw. If we don't, 'getImpl' will be called again on a second request for the data. - setCacheIsValidAndAccessType(iTransiently); + if(!iTransiently) { + nonTransientAccessRequested_ = true; + } + if(0 == cache_) { throwMakeException(iRecord, iKey); } From 5609daee04111e95d880970644ec056e6802cfaa Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 9 Oct 2013 13:17:27 -0500 Subject: [PATCH 12/12] Use less strict acquire/release memory ordering for DataProxy cache info --- FWCore/Framework/interface/DataProxy.h | 2 +- FWCore/Framework/src/DataProxy.cc | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/FWCore/Framework/interface/DataProxy.h b/FWCore/Framework/interface/DataProxy.h index 04ad3ac543182..b98c227687289 100644 --- a/FWCore/Framework/interface/DataProxy.h +++ b/FWCore/Framework/interface/DataProxy.h @@ -38,7 +38,7 @@ namespace edm { virtual ~DataProxy(); // ---------- const member functions --------------------- - bool cacheIsValid() const { return cacheIsValid_; } + bool cacheIsValid() const { return cacheIsValid_.load(std::memory_order_acquire); } void doGet(EventSetupRecord const& iRecord, DataKey const& iKey, bool iTransiently) const; void const* get(EventSetupRecord const&, DataKey const& iKey, bool iTransiently) const; diff --git a/FWCore/Framework/src/DataProxy.cc b/FWCore/Framework/src/DataProxy.cc index 345642b085259..da8cf022902cc 100644 --- a/FWCore/Framework/src/DataProxy.cc +++ b/FWCore/Framework/src/DataProxy.cc @@ -72,14 +72,14 @@ DataProxy::~DataProxy() // member functions // void DataProxy::clearCacheIsValid() { - cacheIsValid_ = false; - nonTransientAccessRequested_ = false; + cacheIsValid_.store(false, std::memory_order_release); + nonTransientAccessRequested_.store(false, std::memory_order_release); cache_ = 0; } void DataProxy::resetIfTransient() { - if (!nonTransientAccessRequested_) { + if (!nonTransientAccessRequested_.load(std::memory_order_acquire)) { clearCacheIsValid(); invalidateTransientCache(); } @@ -106,15 +106,15 @@ DataProxy::get(const EventSetupRecord& iRecord, const DataKey& iKey, bool iTrans if(!cacheIsValid()) { std::lock_guard guard(s_esGlobalMutex); if(!cacheIsValid()) { - cache_ = const_cast(this)->getImpl(iRecord, iKey); - cacheIsValid_ = true; + cache_ = const_cast(this)->getImpl(iRecord, iKey); + cacheIsValid_.store(true,std::memory_order_release); } } //We need to set the AccessType for each request so this can't be called in the if block above. //This also must be before the cache_ check since we want to setCacheIsValid before a possible // exception throw. If we don't, 'getImpl' will be called again on a second request for the data. if(!iTransiently) { - nonTransientAccessRequested_ = true; + nonTransientAccessRequested_.store(true, std::memory_order_release); } if(0 == cache_) {