New collcommobj #329

mikesoehner · 2024-06-24T12:10:20Z

Description

Implemented persistent communication class. This class can also be used to replace most of the already present collective communication in the code.

Related Pull Requests

#PR

Resolved Issues

#Issue

How Has This Been Tested?

Tested with Spinodal Decomposition example and unit tests.

Where possible an ifdef was added. So, depending on the cmake flag for ENABLE_PERSISTENT, the code is compiled differently.

Signed-off-by: Christoph Niethammer <[email protected]>

…onfig Move cmake configuration for persistent collectives into the mpi module

This updated wrapper has an improved interface and is more compact due to the environment class being internal.

This function does not clear the forces in the SoA structure.

This reverts commit 4ba2dff.

This reverts commit 813b6ec.

This reverts commit 3adc768.

FG-TUM

Impressive template magic :D

Can you explain in the main text of the PR why all of this is "just" behind an ifdef and why we can't simply get rid of the old communicator class?

FG-TUM · 2025-01-08T23:44:25Z

src/parallel/CollectiveCommunicationPersistent.h

+class Coll_Comm_Obj
+{


(most of) the rest of the code places the opening { not on a new line. As soon as #342 will be finalized this will be reformatted so we might as well do it now upon introducing new code.

FG-TUM · 2025-01-08T23:48:10Z

src/parallel/CollectiveCommunicationPersistent.h

+template<int tag, int Fn, typename Op, typename Root, typename Comm, typename... Ts>
+class Coll_Comm_Obj


Please add doxygen documentation in javadoc style for all classes, functions, and members.

e.g. here:

Suggested change

template<int tag, int Fn, typename Op, typename Root, typename Comm, typename... Ts>

class Coll_Comm_Obj

/**

* Some brief description

*

* Some more verbose extensive description. Maybe just the large block above.

*

* @tparam tag explanation

* @tparam Fn explanation

* ...

*/

template<int tag, int Fn, typename Op, typename Root, typename Comm, typename... Ts>

class Coll_Comm_Obj

FG-TUM · 2025-01-08T23:54:23Z

src/parallel/CollectiveCommunicationPersistent.h

+    // this function requires that the MPI_Comm and MPI_Op have been setup in the constructor
+    template<typename T = Comm, typename U = Op>
+    auto allreduce()
+        -> std::enable_if_t<std::is_same_v<T, MPI_Comm> && (std::is_same_v<U, add_struct> || std::is_same_v<U, max_struct> || std::is_same_v<U, min_struct>)>
+    {
+        MPI_Allreduce( MPI_IN_PLACE, _buffer.data(), 1, _mpi_members->get_type(), _mpi_members->get_op(), _mpi_comm );
+    }


Just a reminder to add javadoc to all functions as described above.

Also, please explain a bit of the technicalities of the template magic. There are mostly non-computer scientists working on this code ;)

FG-TUM · 2025-01-14T14:12:57Z

src/parallel/CollectiveCommunicationPersistent_helper.h

+class MPI_Env_Wrapper
+{
+public:
+    static auto init_environment(int* argc, char*** argv)
+    {
+        static MPI_Environment mpi_env(argc, argv);
+        _mpi_env = &mpi_env;
+    }
+    static auto init_thread_environment(int* argc, char*** argv, int required)
+    {
+        static MPI_Environment mpi_env(argc, argv, required);
+        _mpi_env = &mpi_env;
+    }
+private:
+    // class to manage MPI_Init and MPI_Finalize
+    class MPI_Environment
+    {
+    public:
+        MPI_Environment() = default;
+
+        MPI_Environment(int* argc, char*** argv)
+        {
+            MPI_Init(argc, argv);
+        }
+        MPI_Environment(int* argc, char*** argv, int required)
+        {
+            int provided;
+            MPI_Init_thread(argc, argv, required, &provided);
+            if (provided < required)
+            {
+                std::cerr << "Cannot provide requested level of MPI thread support." << std::endl;
+                MPI_Abort(MPI_COMM_WORLD, 1);
+            }
+        }
+
+        ~MPI_Environment()
+        {
+            MPI_Finalize();
+        }
+    };
+
+    // storing the class until the end of the program
+    static inline MPI_Environment* _mpi_env;
+};


Is this used anywhere? If not I'd rather not add it to the master branch.

FG-TUM · 2025-01-14T14:29:05Z

src/parallel/CollectiveCommunicationPersistent_helper.h

+#ifndef COLLECTIVECOMMUNICATION_PERSISTENT_HELPER_H_
+#define COLLECTIVECOMMUNICATION_PERSISTENT_HELPER_H_


I prefer #pragma once but ls1 isn't consistent with this so 🤷

FG-TUM · 2025-01-14T14:39:03Z

src/parallel/CollectiveCommunicationPersistent.h

+            // check if we were given a communicator
+            if constexpr( std::is_same_v<Comm, MPI_Comm> )
+                _mpi_comm = comm;
+            // check if we were can cerate a persistent communication request


Suggested change

// check if we were can cerate a persistent communication request

// check if we should create a persistent communication request

FG-TUM · 2025-01-14T15:02:32Z

src/parallel/CollectiveCommunicationPersistent.h

+            std::array<int, sizeof...(Ts)> b;
+            b.fill(1);
+            std::array<MPI_Aint, sizeof...(Ts)> d;
+            std::array<MPI_Datatype, sizeof...(Ts)> t;


please use more expressive variable names ;)

FG-TUM · 2025-01-14T15:06:03Z

src/parallel/CollectiveCommunicationPersistent_helper.h

+    static inline std::vector<MPI_Request*> _allocated_requests;
+    static inline std::vector<MPI_Op*> _allocated_ops;
+    static inline std::vector<MPI_Datatype*> _allocated_types;


So if I understand this correctly, these vectors accumulate all MPI requests, ops, and types created throughout the simulation. Doesn't this mean that in a very long-running simulation, this can accumulate a lot of trash if the persistent communication isn't reusing all of this perfectly? Could this lead to problems? Can we somehow build safeguards / checks against this?

FG-TUM · 2025-01-14T15:13:42Z

src/parallel/CollectiveCommunicationPersistent.h

+    template<int T = Fn>
+    auto persistent()
+        -> std::enable_if_t< T != 0 >
+    {
+        MPI_Start( &_mpi_members->get_request() );
+        MPI_Wait( &_mpi_members->get_request(), MPI_STATUS_IGNORE );
+    }


Not a fan of this name. It sounds like it makes the CollCom object persistent (whatever that would mean). Maybe something along executePersistentRequests or shorter.

For consistency, the next two functions then might need renaming as well.

FG-TUM · 2025-01-14T15:21:38Z

src/io/EnergyLogWriter.cpp

+	unsigned long nNumMolsGlobalEnergyGlobal;
+	double UkinGlobal;
+	double UkinTransGlobal;
+	double UkinRotGlobal;
+	collComm.get(nNumMolsGlobalEnergyGlobal, UkinGlobal, UkinTransGlobal, UkinRotGlobal);


I think we already talked about this but it would be really cool if we could do something like this:

Suggested change

unsigned long nNumMolsGlobalEnergyGlobal;

double UkinGlobal;

double UkinTransGlobal;

double UkinRotGlobal;

collComm.get(nNumMolsGlobalEnergyGlobal, UkinGlobal, UkinTransGlobal, UkinRotGlobal);

auto [nNumMolsGlobalEnergyGlobal, UkinGlobal, UkinTransGlobal, UkinRotGlobal] = collComm.get();

Although looking at the implementation behind get() I'm not sure how to easily do that 😬 . So if it would be too complicated leave it be.

mikesoehner and others added 21 commits May 29, 2024 13:35

Added collective communication classes and helpers

3081561

Updated cmake for new persistent collective comm.

d77f389

Added cleanup of persistent comm in main function.

181b972

Added option for persistent collective comm.

f4ddc47

Where possible an ifdef was added. So, depending on the cmake flag for ENABLE_PERSISTENT, the code is compiled differently.

Move cmake configuration for persistent collectives into the mpi module

70af4b8

Signed-off-by: Christoph Niethammer <[email protected]>

Merge pull request #1 from cniethammer/restrucutre_persistent_cmake_c…

8093aef

…onfig Move cmake configuration for persistent collectives into the mpi module

Updated documentation.

ad0b2c2

Bugfix: Persistent scan request wasn't registered.

b9a58b7

Made the MPI_Members an internal class of Coll_Comm_Obj.

8720dc1

Rewrite of the proposed MPI environment wrapper.

f8bf728

This updated wrapper has an improved interface and is more compact due to the environment class being internal.

Replaced C-style casting with C++-style casting.

1181ace

Merge branch 'ls1mardyn:master' into new_collcommobj

8ee270b

Updated documentation.

7d13197

Bugfix: Fix code for disabled MPI configuration.

73b02a3

Added newline at the end of the files.

d2c954f

New function to update molecule SoA caches.

3adc768

This function does not clear the forces in the SoA structure.

New function to traverse new molecules in a cell.

813b6ec

Updated overlapping communication scheme.

4ba2dff

Revert "Updated overlapping communication scheme."

1aebb0f

This reverts commit 4ba2dff.

Revert "New function to traverse new molecules in a cell."

6786cad

This reverts commit 813b6ec.

Revert "New function to update molecule SoA caches."

f11505b

This reverts commit 3adc768.

cniethammer requested review from cniethammer and FG-TUM December 7, 2024 22:28

FG-TUM requested changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New collcommobj #329

New collcommobj #329

mikesoehner commented Jun 24, 2024

FG-TUM left a comment

FG-TUM Jan 8, 2025

FG-TUM Jan 8, 2025

FG-TUM Jan 8, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

FG-TUM Jan 14, 2025

		template<int tag, int Fn, typename Op, typename Root, typename Comm, typename... Ts>
		class Coll_Comm_Obj

		#ifndef COLLECTIVECOMMUNICATION_PERSISTENT_HELPER_H_
		#define COLLECTIVECOMMUNICATION_PERSISTENT_HELPER_H_

	// check if we were can cerate a persistent communication request
	// check if we should create a persistent communication request

New collcommobj #329

Are you sure you want to change the base?

New collcommobj #329

Conversation

mikesoehner commented Jun 24, 2024

Description

Related Pull Requests

Resolved Issues

How Has This Been Tested?

FG-TUM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment