Fix DD memory leaks

Refs #3984

admin/lsan-suppressions.txt

@@ -5,7 +5,6 @@ leak:atoms_to_constraints
 leak:atoms_to_settles
 leak:balance_fep_lists
 leak:dd_make_reverse_top
-leak:makeBondedLinks
 leak:dd_make_local_constraints
 leak:dd_move_f
 leak:dd_partition_system
@@ -15,8 +14,6 @@ leak:do_edsam
 leak:do_inputrec
 leak:do_single_flood
 leak:get_zone_pulse_groups
-leak:gmx::DomainDecompositionBuilder::Impl::build
-leak:gmx::DomainDecompositionBuilder::Impl::Impl
 leak:gmx_check
 leak:gmx_make_edi
 leak:gmx_pme_receive_f

src/gromacs/domdec/builder.h

@@ -100,10 +100,10 @@ class DomainDecompositionBuilder
     //! Destructor
     ~DomainDecompositionBuilder();
     //! Build the resulting DD manager
-    gmx_domdec_t* build(LocalAtomSetManager*       atomSets,
-                        const gmx_localtop_t&      localTopology,
-                        const t_state&             localState,
-                        ObservablesReducerBuilder* observablesReducerBuilder);
+    std::unique_ptr<gmx_domdec_t> build(LocalAtomSetManager*       atomSets,
+                                        const gmx_localtop_t&      localTopology,
+                                        const t_state&             localState,
+                                        ObservablesReducerBuilder* observablesReducerBuilder);
 
 private:
     class Impl;

src/gromacs/domdec/cellsizes.cpp

@@ -57,15 +57,15 @@
 #include "domdec_internal.h"
 #include "utility.h"
 
-static void set_pme_maxshift(gmx_domdec_t*      dd,
-                             gmx_ddpme_t*       ddpme,
-                             gmx_bool           bUniform,
-                             const gmx_ddbox_t* ddbox,
-                             const real*        cellFrac)
+static void set_pme_maxshift(gmx_domdec_t*             dd,
+                             gmx_ddpme_t*              ddpme,
+                             gmx_bool                  bUniform,
+                             const gmx_ddbox_t*        ddbox,
+                             gmx::ArrayRef<const real> cellFrac)
 {
     int sh = 0;
 
-    gmx_domdec_comm_t* comm = dd->comm;
+    gmx_domdec_comm_t* comm = dd->comm.get();
     const int          nc   = dd->numCells[ddpme->dim];
     const int          ns   = ddpme->nslab;
 
@@ -84,8 +84,8 @@ static void set_pme_maxshift(gmx_domdec_t*      dd,
         /* We need to check for all pme nodes which nodes they
          * could possibly need to communicate with.
          */
-        const int* xmin = ddpme->pp_min;
-        const int* xmax = ddpme->pp_max;
+        gmx::ArrayRef<const int> xmin = ddpme->pp_min;
+        gmx::ArrayRef<const int> xmax = ddpme->pp_max;
         /* Allow for atoms to be maximally 2/3 times the cut-off
          * out of their DD cell. This is a reasonable balance between
          * between performance and support for most charge-group/cut-off
@@ -201,7 +201,7 @@ static real cellsize_min_dlb(gmx_domdec_comm_t* comm, int dim_ind, int dim)
 gmx::ArrayRef<const std::vector<real>>
 set_dd_cell_sizes_slb(gmx_domdec_t* dd, const gmx_ddbox_t* ddbox, int setmode, ivec npulse)
 {
-    gmx_domdec_comm_t* comm = dd->comm;
+    gmx_domdec_comm_t* comm = dd->comm.get();
 
     gmx::ArrayRef<std::vector<real>> cell_x_master;
     if (setmode == setcellsizeslbMASTER)
@@ -214,7 +214,7 @@ set_dd_cell_sizes_slb(gmx_domdec_t* dd, const gmx_ddbox_t* ddbox, int setmode, i
     {
         cellsize_min[d] = ddbox->box_size[d] * ddbox->skew_fac[d];
         npulse[d]       = 1;
-        if (dd->numCells[d] == 1 || comm->slb_frac[d] == nullptr)
+        if (dd->numCells[d] == 1 || comm->slb_frac[d].empty())
         {
             /* Uniform grid */
             real cell_dx = ddbox->box_size[d] / dd->numCells[d];
@@ -306,7 +306,7 @@ set_dd_cell_sizes_slb(gmx_domdec_t* dd, const gmx_ddbox_t* ddbox, int setmode, i
         }
     }
 
-    if (!isDlbOn(comm))
+    if (!isDlbOn(comm->dlbState))
     {
         copy_rvec(cellsize_min, comm->cellsize_min);
     }
@@ -316,7 +316,7 @@ set_dd_cell_sizes_slb(gmx_domdec_t* dd, const gmx_ddbox_t* ddbox, int setmode, i
     {
         set_pme_maxshift(dd,
                          &ddRankSetup.ddpme[d],
-                         comm->slb_frac[dd->dim[d]] == nullptr,
+                         comm->slb_frac[dd->dim[d]].empty(),
                          ddbox,
                          ddRankSetup.ddpme[d].slb_dim_f);
     }
@@ -343,7 +343,7 @@ static void dd_cell_sizes_dlb_root_enforce_limits(gmx_domdec_t*      dd,
     GMX_ASSERT(region_size >= (range[1] - range[0]) * cellsize_limit_f,
                "The region should fit all cells at minimum size");
 
-    gmx_domdec_comm_t* comm = dd->comm;
+    gmx_domdec_comm_t* comm = dd->comm.get();
 
     const int ncd = dd->numCells[dim];
 
@@ -562,7 +562,7 @@ static void set_dd_cell_sizes_dlb_root(gmx_domdec_t*      dd,
                                        gmx_bool           bUniform,
                                        int64_t            step)
 {
-    gmx_domdec_comm_t* comm    = dd->comm;
+    gmx_domdec_comm_t* comm    = dd->comm.get();
     constexpr real     c_relax = 0.5;
     int                range[] = { 0, 0 };
 
@@ -721,7 +721,7 @@ static void set_dd_cell_sizes_dlb_root(gmx_domdec_t*      dd,
         /* The master determines the maximum shift for
          * the coordinate communication between separate PME nodes.
          */
-        set_pme_maxshift(dd, &ddRankSetup.ddpme[d], bUniform, ddbox, rowMaster->cellFrac.data());
+        set_pme_maxshift(dd, &ddRankSetup.ddpme[d], bUniform, ddbox, rowMaster->cellFrac);
     }
     rowMaster->cellFrac[pos++] = ddRankSetup.ddpme[0].maxshift;
     if (d >= 1)
@@ -732,7 +732,7 @@ static void set_dd_cell_sizes_dlb_root(gmx_domdec_t*      dd,
 
 static void relative_to_absolute_cell_bounds(gmx_domdec_t* dd, const gmx_ddbox_t* ddbox, int dimind)
 {
-    gmx_domdec_comm_t*        comm      = dd->comm;
+    gmx_domdec_comm_t*        comm      = dd->comm.get();
     const DDCellsizesWithDlb& cellsizes = comm->cellsizesWithDlb[dimind];
 
     /* Set the cell dimensions */
@@ -850,7 +850,7 @@ static void set_dd_cell_sizes_dlb(gmx_domdec_t*      dd,
                                   int64_t            step,
                                   gmx_wallcycle*     wcycle)
 {
-    gmx_domdec_comm_t* comm = dd->comm;
+    gmx_domdec_comm_t* comm = dd->comm.get();
 
     if (bDoDLB)
     {
@@ -887,13 +887,13 @@ void set_dd_cell_sizes(gmx_domdec_t*      dd,
                        int64_t            step,
                        gmx_wallcycle*     wcycle)
 {
-    gmx_domdec_comm_t* comm = dd->comm;
+    gmx_domdec_comm_t* comm = dd->comm.get();
 
     /* Copy the old cell boundaries for the cg displacement check */
     copy_rvec(comm->cell_x0, comm->old_cell_x0);
     copy_rvec(comm->cell_x1, comm->old_cell_x1);
 
-    if (isDlbOn(comm))
+    if (isDlbOn(comm->dlbState))
     {
         if (DDMASTER(dd))
         {

src/gromacs/domdec/dlb.cpp

@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2018,2019, by the GROMACS development team, led by
+ * Copyright (c) 2018,2019,2021, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -137,7 +137,7 @@ gmx_bool dd_dlb_get_should_check_whether_to_turn_dlb_on(gmx_domdec_t* dd)
 
 gmx_bool dd_dlb_is_on(const gmx_domdec_t* dd)
 {
-    return isDlbOn(dd->comm);
+    return isDlbOn(dd->comm->dlbState);
 }
 
 gmx_bool dd_dlb_is_locked(const gmx_domdec_t* dd)

src/gromacs/domdec/dlbtiming.cpp

@@ -46,10 +46,11 @@
 #include "domdec_internal.h"
 
 /*! \brief Struct for timing the region for dynamic load balancing */
-struct BalanceRegion
+class BalanceRegion::Impl
 {
+public:
     /*! \brief Constructor */
-    BalanceRegion() :
+    Impl() :
         isOpen(false), isOpenOnCpu(false), isOpenOnGpu(false), cyclesOpenCpu(0), cyclesLastCpu(0)
     {
     }
@@ -61,27 +62,29 @@ struct BalanceRegion
     gmx_cycles_t cyclesLastCpu; /**< Cycle count at the last call to \p ddCloseBalanceRegionCpu() */
 };
 
-BalanceRegion* ddBalanceRegionAllocate()
+BalanceRegion::BalanceRegion()
 {
-    return new BalanceRegion;
+    impl_ = std::make_unique<Impl>();
 }
 
+BalanceRegion::~BalanceRegion() = default;
+
 /*! \brief Returns the pointer to the balance region.
  *
  * This should be replaced by a properly managed BalanceRegion class,
  * but that requires a lot of refactoring in domdec.cpp.
  */
-static BalanceRegion* getBalanceRegion(const gmx_domdec_t* dd)
+static BalanceRegion::Impl* getBalanceRegion(const gmx_domdec_t* dd)
 {
     GMX_ASSERT(dd != nullptr && dd->comm != nullptr, "Balance regions should only be used with DD");
-    BalanceRegion* region = dd->comm->balanceRegion;
+    BalanceRegion::Impl* region = dd->comm->balanceRegion.impl_.get();
     GMX_ASSERT(region != nullptr, "Balance region should be initialized before use");
     return region;
 }
 
 void DDBalanceRegionHandler::openRegionCpuImpl(DdAllowBalanceRegionReopen gmx_unused allowReopen) const
 {
-    BalanceRegion* reg = getBalanceRegion(dd_);
+    BalanceRegion::Impl* reg = getBalanceRegion(dd_);
     if (dd_->comm->ddSettings.recordLoad)
     {
         GMX_ASSERT(allowReopen == DdAllowBalanceRegionReopen::yes || !reg->isOpen,
@@ -96,15 +99,15 @@ void DDBalanceRegionHandler::openRegionCpuImpl(DdAllowBalanceRegionReopen gmx_un
 
 void DDBalanceRegionHandler::openRegionGpuImpl() const
 {
-    BalanceRegion* reg = getBalanceRegion(dd_);
+    BalanceRegion::Impl* reg = getBalanceRegion(dd_);
     GMX_ASSERT(reg->isOpen, "Can only open a GPU region inside an open CPU region");
     GMX_ASSERT(!reg->isOpenOnGpu, "Can not re-open a GPU balance region");
     reg->isOpenOnGpu = true;
 }
 
 void ddReopenBalanceRegionCpu(const gmx_domdec_t* dd)
 {
-    BalanceRegion* reg = getBalanceRegion(dd);
+    BalanceRegion::Impl* reg = getBalanceRegion(dd);
     /* If the GPU is busy, don't reopen as we are overlapping with work */
     if (reg->isOpen && !reg->isOpenOnGpu)
     {
@@ -114,7 +117,7 @@ void ddReopenBalanceRegionCpu(const gmx_domdec_t* dd)
 
 void DDBalanceRegionHandler::closeRegionCpuImpl() const
 {
-    BalanceRegion* reg = getBalanceRegion(dd_);
+    BalanceRegion::Impl* reg = getBalanceRegion(dd_);
     if (reg->isOpen && reg->isOpenOnCpu)
     {
         GMX_ASSERT(reg->isOpenOnCpu, "Can only close an open region");
@@ -139,7 +142,7 @@ void DDBalanceRegionHandler::closeRegionCpuImpl() const
 void DDBalanceRegionHandler::closeRegionGpuImpl(float waitGpuCyclesInCpuRegion,
                                                 DdBalanceRegionWaitedForGpu waitedForGpu) const
 {
-    BalanceRegion* reg = getBalanceRegion(dd_);
+    BalanceRegion::Impl* reg = getBalanceRegion(dd_);
     if (reg->isOpen)
     {
         GMX_ASSERT(reg->isOpenOnGpu, "Can not close a non-open GPU balance region");

src/gromacs/domdec/dlbtiming.h

@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2017,2018,2019, by the GROMACS development team, led by
+ * Copyright (c) 2017,2018,2019,2021, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -45,9 +45,10 @@
 #ifndef GMX_DOMDEC_DLBTIMING_H
 #define GMX_DOMDEC_DLBTIMING_H
 
+#include <memory>
+
 #include "gromacs/mdtypes/commrec.h"
 
-struct BalanceRegion;
 struct gmx_domdec_t;
 struct t_nrnb;
 
@@ -203,6 +204,18 @@ class DDBalanceRegionHandler
     gmx_domdec_t* dd_;
 };
 
+//! Object that describes a DLB balancing region
+class BalanceRegion
+{
+public:
+    BalanceRegion();
+    ~BalanceRegion();
+    // Not private because used by DDBalanceRegionHandler
+    // private:
+    class Impl;
+    std::unique_ptr<Impl> impl_;
+};
+
 /*! \brief Returns a pointer to a constructed \p BalanceRegion struct
  *
  * Should be replaced by a proper constructor once BalanceRegion is a proper