Threads

[gassmoeller]

Implements #59. I decided to make it a cmake variable instead of a input parameter, because we need the value right at the beginning of main.cc:main() before even initializing anything, and we do not know the name of the parameter file at that time. Unfortunately now we need to recompile if we want to change the setting, but at least it is user-controllable. Is this an acceptable solution?

[bangerth]

What if you made it a run time parameter, say -j N? You'd then specify it when calling the executable if you want anything other than the default.

[tjhei]

1. anything except 1 will break PETSc (not that this has been tested recently, so I assume it is probably broken in other places) so keeping 1 as the default is a good idea.
2. I don't like that it is a cmake parameter (does that mean you have to recompile the whole code-base if you change it?)

[gassmoeller]

Currently we need to recompile everything to change this, yes. Unfortunately everything else is more complicated, because when parsing our runtime parameters we already need information about the MPI communicator (e.g. -h only outputs on process 0) so MPI_InitFinalize needs to be called before parsing runtime parameters. I could delay executing these commands (and only set a flag when parsing), but that makes the whole parsing code more complicated. Do you have an idea how to work around this?

If this breaks PETSc I should probably add a check to CMakeLists that not both are active at the same time.

[tjhei]

Currently we need to recompile everything to change this, yes.

So this means it is more annoying to switch than editing main.cc. Hrm.

Unfortunately everything else is more complicated

Hang on: we have an environment variable DEAL_II_NUM_THREADS that is used, see http://www.dealii.org/developer/doxygen/deal.II/classMultithreadInfo.html#ae253b8ff679c4119f9c5bff4439fe606

Wouldn't that be enough? This means we would need to set the value to invalid_unsigned_int for the command line value to work.

[gassmoeller]

Hmm, I tried exporting DEAL_II_NUM_THREADS=1 and that indeed limits the number of threads to 1. But can we set this environment variable by default to 1? I would like to keep the current behavior (threading disabled until somebody explicitly enables it). Mostly to follow the principle of least surprise to the user. Or do you think we should just enable this by default? I could also ask around on the mailing list what people think about the feature.

[tjhei]

But can we set this environment variable by default to 1?

Not easily. Either we enable by default (I don't think this will be a problem) or safer:
Can you test early in main.cc:

const char *penv = getenv ("DEAL_II_NUM_THREADS");
unsigned int max_threads = (penv==nullptr) ? 1 : numbers::invalid_unsigned_int;

MPI::InitFinalize(...., max_threads);

[gassmoeller]

Yes that seems to work. I like this solution, it is easy for people to set this if they want to, and everything stays as usual if you do nothing.

[bangerth]

I'd still like to advocate for a command line option -j in the same way as it exists for make and ninja. (Though I'd also be happy if -j has no numeric argument and simply means "use as many threads as reasonable -- possibly limited by DEAL_II_NUM_THREADS. In other words, passing -j would set max_threads to numbers::invalid_unsigned_int, whereas the default is one.)

You can parse command line options before you initialize the MPI system.

[gassmoeller]

I reorganized main() to parse the command line parameters before MPI initialization and made the parameter a command line option. Looks nicer and more intuitive now. I also removed the print_version() function as all of that information is in the header now and removed some duplication by introducing a new template function. Let's see what the tester says. Anything else? (I will follow up with a PR that further cleans main(), but that is unrelated).

[bangerth]

One of the test fails by producing different output. I haven't looked at the code yet, but suspect you have something like

  if (dim>=2 && dim <=3)
    ...do something
  else
     Assert (false, ExcMessage(...));

To restore the previous error (and help the user a bit), just replace the assertion by

  Assert (dim >= 2 && dim <= 3, ...)

Since we're in the else block, the condition is of course always false.

[bangerth]

comma after "set"

[bangerth]

what is "it" here?

[bangerth]

comma after "i.e."

[bangerth]

keep -h at either the top of the bottom of the list.

[bangerth]

This is where the difference in output comes from. I'd say to just revert to the previous condition -- I know it's duplicative, but it's also instructive.

[bangerth]

nice simplification of code repetition

[gassmoeller]

Done 😄.

[bangerth]

OK to merge once the tester is happy.