fixed: need to init ewoms thread manager

[akva2]

This fixes OpenMP. Not entirely sure this is where the call is wanted etc etc, but at least it points out the issue.

Without this call, manager returns 1 thread and we crash and burn in the fvbaselinearizer due to missing contexts.

[andlaus]

jenkins build this please

[andlaus]

the patch looks good. I'll merge if jenkins is happy with it.

[alfbr]

This restored the test at linuxbenchmarking.com, but the performance benefit from openmp is gone. It seems this PR disabled the threading. I can check more on Monday.

[andlaus]

its possible that currently the only way to specify the number of threads per process for non-frankenstein simulators without OPM/opm-models#318 is using the --threads-per-process=$N command line parameter. since the flow frankenstein did not care about OpenMP until the switch to the eWoms parameter system where it got slightly less frankensteiny, setting OMP_NUM_THREADS "accidentally" worked for it.

[alfbr]

So the new parameter system broke the threading implementation? Maybe it should be reverted until we can fix it?

[andlaus]

So the new parameter system broke the threading implementation? Maybe it should be reverted until we can fix it?

I might have been wrong, but no, this PR broke OpenMP by fixing it: before it, openMP was not explicitly initialized at all. I guess this meant that the thread manager object from eWoms just returned garbage and the arrays in Ewoms::FvBaseLinearizer which depend on the number of threads were not initialized correctly. IOW, the fact that you did not get a segfault with flow was always just plain luck. (Also note that OPM/opm-models#318 never suffered from this issue.)

[blattms]

I beg to differ. Before the parameter system was changed OMP was correctly initialized as in parameter system PR you removed the OMP initialization in FlowMainEbos.hpp

            // OpenMP setup.
            if (!getenv("OMP_NUM_THREADS")) {
                // Default to at most 2 threads, regardless of
                // number of cores (unless ENV(OMP_NUM_THREADS) is defined)
                int num_cores = omp_get_num_procs();
                int num_threads = std::min(2, num_cores);
                omp_set_num_threads(num_threads);
            }

and

            std::string numThreadsParam("--threads-per-process=");
            int numThreads = omp_get_max_threads();
            numThreadsParam += std::to_string(numThreads);
            argv.push_back(numThreadsParam.c_str());

without calling something similar somewhere else. Now with the call to ThreadManager::init() we finally initialize set the number of threads again (omp_set_num_threads), but the number of threads still seems different. The default seems to be set to 1 now in ewoms/disc/common/fvbasediscretization.hh.

But there seem to be several ThreadsPerProcess properties:

ewoms$ find . -name \*.[hc][hc]| xargs grep ThreadsPerProcess
./ewoms/disc/common/fvbasediscretization.hh:SET_INT_PROP(FvBaseDiscretization, ThreadsPerProcess, 1);
./ewoms/disc/common/fvbaseproperties.hh:NEW_PROP_TAG(ThreadsPerProcess);
./ewoms/parallel/threadmanager.hh:NEW_PROP_TAG(ThreadsPerProcess);
./ewoms/parallel/threadmanager.hh:        EWOMS_REGISTER_PARAM(TypeTag, int, ThreadsPerProcess,
./ewoms/parallel/threadmanager.hh:        numThreads_ = EWOMS_GET_PARAM(TypeTag, int, ThreadsPerProcess);

Not sure which one rules.

[andlaus]

You're right: the threading stuff used to be initialized correctly, albeit it only supported the OMP_NUM_THREADS method.

But there seem to be several ThreadsPerProcess properties:

The property is only set once (the first line of your grep results). the thread manager uses the corresponding parameter which uses the property as a default.