-
Notifications
You must be signed in to change notification settings - Fork 121
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.
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
fixed: need to init ewoms thread manager #1557
Conversation
jenkins build this please |
the patch looks good. I'll merge if jenkins is happy with it. |
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. |
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 |
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 |
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 But there seem to be several
Not sure which one rules. |
You're right: the threading stuff used to be initialized correctly, albeit it only supported the
The property is only set once (the first line of your |
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.