From 82e48ef4c4aacc3df9e2d1a5fd0f1f99b59fa10f Mon Sep 17 00:00:00 2001 From: Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> Date: Thu, 17 Feb 2022 16:28:03 +0100 Subject: [PATCH] Fix unintended sweep copying introduced in !486 --- src/blockforest/CMakeLists.txt | 2 +- src/core/selectable/SelectableObject.h | 33 +++++-- src/timeloop/SweepTimeloop.cpp | 29 +++++-- tests/timeloop/CMakeLists.txt | 18 +++- tests/timeloop/MultipleSweepFailTest.cpp | 64 ++++++++++++++ tests/timeloop/MultipleSweepTest.cpp | 71 ++++++++++++++++ ...r.cpp => TimeloopAndSweepRegisterTest.cpp} | 85 ++++++++++--------- .../timeloop/TimeloopSweepManagementTest.cpp | 82 ++++++++++++++++++ 8 files changed, 327 insertions(+), 57 deletions(-) create mode 100644 tests/timeloop/MultipleSweepFailTest.cpp create mode 100644 tests/timeloop/MultipleSweepTest.cpp rename tests/timeloop/{TimeloopAndSweepRegister.cpp => TimeloopAndSweepRegisterTest.cpp} (53%) create mode 100644 tests/timeloop/TimeloopSweepManagementTest.cpp diff --git a/src/blockforest/CMakeLists.txt b/src/blockforest/CMakeLists.txt index 1498ef455..70ae336fd 100644 --- a/src/blockforest/CMakeLists.txt +++ b/src/blockforest/CMakeLists.txt @@ -5,7 +5,7 @@ mark_as_advanced( WALBERLA_BLOCKFOREST_PRIMITIVE_BLOCKID ) configure_file( CMakeDefs.in.h CMakeDefs.h ) -add_library( blockforest ) +add_library( blockforest ../../tests/timeloop/TimeloopSweepManagementTest.cpp) target_link_libraries( blockforest PUBLIC communication core domain_decomposition stencil ) target_sources( blockforest PRIVATE diff --git a/src/core/selectable/SelectableObject.h b/src/core/selectable/SelectableObject.h index 6a282536d..e41af2944 100644 --- a/src/core/selectable/SelectableObject.h +++ b/src/core/selectable/SelectableObject.h @@ -1,15 +1,15 @@ //====================================================================================================================== // -// This file is part of waLBerla. waLBerla is free software: you can +// This file is part of waLBerla. waLBerla is free software: you can // redistribute it and/or modify it under the terms of the GNU General Public -// License as published by the Free Software Foundation, either version 3 of +// License as published by the Free Software Foundation, either version 3 of // the License, or (at your option) any later version. -// -// waLBerla is distributed in the hope that it will be useful, but WITHOUT -// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +// +// waLBerla is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License // for more details. -// +// // You should have received a copy of the GNU General Public License along // with waLBerla (see COPYING.txt). If not, see <http://www.gnu.org/licenses/>. // @@ -148,6 +148,8 @@ public: iterator end() { return iterator( this, object_.size() ); } const_iterator end() const { return const_iterator( this, object_.size() ); } + size_t getNumberOfMatching( const S& selector ) const; + size_t get( T& object, const S& selector ) const; void get( std::vector<T>& object, const S& selector ) const; @@ -202,7 +204,24 @@ void SelectableObject<T,A,S>::add( const T& object, const A& attributes, const s attributes_.push_back( attributes ); } +//********************************************************************************************************************** +/*! +* Returns the number of objects matching the specified "selector". + */ +//********************************************************************************************************************** +template< typename T, typename A, typename S > +size_t SelectableObject<T,A,S>::getNumberOfMatching( const S& selector ) const { + + std::vector< size_t > index; + select( index, selector ); + + if( !index.empty() ) { + WALBERLA_ASSERT_LESS( index[0], object_.size() ); + } + + return index.size(); +} //********************************************************************************************************************** /*! diff --git a/src/timeloop/SweepTimeloop.cpp b/src/timeloop/SweepTimeloop.cpp index df4e1d5cd..6064efa27 100644 --- a/src/timeloop/SweepTimeloop.cpp +++ b/src/timeloop/SweepTimeloop.cpp @@ -16,6 +16,7 @@ //! \file SweepTimeloop.cpp //! \ingroup timeloop //! \author Martin Bauer <martin.bauer@fau.de> +//! \author Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> // //====================================================================================================================== @@ -47,16 +48,16 @@ void SweepTimeloop::doTimeStep(const Set<SUID> &selectors) // Loop over all blocks for( BlockStorage::iterator bi = blockStorage_.begin(); bi != blockStorage_.end(); ++bi ) { + // ensure that at least one sweep has been registered (regardless of its selector) if( s.sweep.empty() ) { WALBERLA_ABORT("Selecting Sweep " << sweepIt->first << ": " << "No sweep has been registered! Did you only register a BeforeFunction or AfterFunction?" ); } - Sweep selectedSweep; - size_t numSweeps = s.sweep.get(selectedSweep, selectors + bi->getState()); + // ensure that exactly one sweep has been registered that matches the specified selectors + size_t numSweeps = s.sweep.getNumberOfMatching(selectors + bi->getState()); - // ensure that no more than one sweep has been added to a single SweepAdder object if (numSweeps == size_t(0)) { continue; } else { @@ -66,6 +67,8 @@ void SweepTimeloop::doTimeStep(const Set<SUID> &selectors) } } + Sweep * selectedSweep = s.sweep.getUnique( selectors + bi->getState() ); + WALBERLA_LOG_PROGRESS_SECTION() { std::string sweepName; @@ -73,7 +76,7 @@ void SweepTimeloop::doTimeStep(const Set<SUID> &selectors) WALBERLA_LOG_PROGRESS("Running sweep \"" << sweepName << "\" on block " << bi->getId() ); } - (selectedSweep.function_)( bi.get() ); + (selectedSweep->function_)( bi.get() ); } // select and execute after functions @@ -114,11 +117,16 @@ void SweepTimeloop::doTimeStep(const Set<SUID> &selectors, WcTimingPool &timing) for( BlockStorage::iterator bi = blockStorage_.begin(); bi != blockStorage_.end(); ++bi ) { - Sweep selectedSweep; - std::string sweepName; - size_t numSweeps = s.sweep.get(selectedSweep, sweepName, selectors + bi->getState()); + // ensure that at least one sweep has been registered (regardless of its selector) + if( s.sweep.empty() ) + { + WALBERLA_ABORT("Selecting Sweep " << sweepIt->first << ": " << + "No sweep has been registered! Did you only register a BeforeFunction or AfterFunction?" ); + } + + // ensure that exactly one sweep has been registered that matches the specified selectors + size_t numSweeps = s.sweep.getNumberOfMatching(selectors + bi->getState()); - // ensure that no more than one sweep has been added to a single SweepAdder object if (numSweeps == size_t(0)) { continue; } else { @@ -128,11 +136,14 @@ void SweepTimeloop::doTimeStep(const Set<SUID> &selectors, WcTimingPool &timing) } } + std::string sweepName; + Sweep * selectedSweep = s.sweep.getUnique( selectors + bi->getState(), sweepName ); + WALBERLA_LOG_PROGRESS("Running sweep \"" << sweepName << "\" on block " << bi->getId() ); // loop over all blocks timing[sweepName].start(); - (selectedSweep.function_)( bi.get() ); + (selectedSweep->function_)( bi.get() ); timing[sweepName].end(); } diff --git a/tests/timeloop/CMakeLists.txt b/tests/timeloop/CMakeLists.txt index 862c3b648..e39b3223f 100644 --- a/tests/timeloop/CMakeLists.txt +++ b/tests/timeloop/CMakeLists.txt @@ -4,6 +4,20 @@ # ################################################################################################### +waLBerla_compile_test( FILES MultipleSweepFailTest.cpp + DEPENDS blockforest ) +waLBerla_execute_test( NAME MultipleSweepFailTest ) +set_property( TEST MultipleSweepFailTest + PROPERTY WILL_FAIL TRUE ) -waLBerla_compile_test( FILES TimeloopAndSweepRegister.cpp DEPENDS field blockforest ) -waLBerla_execute_test(NAME TimeloopAndSweepRegister ) +waLBerla_compile_test( FILES MultipleSweepTest.cpp + DEPENDS blockforest ) +waLBerla_execute_test( NAME MultipleSweepTest ) + +waLBerla_compile_test( FILES TimeloopSweepManagementTest.cpp + DEPENDS blockforest ) +waLBerla_execute_test( NAME TimeloopSweepManagementTest ) + +waLBerla_compile_test( FILES TimeloopAndSweepRegisterTest.cpp + DEPENDS blockforest field ) +waLBerla_execute_test( NAME TimeloopAndSweepRegisterTest ) \ No newline at end of file diff --git a/tests/timeloop/MultipleSweepFailTest.cpp b/tests/timeloop/MultipleSweepFailTest.cpp new file mode 100644 index 000000000..61a8eb4c4 --- /dev/null +++ b/tests/timeloop/MultipleSweepFailTest.cpp @@ -0,0 +1,64 @@ +//====================================================================================================================== +// +// This file is part of waLBerla. waLBerla is free software: you can +// redistribute it and/or modify it under the terms of the GNU General Public +// License as published by the Free Software Foundation, either version 3 of +// the License, or (at your option) any later version. +// +// waLBerla is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +// for more details. +// +// You should have received a copy of the GNU General Public License along +// with waLBerla (see COPYING.txt). If not, see <http://www.gnu.org/licenses/>. +// +//! \file MultipleSweepFailTest.cpp +//! \ingroup timeloop +//! \author Markus Holzer <markus.holzer@fau.de> +//! \author Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> +//! \brief Test if multiple sweeps (with the same selector) can not be added to the timeloop at once. +//! +//! THIS TEST MUST FAIL! +// +//====================================================================================================================== + +#include "blockforest/Initialization.h" + +#include "core/DataTypes.h" +#include "core/debug/TestSubsystem.h" +#include "core/mpi/Environment.h" + +#include "timeloop/SweepTimeloop.h" + +namespace walberla +{ +namespace timeloop +{ +namespace MultipleSweepFailTest +{ + +int main(int argc, char** argv) +{ + debug::enterTestMode(); + mpi::Environment env(argc, argv); + + const std::shared_ptr< StructuredBlockForest > blockForest = blockforest::createUniformBlockGrid( + uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), real_c(1), false, false, false, false); + + SweepTimeloop timeloop(blockForest, uint_c(1)); + + // empty sweep that does nothing + const auto emptySweep = [](IBlock*) {}; + + // this must fail, as two sweeps are added at once with the same selectors (default selectors: Set<SUID>::emptySet()) + timeloop.add() << Sweep(emptySweep, "Sweep 1") << Sweep(emptySweep, "Sweep 2"); + timeloop.singleStep(); + + return EXIT_SUCCESS; +} +} // namespace MultipleSweepFailTest +} // namespace timeloop +} // namespace walberla + +int main(int argc, char** argv) { return walberla::timeloop::MultipleSweepFailTest::main(argc, argv); } diff --git a/tests/timeloop/MultipleSweepTest.cpp b/tests/timeloop/MultipleSweepTest.cpp new file mode 100644 index 000000000..3ad8248ff --- /dev/null +++ b/tests/timeloop/MultipleSweepTest.cpp @@ -0,0 +1,71 @@ +//====================================================================================================================== +// +// This file is part of waLBerla. waLBerla is free software: you can +// redistribute it and/or modify it under the terms of the GNU General Public +// License as published by the Free Software Foundation, either version 3 of +// the License, or (at your option) any later version. +// +// waLBerla is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +// for more details. +// +// You should have received a copy of the GNU General Public License along +// with waLBerla (see COPYING.txt). If not, see <http://www.gnu.org/licenses/>. +// +//! \file MultipleSweepTest.cpp +//! \ingroup timeloop +//! \author Markus Holzer <markus.holzer@fau.de> +//! \author Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> +//! \brief Test if multiple sweeps (with different selectors) can be added to the timeloop at once. +// +//====================================================================================================================== + +#include "blockforest/Initialization.h" + +#include "core/DataTypes.h" +#include "core/debug/TestSubsystem.h" +#include "core/mpi/Environment.h" + +#include "timeloop/SweepTimeloop.h" + +namespace walberla +{ +namespace timeloop +{ +namespace MultipleSweepTest +{ + +int main(int argc, char** argv) +{ + debug::enterTestMode(); + mpi::Environment env(argc, argv); + + const std::shared_ptr< StructuredBlockForest > blockForest = blockforest::createUniformBlockGrid( + uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), real_c(1), false, false, false, false); + + SweepTimeloop timeloop(blockForest, uint_c(1)); + + // empty sweep that does nothing + const auto emptySweep = [](IBlock*) {}; + + const SUID selector1("selector1"); + const SUID selector2("selector2"); + const SUID selector3("selector3"); + const SUID selector4("selector4"); + + // this must not fail, as two sweeps with the different (required) selectors are added + timeloop.add() << Sweep(emptySweep, "Sweep 1", selector1, Set< SUID >::emptySet()) + << Sweep(emptySweep, "Sweep 2", selector2, Set< SUID >::emptySet()); + timeloop.add() << Sweep(emptySweep, "Sweep 3", selector3, selector4) + << Sweep(emptySweep, "Sweep 4", selector4, selector3); + + timeloop.singleStep(); + + return EXIT_SUCCESS; +} +} // namespace MultipleSweepTest +} // namespace timeloop +} // namespace walberla + +int main(int argc, char** argv) { return walberla::timeloop::MultipleSweepTest::main(argc, argv); } diff --git a/tests/timeloop/TimeloopAndSweepRegister.cpp b/tests/timeloop/TimeloopAndSweepRegisterTest.cpp similarity index 53% rename from tests/timeloop/TimeloopAndSweepRegister.cpp rename to tests/timeloop/TimeloopAndSweepRegisterTest.cpp index 944210821..e16973f87 100644 --- a/tests/timeloop/TimeloopAndSweepRegister.cpp +++ b/tests/timeloop/TimeloopAndSweepRegisterTest.cpp @@ -13,10 +13,11 @@ // You should have received a copy of the GNU General Public License along // with waLBerla (see COPYING.txt). If not, see <http://www.gnu.org/licenses/>. // -//! \file TimeloopAndSweepRegister.cpp +//! \file TimeloopAndSweepRegisterTest.cpp //! \ingroup timeloop //! \author Markus Holzer <markus.holzer@fau.de> -//! \brief test cases that test the registering of Sweeps at timeloop +//! \author Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> +//! \brief Test if registering and execution of sweeps in a timeloop with different selectors works correctly. // //====================================================================================================================== @@ -30,17 +31,21 @@ #include "timeloop/SweepTimeloop.h" -#include <string> #include <vector> -using namespace walberla; +namespace walberla +{ +namespace timeloop +{ +namespace TimeloopAndSweepRegisterTest +{ using Field_T = Field< uint_t, 1 >; auto FieldAdder = [](IBlock* const block, StructuredBlockStorage* const storage) { return new Field_T(storage->getNumberOfXCells(*block), storage->getNumberOfYCells(*block), - storage->getNumberOfZCells(*block), uint_t(0.0), field::fzyx, - make_shared< field::AllocateAligned< uint_t, 64 > >()); + storage->getNumberOfZCells(*block), uint_t(0), field::fzyx, + make_shared< field::AllocateAligned< uint_t, uint_t(64) > >()); }; class Sweep1 @@ -48,82 +53,86 @@ class Sweep1 public: Sweep1(BlockDataID fieldID) : fieldID_(fieldID) {} - void operator()(IBlock* block) + void operator()(IBlock* const block) { - auto field = block->getData< Field_T >(fieldID_); + Field_T* field = block->getData< Field_T >(fieldID_); - for (auto iter = field->begin(); iter != field->end(); ++iter) - *iter += 1; + WALBERLA_FOR_ALL_CELLS(fieldIt, field, { *fieldIt += uint_c(1); }) // WALBERLA_FOR_ALL_CELLS } private: BlockDataID fieldID_; -}; +}; // class Sweep1 class Sweep2 { public: Sweep2(BlockDataID fieldID) : fieldID_(fieldID) {} - void operator()(IBlock* block) + void operator()(IBlock* const block) { - auto field = block->getData< Field_T >(fieldID_); - - for (auto iter = field->begin(); iter != field->end(); ++iter) - *iter += 2; + Field_T* field = block->getData< Field_T >(fieldID_); + WALBERLA_FOR_ALL_CELLS(fieldIt, field, { *fieldIt += uint_c(2); }) // WALBERLA_FOR_ALL_CELLS } private: BlockDataID fieldID_; -}; +}; // class Sweep2 int main(int argc, char** argv) { debug::enterTestMode(); mpi::Environment env(argc, argv); - std::vector<std::string> expectedSequence; - std::vector<std::string> sequence; + std::vector< std::string > expectedSequence; + std::vector< std::string > sequence; - SUID sweepSelect1("Sweep1"); - SUID sweepSelect2("Sweep2"); + const SUID sweepSelect1("Sweep1"); + const SUID sweepSelect2("Sweep2"); - shared_ptr< StructuredBlockForest > blocks = blockforest::createUniformBlockGrid( + const shared_ptr< StructuredBlockForest > blockForest = blockforest::createUniformBlockGrid( uint_c(4), uint_c(2), uint_c(2), uint_c(10), uint_c(10), uint_c(10), real_c(1), false, false, false, false); - BlockDataID fieldID = blocks->addStructuredBlockData< Field_T >(FieldAdder, "Test Field"); + const BlockDataID fieldID = blockForest->addStructuredBlockData< Field_T >(FieldAdder, "Test Field"); - for (auto& block : *blocks) + for (auto& block : *blockForest) { - if (block.getAABB().min()[0] < 20) - block.setState(sweepSelect1); + if (block.getAABB().min()[0] < real_c(20)) { block.setState(sweepSelect1); } else + { block.setState(sweepSelect2); + } } - uint_t timesteps = 10; - SweepTimeloop timeloop(blocks->getBlockStorage(), timesteps); + const uint_t timesteps = uint_c(10); + SweepTimeloop timeloop(blockForest, timesteps); timeloop.add() << Sweep(Sweep1(fieldID), "Sweep 1", sweepSelect1, sweepSelect2); timeloop.add() << Sweep(Sweep2(fieldID), "Sweep 2", sweepSelect2, sweepSelect1); - WcTimingPool timingPool; + timeloop.run(); - timeloop.run(timingPool); - for (auto& block : *blocks) + for (const auto& block : *blockForest) { - auto field = block.getData< Field_T >(fieldID); - if (block.getAABB().min()[0] < 20) + const Field_T* field = block.getData< Field_T >(fieldID); + + if (block.getAABB().min()[0] < real_c(20)) { - for (auto iter = field->begin(); iter != field->end(); ++iter) - WALBERLA_CHECK_EQUAL(*iter, timesteps) + WALBERLA_FOR_ALL_CELLS(fieldIt, field, + { WALBERLA_CHECK_EQUAL(*fieldIt, timesteps); }) // WALBERLA_FOR_ALL_CELLS } else { - for (auto iter = field->begin(); iter != field->end(); ++iter) - WALBERLA_CHECK_EQUAL(*iter, timesteps * 2) + WALBERLA_FOR_ALL_CELLS(fieldIt, field, + { WALBERLA_CHECK_EQUAL(*fieldIt, timesteps * uint_c(2)); }) // WALBERLA_FOR_ALL_CELLS } } - + return EXIT_SUCCESS; } + +} // namespace TimeloopAndSweepRegisterTest +} // namespace timeloop +} // namespace walberla + +int main(int argc, char** argv) { return walberla::timeloop::TimeloopAndSweepRegisterTest::main(argc, argv); } \ No newline at end of file diff --git a/tests/timeloop/TimeloopSweepManagementTest.cpp b/tests/timeloop/TimeloopSweepManagementTest.cpp new file mode 100644 index 000000000..4126a6bd9 --- /dev/null +++ b/tests/timeloop/TimeloopSweepManagementTest.cpp @@ -0,0 +1,82 @@ +//====================================================================================================================== +// +// This file is part of waLBerla. waLBerla is free software: you can +// redistribute it and/or modify it under the terms of the GNU General Public +// License as published by the Free Software Foundation, either version 3 of +// the License, or (at your option) any later version. +// +// waLBerla is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +// for more details. +// +// You should have received a copy of the GNU General Public License along +// with waLBerla (see COPYING.txt). If not, see <http://www.gnu.org/licenses/>. +// +//! \file TimeloopSweepManagementTest.cpp +//! \ingroup timeloop +//! \author Markus Holzer <markus.holzer@fau.de> +//! \author Christoph Schwarzmeier <christoph.schwarzmeier@fau.de> +//! \brief Test if a sweep in the timeloop works on its members rather can re-initializing them in every time step. +// +//====================================================================================================================== + +#include "blockforest/Initialization.h" + +#include "core/DataTypes.h" +#include "core/Environment.h" +#include "core/debug/TestSubsystem.h" + +#include "timeloop/SweepTimeloop.h" + +namespace walberla +{ +namespace timeloop +{ +namespace TimeloopSweepManagementTest +{ + +class CounterSweep +{ + public: + CounterSweep(const std::shared_ptr< uint_t >& externalCounter) + : externalCounter_(externalCounter), internalCounter_(uint_c(0)) + {} + + void operator()(IBlock*) + { + ++(*externalCounter_); + ++internalCounter_; + + WALBERLA_CHECK_EQUAL(*externalCounter_, internalCounter_); + } + + private: + std::shared_ptr< uint_t > externalCounter_; + uint_t internalCounter_; + +}; // class CounterSweep + +int main(int argc, char** argv) +{ + debug::enterTestMode(); + mpi::Environment env(argc, argv); + + shared_ptr< StructuredBlockForest > blockForest = + blockforest::createUniformBlockGrid(uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), uint_c(1), real_c(1)); + + SweepTimeloop timeloop(blockForest, uint_c(10)); + + const std::shared_ptr< uint_t > counter = std::make_shared< uint_t >(uint_c(0)); + + auto TestSweep = Sweep(CounterSweep(counter), "Counter sweep"); + timeloop.add() << TestSweep; + timeloop.run(); + + return EXIT_SUCCESS; +} +} // namespace TimeloopSweepManagementTest +} // namespace timeloop +} // namespace walberla + +int main(int argc, char** argv) { return walberla::timeloop::TimeloopSweepManagementTest::main(argc, argv); } -- GitLab