From a8946446ca3b37a0d9201c792925281a765ef402 Mon Sep 17 00:00:00 2001
From: Simon Gardner <simon.gardner@glasgow.ac.uk>
Date: Mon, 16 Sep 2024 16:00:57 +0100
Subject: [PATCH] GeometrySplit functor isn't threadsafe (#1607)

### Briefly, what does this PR introduce?
Fixes issues with the initialization of the GeometrySplit functor across
threads.

Currently only the std::once_flag is a shared pointer and none of the
other variables which are being initialized. This meant that when copied
to other threads the is_init flag was set but the decoder and geometry
divisions had not been so the loop over the detector ids was always
empty. This resulted in an apparent variable decrease in the efficiency
of the Low-Q2 tagger depending on how many threads were asked for.

Unit tests for all of the meta factories and functors should be added as
an issue to try head this off in the future but outwith this fix.

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No
### Does this PR change default behavior?
All of the Low-Q2 Tagger hits will be converted into clusters rather
than just those run on the first thread to call the functor.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 src/algorithms/meta/CollectionCollector.h     |  2 ++
 src/algorithms/meta/SubDivideCollection.h     |  7 +++++++
 src/algorithms/meta/SubDivideFunctors.h       | 20 +++++++++++--------
 .../meta/CollectionCollector_factory.h        |  1 +
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/algorithms/meta/CollectionCollector.h b/src/algorithms/meta/CollectionCollector.h
index f6806366b8..ccafe3fc36 100644
--- a/src/algorithms/meta/CollectionCollector.h
+++ b/src/algorithms/meta/CollectionCollector.h
@@ -42,6 +42,8 @@ namespace eicrecon {
           out_collection->push_back(hit);
         }
       }
+      //Log how many hits were collected from N input collections
+      this->debug("Collected {} hits from {} input collections", out_collection->size(), in_collections.size());
     }
 
   };
diff --git a/src/algorithms/meta/SubDivideCollection.h b/src/algorithms/meta/SubDivideCollection.h
index de12a3712d..8329b930d1 100644
--- a/src/algorithms/meta/SubDivideCollection.h
+++ b/src/algorithms/meta/SubDivideCollection.h
@@ -56,6 +56,13 @@ namespace eicrecon {
 
           }
 
+          //Log how the hits were divided between the collection names
+          this->debug("Divided {} hits between {} collections", entries->size(), subdivided_entries.size());
+          //How many hits in each output collection
+          for (size_t i = 0; i < subdivided_entries.size(); i++) {
+            this->debug("Collection {} takes {} hits", i, subdivided_entries[i]->size());
+          }
+
         };
 
   };
diff --git a/src/algorithms/meta/SubDivideFunctors.h b/src/algorithms/meta/SubDivideFunctors.h
index 090b54ad89..a6bb89167d 100644
--- a/src/algorithms/meta/SubDivideFunctors.h
+++ b/src/algorithms/meta/SubDivideFunctors.h
@@ -40,7 +40,10 @@ class GeometrySplit {
 public:
 
     GeometrySplit(std::vector<std::vector<long int>> ids, std::string readout, std::vector<std::string> divisions)
-    : m_ids(ids), m_readout(readout), m_divisions(divisions){};
+    : m_ids(ids), m_readout(readout), m_divisions(divisions),
+      is_init(std::make_shared<std::once_flag>()),
+      m_id_dec(std::make_shared<dd4hep::DDSegmentation::BitFieldCoder*>()),
+      m_div_ids(std::make_shared<std::vector<size_t>>()) {};
 
     template <typename T>
     std::vector<int> operator()(T& instance) const {
@@ -51,9 +54,10 @@ class GeometrySplit {
         //Check which detector division to put the hit into
         auto cellID = instance.getCellID();
         std::vector<long int> det_ids;
-        for(auto d : m_div_ids){
-            det_ids.push_back(m_id_dec->get(cellID, d));
+        for(auto d : *m_div_ids){
+            det_ids.push_back((*m_id_dec)->get(cellID, d));
         }
+
         auto index = std::find(m_ids.begin(),m_ids.end(),det_ids);
 
         std::vector<int> ids;
@@ -66,9 +70,9 @@ class GeometrySplit {
 private:
 
     void init() const {
-        m_id_dec = algorithms::GeoSvc::instance().detector()->readout(m_readout).idSpec().decoder();
+        *m_id_dec = algorithms::GeoSvc::instance().detector()->readout(m_readout).idSpec().decoder();
         for (auto d : m_divisions){
-            m_div_ids.push_back(m_id_dec->index(d));
+            m_div_ids->push_back((*m_id_dec)->index(d));
         }
     }
 
@@ -76,9 +80,9 @@ class GeometrySplit {
     std::vector<std::string> m_divisions;
     std::string m_readout;
 
-    mutable std::shared_ptr<std::once_flag> is_init = std::make_shared<std::once_flag>();
-    mutable dd4hep::DDSegmentation::BitFieldCoder* m_id_dec;
-    mutable std::vector<size_t> m_div_ids;
+    std::shared_ptr<std::once_flag> is_init;
+    std::shared_ptr<dd4hep::DDSegmentation::BitFieldCoder*> m_id_dec;
+    std::shared_ptr<std::vector<size_t>> m_div_ids;
 
 };
 
diff --git a/src/factories/meta/CollectionCollector_factory.h b/src/factories/meta/CollectionCollector_factory.h
index df5f6da39c..50d484b311 100644
--- a/src/factories/meta/CollectionCollector_factory.h
+++ b/src/factories/meta/CollectionCollector_factory.h
@@ -35,6 +35,7 @@ namespace eicrecon {
         }
         typename T::collection_type* merged_collection = m_output().get();
         m_algo->process(in_collections, merged_collection);
+
     };
 
     };