Merge pull request #48266 from fwyzard/fix_CTPPS_crash_151x

Improve sorting of `DetSetVector<CTPPSPixelRecHit>`

Author: cmsbuild

CondFormats/PPSObjects/interface/CTPPSPixelIndices.h

@@ -65,7 +65,7 @@ namespace rpixValues {
   constexpr int ROCSizeInY = 52;  // ROC col size in pixels
   // Default DET barrel size
   constexpr int defaultDetSizeInX = 160;  // Det row size in pixels (2 ROCs)
-  constexpr int defaultDetSizeInY = 156;  //  Det col size in pixels (3 ROCs)
+  constexpr int defaultDetSizeInY = 156;  // Det col size in pixels (3 ROCs)
 
   // Check the limits
   constexpr bool CTPPS_CHECK_LIMITS = true;
@@ -168,13 +168,13 @@ class CTPPSPixelIndices {
     // The transformation depends on the ROC-ID
     if (rocId >= 0 && rocId < 3) {
       row = 159 - rowROC;
-
       col = (rocId + 1) * rpixValues::ROCSizeInY - colROC - 1;
     } else if (rocId >= 3 && rocId < 6) {
       row = rowROC;
-
       col = (5 - rocId) * rpixValues::ROCSizeInY + colROC;
     } else {
+      row = -1;
+      col = -1;
       edm::LogError("RPix") << "CTPPSPixelIndices: wrong ROC ID " << rocId;
       return -1;
     }
@@ -188,6 +188,7 @@ class CTPPSPixelIndices {
 
     return 0;
   }
+
   //**************************************************************************
   // Transform from the module indixes to the ROC indices.
   // col, row - indices in the Module

DataFormats/CTPPSReco/interface/CTPPSPixelRecHit.h

@@ -1,3 +1,6 @@
+#ifndef DataFormats_CTPPSReco_interface_CTPPSPixelRecHit_h
+#define DataFormats_CTPPSReco_interface_CTPPSPixelRecHit_h
+
 /*
  *
  * This is a part of CTPPS offline software.
@@ -6,11 +9,11 @@
  *
  */
 
-#ifndef DataFormats_CTPPSReco_CTPPSPixelRecHit_H
-#define DataFormats_CTPPSReco_CTPPSPixelRecHit_H
+#include <cassert>
 
 #include "DataFormats/GeometrySurface/interface/LocalError.h"
 #include "DataFormats/GeometryVector/interface/LocalPoint.h"
+#include "FWCore/Utilities/interface/isFinite.h"
 
 // Reconstructed hits in CTPPS Pixel detector
 
@@ -37,19 +40,21 @@ class CTPPSPixelRecHit {
         clusterSizeRow_(rowsize),
         clusterSizeCol_(colsize) {}
 
-  inline LocalPoint point() const { return thePoint_; }
-  inline LocalError error() const { return theError_; }
+  LocalPoint point() const { return thePoint_; }
+  LocalError error() const { return theError_; }
+
+  bool isOnEdge() const { return isOnEdge_; }
+  bool hasBadPixels() const { return hasBadPixels_; }
+  bool spanTwoRocs() const { return spanTwoRocs_; }
 
-  inline bool isOnEdge() const { return isOnEdge_; }
-  inline bool hasBadPixels() const { return hasBadPixels_; }
-  inline bool spanTwoRocs() const { return spanTwoRocs_; }
+  unsigned int minPixelRow() const { return minPixelRow_; }
+  unsigned int minPixelCol() const { return minPixelCol_; }
 
-  inline unsigned int minPixelRow() const { return minPixelRow_; }
-  inline unsigned int minPixelCol() const { return minPixelCol_; }
+  unsigned int clusterSize() const { return clusterSize_; }
+  unsigned int clusterSizeRow() const { return clusterSizeRow_; }
+  unsigned int clusterSizeCol() const { return clusterSizeCol_; }
 
-  inline unsigned int clusterSize() const { return clusterSize_; }
-  inline unsigned int clusterSizeRow() const { return clusterSizeRow_; }
-  inline unsigned int clusterSizeCol() const { return clusterSizeCol_; }
+  float sort_key() const { return thePoint_.mag2(); }
 
 private:
   LocalPoint thePoint_;
@@ -67,6 +72,12 @@ class CTPPSPixelRecHit {
   unsigned int clusterSizeCol_;
 };
 
-inline bool operator<(CTPPSPixelRecHit& a, CTPPSPixelRecHit& b) { return (a.point().mag() < b.point().mag()); };
+inline bool operator<(CTPPSPixelRecHit const& a, CTPPSPixelRecHit const& b) {
+  float a_key = a.sort_key();
+  float b_key = b.sort_key();
+  assert(edm::isFinite(a_key));
+  assert(edm::isFinite(b_key));
+  return a_key < b_key;
+}
 
-#endif
+#endif  // DataFormats_CTPPSReco_interface_CTPPSPixelRecHit_h

DataFormats/Common/interface/DetSetVector.h

@@ -37,22 +37,71 @@ behavior (usually a core dump).
 
 #include <algorithm>
 #include <iterator>
+#include <numeric>
+#include <type_traits>
 #include <vector>
 
-#include <type_traits>
-#include "boost/concept_check.hpp"
+#include <boost/concept_check.hpp>
 
+#include "DataFormats/Common/interface/BoolCache.h"
 #include "DataFormats/Common/interface/CMS_CLASS_VERSION.h"
 #include "DataFormats/Common/interface/DetSet.h"
 #include "DataFormats/Common/interface/FillView.h"
 #include "DataFormats/Common/interface/Ref.h"
 #include "DataFormats/Common/interface/traits.h"
-
 #include "FWCore/Utilities/interface/EDMException.h"
 
-#include "DataFormats/Common/interface/BoolCache.h"
-
 namespace edm {
+
+  namespace detail {
+
+    // from https://devblogs.microsoft.com/oldnewthing/20170104-00/?p=95115
+
+    template <typename Iter1, typename Iter2>
+    void apply_permutation(Iter1 first, Iter1 last, Iter2 indices) {
+      using T = std::iterator_traits<Iter1>::value_type;
+      using Diff = std::iterator_traits<Iter2>::value_type;
+      Diff length = last - first;
+      for (Diff i = 0; i < length; i++) {
+        Diff current = i;
+        if (i != indices[current]) {
+          T t{std::move(first[i])};
+          while (i != indices[current]) {
+            Diff next = indices[current];
+            first[current] = std::move(first[next]);
+            indices[current] = current;
+            current = next;
+          }
+          first[current] = std::move(t);
+          indices[current] = current;
+        }
+      }
+    }
+
+    // from https://devblogs.microsoft.com/oldnewthing/20170106-00/?p=95135
+
+    template <typename Iter, typename UnaryOperation, typename Compare>
+    void sort_by_key(Iter first, Iter last, UnaryOperation op, Compare comp) {
+      using Diff = std::iterator_traits<Iter>::difference_type;
+      using T = std::iterator_traits<Iter>::value_type;
+      using Key = decltype(op(std::declval<T>()));
+      Diff length = std::distance(first, last);
+      std::vector<Key> keys;
+      keys.reserve(length);
+      std::transform(first, last, std::back_inserter(keys), [&](T& t) { return op(t); });
+      std::vector<Diff> indices(length);
+      std::iota(indices.begin(), indices.end(), static_cast<Diff>(0));
+      std::sort(indices.begin(), indices.end(), [&](Diff a, Diff b) { return comp(keys[a], keys[b]); });
+      apply_permutation(first, last, indices.begin());
+    }
+
+    template <typename Iter, typename UnaryOperation>
+    void sort_by_key(Iter first, Iter last, UnaryOperation op) {
+      sort_by_key(first, last, op, std::less<>());
+    }
+
+  }  // namespace detail
+
   class ProductID;
 
   //------------------------------------------------------------
@@ -340,7 +389,11 @@ namespace edm {
     for (; i != e; ++i) {
       i->data.shrink_to_fit();
       // sort the Detset pointed to by
-      std::sort(i->data.begin(), i->data.end());
+      if constexpr (requires(const T& t) { t.sort_key(); }) {
+        edm::detail::sort_by_key(i->data.begin(), i->data.end(), [](const T& t) { return t.sort_key(); });
+      } else {
+        std::sort(i->data.begin(), i->data.end());
+      }
     }
   }
 

DataFormats/Common/test/BuildFile.xml

@@ -14,8 +14,8 @@
 <bin name="testDataFormatsCommon" file="testRunner.cpp,testOwnVector.cc,testOneToOneAssociation.cc,testValueMap.cc,testOneToManyAssociation.cc,testAssociationVector.cc,testAssociationNew.cc,testValueMapNew.cc,testSortedCollection.cc,testRangeMap.cc,ref_t.cppunit.cc,DetSetRefVector_t.cppunit.cc,reftobasevector_t.cppunit.cc,cloningptr_t.cppunit.cc,ptr_t.cppunit.cc,ptrvector_t.cppunit.cc,containermask_t.cppunit.cc,reftobaseprod_t.cppunit.cc,handle_t.cppunit.cc">
 </bin>
 
-<bin name="testDataFormatsCommonCatch2" file="DetSetVector_t.cpp,exDSTV.cpp,exTrie.cpp,OwnVector_t.cpp,Wrapper_t.cpp,testBoostRange.cpp,reftobase_t.cpp,">
-<use name="catch2"/>
+<bin name="testDataFormatsCommonCatch2" file="DetSetVector_t.cpp,DetSetVectorWithKey_t.cpp,exDSTV.cpp,exTrie.cpp,OwnVector_t.cpp,Wrapper_t.cpp,testBoostRange.cpp,reftobase_t.cpp">
+  <use name="catch2"/>
 </bin>
 
 <bin file="DictionaryTools_t.cpp">