MPIHelper should only finalize MPI if it called init

MPIHelper only calls MPI if it was not already called before MPIHelper is constructed. Finalize is however always called on destruction leading to problems with other packages calling init/finalize before/after MPIHelper is setup. Added a bool storing the information if MPIInit was called by the MPIHelper so that finalize is only called in that case.

I would pefer using something along the lines

int finalized;
MPI_Initialized(&initialized);
if(initialized){
  int initialzed;
  MPI_Finalized(&finalized);
  if(!finalized)
    MPI_Finalize();
}

As the MPI_Helper is a singleton the attemp to call MPI_Finalize should happen when main exits. As long as MPI_Finalize is called additionally during main, my version should work, too. Or is there another reason why MPI_Finalize should not be called if MPIHelper did not initialize it.

I had a problem with it because some other class was initializing MPI and then finalizing it (think of another singleton which is set up before MPIHelper using the same trick but not checking before calling finalize if it has been called). Now of course that is a bug in that package but I really think MPIHelper should only finalize if it initializes. If the constructor notices that Init was all ready called then there is a good reason to assume that finalize will be called by the user after destruction, right?

Maybe unrelated, but what is this weird static int is_initialized doing here? If anything, we should check its value and throw an exception on error...

Why throw an exception when we just want to discard subequent calls to MPI_Init? But we might be overprotective here as instance should already take care of this with the static MPIHelper singleton variable.

Not necessarily. It might be that it is already initialized via some other part of the code. E.g. in my python code I initialize via mpi4py and thus DUNE should avoid a second initialization.

Actually MPI_Initialized( &wasInitialized ); if(!wasInitialized) should take care of that so the static is not really needed (at least for MPI2+. Throwing an exception in the case that the return is not MPI_SUCCESS does make sense to me.

@andreas.dedner I agree: You should never destruct an object you did not create.

But, at the risk of sounding stupid: Why do we need the MPIHelper at all? As far as I see it, it serves two purposes:

• Initialising / Finalizing MPI (hiding the possibility that MPI is not available)
• Returning an all-process communicator

The first can be achieved by simple wrapper functions replacing MPI_Init and MPI_Finalize, doing nothing if MPI is not present. The second issue is automatically resolved by creating a CollectiveCommunication (either MPI or not, depending on whether MPI is present).

I don't quite see why "simple wrapper functions replacing MPI_Init and MPI_Finalize" would do anything differently from what we have and in addition one idea you didn't mention is that MPI_Finalize is automatically called on destruction of the object i.e. at end of program.

@andreas.dedner Are you arguing that I do not have to use the MPIHelper (i.e., no library code is allowed to use it)? In that case, I don't see why it is a singleton, nor why it needs to be protected against multiple initialization / finalization. Note, however, that there are some uses in the DGF code, but these can be replaced (as outlined above).

MPIHelper::rank/size of course requires you to have initialized the singleton. If you get that info from somewhere else (from where would you suggest getting it?) then MPIHelper is not needed

Those methods also exist on the CollectiveCommunication< MPI_Comm >.

By the way: My main motive for asking this question was not to remove the MPIHelper, but to clarify whether we require it to be used. If it is not required, I can also call MPI_Init / Finalize by hand (or rely on some external package to do it), never initializing the MPIHelper.

There is another reason for MPIHelper. I can use it whether MPI is there or not (the FakeMPIHelper is used automatically), which means can write programs that work both with and without MPI.

I have a lot of programs that are purely sequential. Nevertheless they need to start by initializing MPIHelper, otherwise I get MPI run-time errors. So in a sense I need MPIHelper, but it always seemed strange to me.

We are getting quite a bit off topic. @oliver.sander you should open a bug report. I had a quick look through dune-grid and as Martin pointed out the dgfparser needs the MPIHelper to be set up and the UG Gridfactory uses it e.g. MPIHelper::getCollectiveCommunication().broadcast(&numVertices, 1, 0); which will cause problems if MPI was found even in serial runs. But as already said: this is off topic.

I'd like to revisit this issue and if no objections come up merge. Is there any objection to having the MPIHelper not call Finalize if it did not call Init? Please avoid the philosophical discussions for now - it's actually quite a simple change. It only case that I see were it could cause problems with existing code is if somebody initialized MPI some other way, then set up an MPIHelper and relied on that to finalize MPI - seems an unlikely scenario to me.

I think the change is fine and I don't see a good reason why we should not merge.

Mentioned in commit 9af12b69

Status changed to merged

Mentioned in commit 62237117

Mentioned in merge request !172 (merged)

Mentioned in commit c15701e9