Merge pull request #41936 from makortel/stMeasurementDetSetAssignment

Protect assignments of StripDetset() in StMeasurementDetSet::getDetSet()

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 {

DataFormats/Common/interface/DetSetVectorNew.h

@@ -183,14 +183,10 @@ namespace edmNew {
       IterHelp() : m_v(nullptr), m_update(false) {}
       IterHelp(DetSetVector<T> 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<T> const* m_v;
-      mutable result_type m_detset;
       bool m_update;
     };
 

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++);

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<edmNew::DetSetVector<SiStripCluster>>& 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,17 +232,24 @@ class StMeasurementDetSet {
 private:
   void getDetSet(int i) const {
     const auto& det = detSet_[i];
-    if (detIndex_[i] >= 0) {
-      // edmNew::DetSet<T>::set() internally does an atomic update
-      det.detSet_.set(*handle_, handle_->item(detIndex_[i]));
-      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;
@@ -252,14 +259,15 @@ class StMeasurementDetSet {
   // Globals, per-event
   edm::Handle<edmNew::DetSetVector<SiStripCluster>> 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<bool> empty_ = true;
-    mutable std::atomic<bool> 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<ReadyState> ready_ = ReadyState::kUnset;  // to be cleaned
+    CMS_THREAD_GUARD(ready_) mutable StripDetset detSet_;
   };
 
   std::vector<bool> activeThisEvent_;