From 6890e8db711c5c55cf243a74136e9c6108b41b38 Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Fri, 4 Sep 2015 21:31:39 +0200 Subject: [PATCH 1/3] Renames ParallelPreconditionerDeleter and moves it to a separate header. The class is not limited to parallel preconditioners. To reflect this we rename it to AdditionalObjectDeleter. As it will also be used for the parallel interleaved version we move the class to a separate header. --- opm/autodiff/AdditionalObjectDeleter.hpp | 58 ++++++++++++++++++++++++ opm/autodiff/CPRPreconditioner.hpp | 34 ++------------ 2 files changed, 63 insertions(+), 29 deletions(-) create mode 100644 opm/autodiff/AdditionalObjectDeleter.hpp diff --git a/opm/autodiff/AdditionalObjectDeleter.hpp b/opm/autodiff/AdditionalObjectDeleter.hpp new file mode 100644 index 000000000..985bf741f --- /dev/null +++ b/opm/autodiff/AdditionalObjectDeleter.hpp @@ -0,0 +1,58 @@ + /* + Copyright 2015 Dr. Blatt - HPC-Simulation-Software & Services + Copyright 2015 Statoil AS + + This file is part of the Open Porous Media project (OPM). + + OPM 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. + + OPM 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 OPM. If not, see . +*/ +#ifndef OPM_ADDITIONALOBJECTDELETER_HEADER_INCLUDED +#define OPM_ADDITIONALOBJECTDELETER_HEADER_INCLUDED +namespace Opm +{ +//! \brief A custom deleter that will delete an additional object, too. +//! +//! In dune-istl most parallel preconditioners hold a reference to +//! a sequential preconditioner. +//! In CPRPreconditioner and NewtonIterationBlackoilInterleaved we use unique_ptr +//! for the memory management. +//! Ergo we need to construct the sequential preconditioner with new and +//! make sure that it gets deleted together with the enclosing parallel +//! preconditioner. Therefore this deleter stores a pointer to it and deletes +//! it during destruction. +//! \tparam The type of the additional object to be deleted. +template +class AdditionalObjectDeleter +{ +public: + //! \brief empty constructor. + AdditionalObjectDeleter() + : additional_object_() + {} + //! \brief Constructor taking the object that needs to deleted. + AdditionalObjectDeleter(T& additional_object) + : additional_object_(&additional_object){} + //! \brief Delete an object and the additional one. + template + void operator()(T1* pt) + { + delete pt; + delete additional_object_; + } +private: + T* additional_object_; +}; + +} +#endif diff --git a/opm/autodiff/CPRPreconditioner.hpp b/opm/autodiff/CPRPreconditioner.hpp index 91a5962de..816ba0cee 100644 --- a/opm/autodiff/CPRPreconditioner.hpp +++ b/opm/autodiff/CPRPreconditioner.hpp @@ -47,36 +47,12 @@ #include #include +#include +#include namespace Opm { namespace { -//! \brief A custom deleter for the parallel preconditioners. -//! -//! In dune-istl they hold a reference to the sequential preconditioner. -//! In CPRPreconditioner we use unique_ptr for the memory management. -//! Ergo we need to construct the sequential preconditioner with new and -//! make sure that it gets deleted together with the enclosing parallel -//! preconditioner. Therefore this deleter stores a pointer to it and deletes -//! it during destruction. -template -class ParallelPreconditionerDeleter -{ -public: - ParallelPreconditionerDeleter() - : ilu_() - {} - ParallelPreconditionerDeleter(PREC& ilu) - : ilu_(&ilu){} - template - void operator()(T* pt) - { - delete pt; - delete ilu_; - } -private: - PREC* ilu_; -}; /// /// \brief A traits class for selecting the types of the preconditioner. /// @@ -124,7 +100,7 @@ struct CPRSelector > EllipticPreconditioner; /// \brief The type of the unique pointer to the preconditioner of the elliptic part. typedef std::unique_ptr > > + AdditionalObjectDeleter > > EllipticPreconditionerPointer; typedef EllipticPreconditioner Smoother; @@ -146,11 +122,11 @@ struct CPRSelector > //! \param ilu A reference to the wrapped preconditioner //! \param p The parallel information for template parameter deduction. template -ParallelPreconditionerDeleter +AdditionalObjectDeleter createParallelDeleter(ILU& ilu, const Dune::OwnerOverlapCopyCommunication& p) { (void) p; - return ParallelPreconditionerDeleter(ilu); + return AdditionalObjectDeleter(ilu); } #endif From 0adde744bf45b79199b04ee751197c1fe2d84ace Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Fri, 4 Sep 2015 21:52:50 +0200 Subject: [PATCH 2/3] [bugfix] Make sequential preconditioner live as long as the parallel one. It holds a reference to the sequential code. Previously this reference point to a temporary object that was deleted upon exit of constructPrecond. With this commit use a unique_ptr to store the parallel preconditioner. It also gets a custom deleter that will delete the nested sequential iterator during destruction. --- .../NewtonIterationBlackoilInterleaved.hpp | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/opm/autodiff/NewtonIterationBlackoilInterleaved.hpp b/opm/autodiff/NewtonIterationBlackoilInterleaved.hpp index 16282a320..4eef1944e 100644 --- a/opm/autodiff/NewtonIterationBlackoilInterleaved.hpp +++ b/opm/autodiff/NewtonIterationBlackoilInterleaved.hpp @@ -26,6 +26,7 @@ #include +#include #include #include @@ -107,17 +108,18 @@ namespace Opm parallelInformation_arg.copyOwnerToAll(istlb, istlb); // Solve. - solve(opA, x, istlb, *sp, precond, result); + solve(opA, x, istlb, *sp, *precond, result); } typedef Dune::SeqILU0 SeqPreconditioner; template - SeqPreconditioner constructPrecond(Operator& opA, const Dune::Amg::SequentialInformation&) const + std::unique_ptr constructPrecond(Operator& opA, const Dune::Amg::SequentialInformation&) const { const double relax = 1.0; - SeqPreconditioner precond(opA.getmat(), relax); + std::unique_ptr + precond(new SeqPreconditioner(opA.getmat(), relax)); return precond; } @@ -126,11 +128,17 @@ namespace Opm typedef Dune::BlockPreconditioner ParPreconditioner; template - ParPreconditioner constructPrecond(Operator& opA, const Comm& comm) const + std::unique_ptr > + constructPrecond(Operator& opA, const Comm& comm) const { const double relax = 1.0; - SeqPreconditioner seq_precond(opA.getmat(), relax); - ParPreconditioner precond(seq_precond, comm); + SeqPreconditioner* seq_precond= new SeqPreconditioner(opA.getmat(), + relax); + typedef AdditionalObjectDeleter Deleter; + std::unique_ptr + precond(new ParPreconditioner(*seq_precond, comm), + Deleter(*seq_precond)); return precond; } #endif From 45fdb3ac48afdff14c9f83872b30968a1bc2641c Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Sat, 5 Sep 2015 16:06:19 +0200 Subject: [PATCH 3/3] [bugfix] Use correct parameters for ParallelIstlInformation::copyValuesTo. The last parameter of this functions specifies how vector/martix entries there are per index/cell of the grid. For the interleaved versions all components end up in one block. Therefore this number needs to be one here (in contrast to the number of phases for the non-interleaved version). This commit new uses the correct number. --- opm/autodiff/NewtonIterationBlackoilInterleaved.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/opm/autodiff/NewtonIterationBlackoilInterleaved.cpp b/opm/autodiff/NewtonIterationBlackoilInterleaved.cpp index 3fb8d80c3..c4906abca 100644 --- a/opm/autodiff/NewtonIterationBlackoilInterleaved.cpp +++ b/opm/autodiff/NewtonIterationBlackoilInterleaved.cpp @@ -153,8 +153,10 @@ namespace Opm const ParallelISTLInformation& info = boost::any_cast( parallelInformation_); Comm istlComm(info.communicator()); + // As we use a dune-istl with block size np the number of components + // per parallel is only one. info.copyValuesTo(istlComm.indexSet(), istlComm.remoteIndices(), - size, np); + size, 1); // Construct operator, scalar product and vectors needed. typedef Dune::OverlappingSchwarzOperator Operator; Operator opA(istlA, istlComm);