From 731222d38aa2c66fb3e111d181b02f2a174cffed Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Tue, 13 Jun 2023 18:21:28 +0200 Subject: [PATCH 1/2] Make edmNew::DetSet::set() private This change makes it easier to reason about thread safety elsewhere. --- DataFormats/Common/interface/DetSetNew.h | 6 +++--- DataFormats/Common/interface/DetSetVectorNew.h | 6 +----- .../SiPixelClusterizer/plugins/SiPixelClusterProducer.cc | 2 +- RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h | 3 +-- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/DataFormats/Common/interface/DetSetNew.h b/DataFormats/Common/interface/DetSetNew.h index 0718f30afd282..4df05206f66e2 100644 --- a/DataFormats/Common/interface/DetSetNew.h +++ b/DataFormats/Common/interface/DetSetNew.h @@ -42,9 +42,6 @@ namespace edmNew { set(icont, item, update); } - //FIXME (it may confuse users as size_type is same type as id_type...) - inline void set(Container const &icont, typename Container::Item const &item, bool update = true); - bool isValid() const { return m_offset >= 0; } inline data_type &operator[](size_type i) { return data()[i]; } @@ -79,6 +76,9 @@ namespace edmNew { unsigned int makeKeyOf(const_iterator ci) const { return ci - &(container().front()); } private: + //FIXME (it may confuse users as size_type is same type as id_type...) + inline void set(Container const &icont, typename Container::Item const &item, bool update = true); + DataContainer const &container() const { return *m_data; } data_type const *data() const { diff --git a/DataFormats/Common/interface/DetSetVectorNew.h b/DataFormats/Common/interface/DetSetVectorNew.h index f74c61c3bad71..041098a000b9f 100644 --- a/DataFormats/Common/interface/DetSetVectorNew.h +++ b/DataFormats/Common/interface/DetSetVectorNew.h @@ -183,14 +183,10 @@ namespace edmNew { IterHelp() : m_v(nullptr), m_update(false) {} IterHelp(DetSetVector const& iv, bool iup) : m_v(&iv), m_update(iup) {} - result_type& operator()(Item const& item) const { - m_detset.set(*m_v, item, m_update); - return m_detset; - } + result_type operator()(Item const& item) const { return result_type(*m_v, item, m_update); } private: DetSetVector const* m_v; - mutable result_type m_detset; bool m_update; }; diff --git a/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelClusterProducer.cc b/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelClusterProducer.cc index 53e0a958e5a1e..a23318293c3be 100644 --- a/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelClusterProducer.cc +++ b/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelClusterProducer.cc @@ -124,7 +124,7 @@ void SiPixelClusterProducer::produce(edm::Event& e, const edm::EventSetup& es) { output->shrink_to_fit(); // set sequential identifier - for (auto& clusters : *output) { + for (auto clusters : *output) { uint16_t id = 0; for (auto& cluster : clusters) { cluster.setOriginalId(id++); diff --git a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h index 92a85c6a5603e..f9dabef9b6c82 100644 --- a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h +++ b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h @@ -233,8 +233,7 @@ class StMeasurementDetSet { void getDetSet(int i) const { const auto& det = detSet_[i]; if (detIndex_[i] >= 0) { - // edmNew::DetSet::set() internally does an atomic update - det.detSet_.set(*handle_, handle_->item(detIndex_[i])); + det.detSet_ = StripDetset(*handle_, handle_->item(detIndex_[i]), true); det.empty_ = false; // better be false already incAct(); } else { // we should not be here From 7dc55c075ed05128e53b18573f76764c98342d51 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Mon, 12 Jun 2023 18:46:41 +0200 Subject: [PATCH 2/2] Protect assignments of StripDetset() StMeasurementDetSet::getDetSet() The assigments are a critical section and must be protected. --- .../MeasurementDet/src/TkMeasurementDetSet.h | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h index f9dabef9b6c82..cf6882dc09b18 100644 --- a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h +++ b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h @@ -165,7 +165,7 @@ class StMeasurementDetSet { void update(int i, int j) { assert(j >= 0); assert(detSet_[i].empty_); - assert(detSet_[i].ready_); + assert(detSet_[i].ready_ == ReadyState::kUnset); detIndex_[i] = j; detSet_[i].empty_ = false; incReady(); @@ -176,7 +176,7 @@ class StMeasurementDetSet { unsigned int id(int i) const { return conditions().id(i); } int find(unsigned int jd, int i = 0) const { return conditions().find(jd, i); } - bool empty(int i) const { return detSet_[i].empty_; } + bool empty(int i) const { return detSet_[i].empty_.load(std::memory_order_relaxed); } bool isActive(int i) const { return activeThisEvent_[i] && conditions().isActiveThisPeriod(i); } void setEmpty(int i) { @@ -189,7 +189,7 @@ class StMeasurementDetSet { printStat(); for (auto& d : detSet_) { d.empty_ = true; - d.ready_ = true; + d.ready_ = ReadyState::kUnset; } std::fill(detIndex_.begin(), detIndex_.end(), -1); std::fill(activeThisEvent_.begin(), activeThisEvent_.end(), true); @@ -208,7 +208,7 @@ class StMeasurementDetSet { const edm::Handle>& handle() const { return handle_; } // StripDetset & detSet(int i) { return detSet_[i]; } const StripDetset& detSet(int i) const { - if (detSet_[i].ready_) + if (detSet_[i].ready_.load(std::memory_order_acquire) != ReadyState::kSet) getDetSet(i); return detSet_[i].detSet_; } @@ -232,16 +232,24 @@ class StMeasurementDetSet { private: void getDetSet(int i) const { const auto& det = detSet_[i]; - if (detIndex_[i] >= 0) { - det.detSet_ = StripDetset(*handle_, handle_->item(detIndex_[i]), true); - det.empty_ = false; // better be false already - incAct(); - } else { // we should not be here - det.detSet_ = StripDetset(); - det.empty_ = true; + + ReadyState expected = ReadyState::kUnset; + if (det.ready_.compare_exchange_strong(expected, ReadyState::kSetting, std::memory_order_acq_rel)) { + if (detIndex_[i] >= 0) { + det.detSet_ = StripDetset(*handle_, handle_->item(detIndex_[i]), true); + det.empty_.store(false, std::memory_order_relaxed); // better be false already + incAct(); + } else { // we should not be here + det.detSet_ = StripDetset(); + det.empty_.store(true, std::memory_order_relaxed); + } + det.ready_.store(ReadyState::kSet, std::memory_order_release); + incSet(); + } else { + // need to wait + while (ReadyState::kSet != det.ready_.load(std::memory_order_acquire)) { + } } - det.ready_ = false; - incSet(); } friend class MeasurementTrackerImpl; @@ -251,14 +259,15 @@ class StMeasurementDetSet { // Globals, per-event edm::Handle> handle_; + enum class ReadyState : char { kUnset, kSetting, kSet }; + // Helper struct to define only the vector elements as mutable and // to have a vector of atomics without an explicit loop over // elements to set their values struct DetSetHelper { mutable std::atomic empty_ = true; - mutable std::atomic ready_ = true; // to be cleaned - // only thread-safe non-const member functions are called from a const function - CMS_THREAD_SAFE mutable StripDetset detSet_; + mutable std::atomic ready_ = ReadyState::kUnset; // to be cleaned + CMS_THREAD_GUARD(ready_) mutable StripDetset detSet_; }; std::vector activeThisEvent_;