Skip to content

Commit

Permalink
GeometrySplit functor isn't threadsafe (#1607)
Browse files Browse the repository at this point in the history
### 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 <[email protected]>
  • Loading branch information
3 people authored Sep 16, 2024
1 parent 0e1566c commit a894644
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 8 deletions.
2 changes: 2 additions & 0 deletions src/algorithms/meta/CollectionCollector.h
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

};
Expand Down
7 changes: 7 additions & 0 deletions src/algorithms/meta/SubDivideCollection.h
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

};

};
Expand Down
20 changes: 12 additions & 8 deletions src/algorithms/meta/SubDivideFunctors.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Expand All @@ -66,19 +70,19 @@ 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));
}
}

std::vector<std::vector<long int>> m_ids;
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;

};

Expand Down
1 change: 1 addition & 0 deletions src/factories/meta/CollectionCollector_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace eicrecon {
}
typename T::collection_type* merged_collection = m_output().get();
m_algo->process(in_collections, merged_collection);

};

};
Expand Down

0 comments on commit a894644

Please sign in to comment.