Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tabulated bond memory leaks #3961

Merged
merged 7 commits into from
Oct 24, 2020
Merged

Fix tabulated bond memory leaks #3961

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
34e7952
core: Remove unimplemented callback
jngrad Oct 20, 2020
5419b41
core: Cleanup comments and doxygen blocks
jngrad Oct 20, 2020
00f1d18
core: Extract bond type check logic
jngrad Oct 20, 2020
0451fef
core: Fix memory leak for tabulated bonds
jngrad Oct 20, 2020
7c85067
core: Remove callback code duplication
jngrad Oct 20, 2020
ef9c69a
core: Rename callback
jngrad Oct 20, 2020
b8e1eba
Merge branch 'python' into fix-3906
kodiakhq[bot] Oct 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/core/bonded_interactions/bonded_tab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ int tabulated_bonded_set_params(int bond_type,
make_bond_type_exist(bond_type);

/* set types */
if (bonded_ia_params[bond_type].type == BONDED_IA_TABULATED_DISTANCE or
bonded_ia_params[bond_type].type == BONDED_IA_TABULATED_ANGLE or
bonded_ia_params[bond_type].type == BONDED_IA_TABULATED_DIHEDRAL) {
delete bonded_ia_params[bond_type].p.tab.pot;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant't you just check for nullptr?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (bonded_ia_params[bond_type].p.tab.pot) delete bonded_ia_params[bond_type].p.tab.pot;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this would raise a Clang-Tidy warning because you can simplify it to

delete bonded_ia_params[bond_type].p.tab.pot;

which is safe on a nullptr.
However, this would also trigger a segfault, because you can create a tabulated bond on top of an existing FENE bond, in which case .p.tab.pot contains a random address. You can overwrite an bond existing bond in ESPResSo, this is used for example in testsuite/python/interactions_bonded_interface.py.

}
auto tab_pot = bonded_ia_params[bond_type].p.tab.pot = new TabulatedPotential;
bonded_ia_params[bond_type].p.tab.type = tab_type;

Expand Down
9 changes: 5 additions & 4 deletions src/core/communication.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,14 +335,15 @@ void mpi_bcast_ia_params_slave(int i, int j) {
boost::mpi::broadcast(comm_cart, *get_ia_param(i, j), 0);
} else { /* bonded interaction parameters */
make_bond_type_exist(i); /* realloc bonded_ia_params on slave nodes! */
if (is_tabulated_bond(bonded_ia_params[i].type)) {
delete bonded_ia_params[i].p.tab.pot;
}
MPI_Bcast(&(bonded_ia_params[i]), sizeof(Bonded_ia_parameters), MPI_BYTE, 0,
comm_cart);
/* For tabulated potentials we have to send the tables extra */
if (is_tabulated_bond(bonded_ia_params[i].type)) {
auto *tab_pot = new TabulatedPotential();
boost::mpi::broadcast(comm_cart, *tab_pot, 0);

bonded_ia_params[i].p.tab.pot = tab_pot;
bonded_ia_params[i].p.tab.pot = new TabulatedPotential();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't looked into the code, but can't we use smart pointers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so: we overwrite the memory block of the bond object on worker nodes with a bytestring of the same object from the head node, so the counter from a shared_ptr would be overwritten. As for unique_ptr, in the line you highlighted, we do an assignment, so the smart pointer goes out of scope and gets garbage-collected... but it contains a random memory address (because of the bytestring deserialization a few lines above), which is a recipe for segmentation faults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure i'm following... but it sounds like this is a place for larger scale refactoring which is out of scope of this pull request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the correct way would be to write boost serialization methods, which can handle pointer members out of the box, e.g.

struct Tabulated_bond_parameters {
  int type;
  TabulatedPotential *pot;

private:
  friend boost::serialization::access;
  template <typename Archive>
  void serialize(Archive &ar, long int /* version */) {
    ar &type;
    ar &pot;
  }
};

but there is no guarantee that we are overwriting a bond type with the same bond type, so we would have to add destructors too... Not sure if this is worth the effort, if our long-term goal is to replace this interaction communication infrastructure with a python setter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a much more promising refactoring goal is breaking the dependency on MpiCallbacks.hpp from consumers of MPI callbacks (fix-3906...fix-3907-p1), which as a side effect is removing global variables and removing prototypes of MPI worker functions from header files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a much more promising refactoring goal is breaking the dependency on MpiCallbacks.hpp from consumers of MPI callbacks (fix-3906...fix-3907-p1), which as a side effect is removing global variables and removing prototypes of MPI worker functions from header files.

Isn't that totally orthogonal to the memory leak issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the correct way would be to write boost serialization methods, which can handle pointer members out of the box, e.g.

struct Tabulated_bond_parameters {
  int type;
  TabulatedPotential *pot;

private:
  friend boost::serialization::access;
  template <typename Archive>
  void serialize(Archive &ar, long int /* version */) {
    ar &type;
    ar &pot;
  }
};

but there is no guarantee that we are overwriting a bond type with the same bond type, so we would have to add destructors too... Not sure if this is worth the effort, if our long-term goal is to replace this interaction communication infrastructure with a python setter.

sorry, if that has been discussed and i don't remember it anymore... Do we have some design plans for the interaction stuff already?

boost::mpi::broadcast(comm_cart, *bonded_ia_params[i].p.tab.pot, 0);
}
}

Expand Down