From cd247e5fa89ec0a71caf873199a71ff12022610d Mon Sep 17 00:00:00 2001 From: Sanghyun Ko Date: Thu, 18 Jul 2024 09:49:45 +0200 Subject: [PATCH 1/3] speedup lookup of GeoHandler (issue #1291) --- DDCore/include/DD4hep/GeoHandler.h | 2 ++ DDCore/src/GeoHandler.cpp | 19 +++++++++++++++---- DDG4/src/Geant4Converter.cpp | 1 + 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/DDCore/include/DD4hep/GeoHandler.h b/DDCore/include/DD4hep/GeoHandler.h index 31d507f17..473989529 100644 --- a/DDCore/include/DD4hep/GeoHandler.h +++ b/DDCore/include/DD4hep/GeoHandler.h @@ -89,6 +89,7 @@ namespace dd4hep { protected: bool m_propagateRegions { false }; std::map >* m_data { nullptr }; + std::map >* m_set_data { nullptr }; std::map >* m_daughters { nullptr }; /// Internal helper to collect geometry information from traversal GeoHandler& i_collect(const TGeoNode* parent, @@ -109,6 +110,7 @@ namespace dd4hep { GeoHandler(); /// Initializing constructor GeoHandler(std::map >* ptr, + std::map >* ptr_set, std::map >* daus = nullptr); /// Default destructor virtual ~GeoHandler(); diff --git a/DDCore/src/GeoHandler.cpp b/DDCore/src/GeoHandler.cpp index ca443d062..000705bf2 100644 --- a/DDCore/src/GeoHandler.cpp +++ b/DDCore/src/GeoHandler.cpp @@ -55,12 +55,14 @@ namespace { /// Default constructor detail::GeoHandler::GeoHandler() { m_data = new std::map >(); + m_set_data = new std::map >(); } /// Initializing constructor detail::GeoHandler::GeoHandler(std::map >* ptr, - std::map >* daus) - : m_data(ptr), m_daughters(daus) + std::map >* ptr_set, + std::map >* daus) + : m_data(ptr), m_set_data(ptr_set), m_daughters(daus) { } @@ -68,12 +70,20 @@ detail::GeoHandler::GeoHandler(std::map >* ptr detail::GeoHandler::~GeoHandler() { if (m_data) delete m_data; + if (m_set_data) + delete m_set_data; + m_data = nullptr; + m_set_data = nullptr; } std::map >* detail::GeoHandler::release() { std::map >* d = m_data; m_data = nullptr; + + delete m_set_data; + m_set_data = nullptr; + return d; } @@ -88,6 +98,7 @@ detail::GeoHandler& detail::GeoHandler::collect(DetElement element) { DetElement par = element.parent(); TGeoNode* par_node = par.isValid() ? par.placement().ptr() : nullptr; m_data->clear(); + m_set_data->clear(); return i_collect(par_node, element.placement().ptr(), 0, Region(), LimitSet()); } @@ -95,6 +106,7 @@ detail::GeoHandler& detail::GeoHandler::collect(DetElement element, GeometryInfo DetElement par = element.parent(); TGeoNode* par_node = par.isValid() ? par.placement().ptr() : nullptr; m_data->clear(); + m_set_data->clear(); i_collect(par_node, element.placement().ptr(), 0, Region(), LimitSet()); for ( auto i = m_data->rbegin(); i != m_data->rend(); ++i ) { const auto& mapped = (*i).second; @@ -150,8 +162,7 @@ detail::GeoHandler& detail::GeoHandler::i_collect(const TGeoNode* /* parent */, } } /// Collect the hierarchy of placements - auto& vec = (*m_data)[level]; - if(std::find(vec.begin(), vec.end(), current) == vec.end()) { + if ( (*m_set_data)[level].emplace(current).second ) { (*m_data)[level].push_back(current); } int num = nodes ? nodes->GetEntriesFast() : 0; diff --git a/DDG4/src/Geant4Converter.cpp b/DDG4/src/Geant4Converter.cpp index 3b8a6d5e5..57531caf7 100644 --- a/DDG4/src/Geant4Converter.cpp +++ b/DDG4/src/Geant4Converter.cpp @@ -1668,6 +1668,7 @@ Geant4Converter& Geant4Converter::create(DetElement top) { World wrld = top.world(); m_data->clear(); + m_set_data->clear(); m_daughters = &daughters; geo.manager = &wrld.detectorDescription().manager(); this->collect(top, geo); From e6ec5ca8cd3f191f810e945144b06cd1402c8d16 Mon Sep 17 00:00:00 2001 From: Sanghyun Ko Date: Mon, 22 Jul 2024 09:43:42 +0200 Subject: [PATCH 2/3] add more comments to the GeoHandler --- DDCore/include/DD4hep/GeoHandler.h | 4 ++++ DDCore/src/GeoHandler.cpp | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/DDCore/include/DD4hep/GeoHandler.h b/DDCore/include/DD4hep/GeoHandler.h index 473989529..85031a171 100644 --- a/DDCore/include/DD4hep/GeoHandler.h +++ b/DDCore/include/DD4hep/GeoHandler.h @@ -88,8 +88,12 @@ namespace dd4hep { protected: bool m_propagateRegions { false }; + + /// actual container with std::vector (preserves order) std::map >* m_data { nullptr }; + /// redundant container with std::set (for lookup purpose) std::map >* m_set_data { nullptr }; + std::map >* m_daughters { nullptr }; /// Internal helper to collect geometry information from traversal GeoHandler& i_collect(const TGeoNode* parent, diff --git a/DDCore/src/GeoHandler.cpp b/DDCore/src/GeoHandler.cpp index 000705bf2..6851009aa 100644 --- a/DDCore/src/GeoHandler.cpp +++ b/DDCore/src/GeoHandler.cpp @@ -78,9 +78,13 @@ detail::GeoHandler::~GeoHandler() { } std::map >* detail::GeoHandler::release() { + /// release the std::vector geometry container (preserves order) std::map >* d = m_data; m_data = nullptr; + /// the std::set container (for lookup purpose) is not needed anymore, so delete it + /// the container is always present since the call of the constructor + /// we never expect to call release() twice (will release nullptr) delete m_set_data; m_set_data = nullptr; @@ -162,6 +166,7 @@ detail::GeoHandler& detail::GeoHandler::i_collect(const TGeoNode* /* parent */, } } /// Collect the hierarchy of placements + /// perform lookup using std::set::emplace (faster than std::find for the large number of geometries) if ( (*m_set_data)[level].emplace(current).second ) { (*m_data)[level].push_back(current); } From 7967bdfc64246f4d698616a9469e46ffbdad49c4 Mon Sep 17 00:00:00 2001 From: Sanghyun Ko <37464245+SanghyunKo@users.noreply.github.com> Date: Mon, 22 Jul 2024 17:38:33 +0900 Subject: [PATCH 3/3] Adopt a suggestion for improving comments Co-authored-by: Andre Sailer --- DDCore/src/GeoHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DDCore/src/GeoHandler.cpp b/DDCore/src/GeoHandler.cpp index 6851009aa..07f7a4521 100644 --- a/DDCore/src/GeoHandler.cpp +++ b/DDCore/src/GeoHandler.cpp @@ -166,7 +166,7 @@ detail::GeoHandler& detail::GeoHandler::i_collect(const TGeoNode* /* parent */, } } /// Collect the hierarchy of placements - /// perform lookup using std::set::emplace (faster than std::find for the large number of geometries) + /// perform lookup using std::set::emplace (faster than std::find for very large number of volumes) if ( (*m_set_data)[level].emplace(current).second ) { (*m_data)[level].push_back(current); }