From d443cda030ae03ee0f3b46acf627daa82958a384 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Del=20Pino?= <stephane.delpino44@gmail.com> Date: Mon, 14 Feb 2022 17:35:30 +0100 Subject: [PATCH] Improve Synchronizer handling - The constructor is now private and can only be called by the SynchronizerManager - Building of exchange info is cleaner: checking if the lists have been build (on-demand) is no more obtained by checking if the lists are empty, one now use a unique pointer that is filled at construction. This strategy is safer (for instance, a processor could be empty which would generate a code crash in the previous implementation). --- src/mesh/Synchronizer.hpp | 74 +++++++++++++++++++++----------- src/mesh/SynchronizerManager.cpp | 2 +- tests/test_Synchronizer.cpp | 31 ++++++------- 3 files changed, 67 insertions(+), 40 deletions(-) diff --git a/src/mesh/Synchronizer.hpp b/src/mesh/Synchronizer.hpp index dc0773ace..fcf5cc740 100644 --- a/src/mesh/Synchronizer.hpp +++ b/src/mesh/Synchronizer.hpp @@ -11,25 +11,27 @@ #include <iostream> #include <map> +#include <memory> #ifdef PUGS_HAS_MPI class Synchronizer { + private: template <ItemType item_type> using ExchangeItemTypeInfo = std::vector<Array<const ItemIdT<item_type>>>; - ExchangeItemTypeInfo<ItemType::cell> m_requested_cell_info; - ExchangeItemTypeInfo<ItemType::cell> m_provided_cell_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::cell>> m_requested_cell_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::cell>> m_provided_cell_info; - ExchangeItemTypeInfo<ItemType::face> m_requested_face_info; - ExchangeItemTypeInfo<ItemType::face> m_provided_face_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::face>> m_requested_face_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::face>> m_provided_face_info; - ExchangeItemTypeInfo<ItemType::edge> m_requested_edge_info; - ExchangeItemTypeInfo<ItemType::edge> m_provided_edge_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::edge>> m_requested_edge_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::edge>> m_provided_edge_info; - ExchangeItemTypeInfo<ItemType::node> m_requested_node_info; - ExchangeItemTypeInfo<ItemType::node> m_provided_node_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::node>> m_requested_node_info; + std::unique_ptr<ExchangeItemTypeInfo<ItemType::node>> m_provided_node_info; template <ItemType item_type> PUGS_INLINE constexpr auto& @@ -68,22 +70,24 @@ class Synchronizer const auto& item_owner = connectivity.template owner<item_type>(); using ItemId = ItemIdT<item_type>; - auto& requested_item_info = this->_getRequestedItemInfo<item_type>(); - requested_item_info = [&]() { + auto& p_requested_item_info = this->_getRequestedItemInfo<item_type>(); + p_requested_item_info = [&]() { std::vector<std::vector<ItemId>> requested_item_vector_info(parallel::size()); for (ItemId item_id = 0; item_id < item_owner.numberOfItems(); ++item_id) { if (const size_t owner = item_owner[item_id]; owner != parallel::rank()) { requested_item_vector_info[owner].emplace_back(item_id); } } - std::vector<Array<const ItemId>> requested_item_info(parallel::size()); + ExchangeItemTypeInfo<item_type> requested_item_info(parallel::size()); for (size_t i_rank = 0; i_rank < parallel::size(); ++i_rank) { const auto& requested_item_vector = requested_item_vector_info[i_rank]; requested_item_info[i_rank] = convert_to_array(requested_item_vector); } - return requested_item_info; + return std::make_unique<ExchangeItemTypeInfo<item_type>>(std::move(requested_item_info)); }(); + auto& requested_item_info = *p_requested_item_info; + Array<unsigned int> local_number_of_requested_values(parallel::size()); for (size_t i_rank = 0; i_rank < parallel::size(); ++i_rank) { local_number_of_requested_values[i_rank] = requested_item_info[i_rank].size(); @@ -114,9 +118,9 @@ class Synchronizer item_number_to_id_correspondance[item_number[item_id]] = item_id; } - auto& provided_item_info = this->_getProvidedItemInfo<item_type>(); - provided_item_info = [&]() { - std::vector<Array<const ItemId>> provided_item_info(parallel::size()); + auto& p_provided_item_info = this->_getProvidedItemInfo<item_type>(); + p_provided_item_info = [&]() { + ExchangeItemTypeInfo<item_type> provided_item_info(parallel::size()); for (size_t i_rank = 0; i_rank < parallel::size(); ++i_rank) { Array<ItemId> provided_item_id_to_rank{local_number_of_values_to_send[i_rank]}; const Array<int>& provided_item_number_to_rank = provided_item_number_list_by_rank[i_rank]; @@ -125,7 +129,7 @@ class Synchronizer } provided_item_info[i_rank] = provided_item_id_to_rank; } - return provided_item_info; + return std::make_unique<ExchangeItemTypeInfo<item_type>>(provided_item_info); }(); } @@ -137,15 +141,20 @@ class Synchronizer using ItemId = ItemIdT<item_type>; - const auto& provided_item_info = this->_getProvidedItemInfo<item_type>(); - const auto& requested_item_info = this->_getRequestedItemInfo<item_type>(); + const auto& p_provided_item_info = this->_getProvidedItemInfo<item_type>(); + const auto& p_requested_item_info = this->_getRequestedItemInfo<item_type>(); - Assert(requested_item_info.size() == provided_item_info.size()); + Assert(static_cast<bool>(p_provided_item_info) == static_cast<bool>(p_requested_item_info)); - if (provided_item_info.size() == 0) { + if (not p_provided_item_info) { this->_buildSynchronizeInfo<ConnectivityType, item_type>(connectivity); } + const auto& provided_item_info = *p_provided_item_info; + const auto& requested_item_info = *p_requested_item_info; + + Assert(requested_item_info.size() == provided_item_info.size()); + std::vector<Array<const DataType>> provided_data_list(parallel::size()); for (size_t i_rank = 0; i_rank < parallel::size(); ++i_rank) { const Array<const ItemId>& provided_item_info_to_rank = provided_item_info[i_rank]; @@ -181,15 +190,20 @@ class Synchronizer using ItemId = ItemIdT<item_type>; - const auto& provided_item_info = this->_getProvidedItemInfo<item_type>(); - const auto& requested_item_info = this->_getRequestedItemInfo<item_type>(); + const auto& p_provided_item_info = this->_getProvidedItemInfo<item_type>(); + const auto& p_requested_item_info = this->_getRequestedItemInfo<item_type>(); - Assert(requested_item_info.size() == provided_item_info.size()); + Assert(static_cast<bool>(p_provided_item_info) == static_cast<bool>(p_requested_item_info)); - if (provided_item_info.size() == 0) { + if (not p_provided_item_info) { this->_buildSynchronizeInfo<ConnectivityType, item_type>(connectivity); } + const auto& provided_item_info = *p_provided_item_info; + const auto& requested_item_info = *p_requested_item_info; + + Assert(requested_item_info.size() == provided_item_info.size()); + const size_t size_of_arrays = item_array.sizeOfArrays(); std::vector<Array<const DataType>> provided_data_list(parallel::size()); @@ -286,6 +300,12 @@ class Synchronizer } } + Synchronizer(const Synchronizer&) = delete; + Synchronizer(Synchronizer&&) = delete; + + private: + friend class SynchronizerManager; + PUGS_INLINE Synchronizer() { @@ -308,6 +328,12 @@ class Synchronizer synchronize(ItemArray<DataType, item_type, ConnectivityPtr>&) {} + Synchronizer(const Synchronizer&) = delete; + Synchronizer(Synchronizer&&) = delete; + + private: + friend class SynchronizerManager; + PUGS_INLINE Synchronizer() { diff --git a/src/mesh/SynchronizerManager.cpp b/src/mesh/SynchronizerManager.cpp index a72191fd3..45f2d17a4 100644 --- a/src/mesh/SynchronizerManager.cpp +++ b/src/mesh/SynchronizerManager.cpp @@ -45,7 +45,7 @@ SynchronizerManager::getConnectivitySynchronizer(const IConnectivity* connectivi connectivity_synchronizer != m_connectivity_synchronizer_map.end()) { return (*connectivity_synchronizer->second); } else { - std::shared_ptr synchronizer = std::make_shared<Synchronizer>(); + std::shared_ptr<Synchronizer> synchronizer(new Synchronizer); m_connectivity_synchronizer_map[connectivity] = synchronizer; return *synchronizer; } diff --git a/tests/test_Synchronizer.cpp b/tests/test_Synchronizer.cpp index 6a4a9b52a..8cc0ea661 100644 --- a/tests/test_Synchronizer.cpp +++ b/tests/test_Synchronizer.cpp @@ -5,6 +5,7 @@ #include <mesh/Connectivity.hpp> #include <mesh/Mesh.hpp> #include <mesh/Synchronizer.hpp> +#include <mesh/SynchronizerManager.hpp> #include <utils/pugs_config.hpp> // clazy:excludeall=non-pod-global-static @@ -57,7 +58,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(node_value, node_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(node_value); REQUIRE(is_same_item_value(node_value, node_value_ref)); @@ -82,7 +83,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(edge_value, edge_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(edge_value); REQUIRE(is_same_item_value(edge_value, edge_value_ref)); @@ -107,7 +108,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(face_value, face_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(face_value); REQUIRE(is_same_item_value(face_value, face_value_ref)); @@ -132,7 +133,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(cell_value, cell_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_value); REQUIRE(is_same_item_value(cell_value, cell_value_ref)); @@ -163,7 +164,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_array(cell_array, cell_array_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_array); REQUIRE(is_same_item_array(cell_array, cell_array_ref)); @@ -196,7 +197,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(node_value, node_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(node_value); REQUIRE(is_same_item_value(node_value, node_value_ref)); @@ -221,7 +222,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(edge_value, edge_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(edge_value); REQUIRE(is_same_item_value(edge_value, edge_value_ref)); @@ -246,7 +247,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(face_value, face_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(face_value); REQUIRE(is_same_item_value(face_value, face_value_ref)); @@ -271,7 +272,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(cell_value, cell_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_value); REQUIRE(is_same_item_value(cell_value, cell_value_ref)); @@ -302,7 +303,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_array(cell_array, cell_array_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_array); REQUIRE(is_same_item_array(cell_array, cell_array_ref)); @@ -335,7 +336,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(node_value, node_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(node_value); REQUIRE(is_same_item_value(node_value, node_value_ref)); @@ -360,7 +361,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(edge_value, edge_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(edge_value); REQUIRE(is_same_item_value(edge_value, edge_value_ref)); @@ -385,7 +386,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(face_value, face_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(face_value); REQUIRE(is_same_item_value(face_value, face_value_ref)); @@ -410,7 +411,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_value(cell_value, cell_value_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_value); REQUIRE(is_same_item_value(cell_value, cell_value_ref)); @@ -441,7 +442,7 @@ TEST_CASE("Synchronizer", "[mesh]") REQUIRE(not is_same_item_array(cell_array, cell_array_ref)); } - Synchronizer synchronizer; + Synchronizer& synchronizer = SynchronizerManager::instance().getConnectivitySynchronizer(&connectivity); synchronizer.synchronize(cell_array); REQUIRE(is_same_item_array(cell_array, cell_array_ref)); -- GitLab