[Core] Use ParallelUtilities::IsInParallel() instead of legacy OpenMPUtils in DataValueContainer
#14009
Conversation
loumalouomega
added
Cleanup
Kratos Core
FastPR
loumalouomega
changed the title
ParallelUtilities::GetNumThreads() instead of legacy OpenMPUtils in DataValueContainerParallelUtilities::IsInParallel() instead of legacy OpenMPUtils in DataValueContainer
| * @brief Wrapper for omp_in_parallel() | ||
| * @return Maximum number of OpenMP threads that will be used in parallel regions. | ||
| */ | ||
| [[nodiscard]] static int IsInParallel(); |
There was a problem hiding this comment.
this was deliberately not added so far since there is no good solution for Cxx-based parallelisation. I did lots of testing on this, but couldnt get it to work
IIRC the main issue was that the main-thread contributes also in the parallel loop, thus its not as simple as to just check if the current thread-id is different from the main-thread id
There was a problem hiding this comment.
Yes, that's what I found, that why I only return in OMP
There was a problem hiding this comment.
my reasoning for not adding it was that if it doesnt work generally it shouldnt be in the ParallelUtils
Its sth OpenMP specific, thus should remain there until we figure it out
There was a problem hiding this comment.
Yes, otherwise we will never remove the legacy OMPUtils. An alternative is to simply hardcode it in OMP
There was a problem hiding this comment.
someone should probably invest more time to solve it properly rather than hiding the issue ;)
There was a problem hiding this comment.
my opinion, feel free to ignore 🙃
There was a problem hiding this comment.
Of course, I don't have time to invest on that. Anyway no one is using the C++11 parallel and we should support the C++17 which was actually game changer.
philbucher
removed
the
FastPR
| return 1; | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
yes, as @philbucher pointed out, this one was deliberately left out. We should NOT use this to be forward compatible
There was a problem hiding this comment.
In that case I will simply hardcode it
|
My two cents is that we shouldn't even construct an entry in |
Oh boy, probably one of the biggest bugOrfeature discussions :D |
📝 Description
Use
ParallelUtilities::IsInParallel()instead of legacyOpenMPUtilsinDataValueContainer. Also minor clean up.🆕 Changelog
ParallelUtilities::IsInParallel()IsInParallel