[GitHub] [incubator-mxnet] anirudh2290 commented on a change in pull request #16999: Prevent after-fork number of OMP threads being bigger than 1.

[GitBox <gi...@apache.org>]

anirudh2290 commented on a change in pull request #16999: Prevent after-fork number of OMP threads being bigger than 1.
URL: https://github.com/apache/incubator-mxnet/pull/16999#discussion_r355083334
 
 

 ##########
 File path: src/engine/openmp.cc
 ##########
 @@ -83,10 +83,10 @@ void OpenMP::set_reserve_cores(int cores) {
 
 int OpenMP::GetRecommendedOMPThreadCount(bool exclude_reserved) const {
 #ifdef _OPENMP
-  if (omp_num_threads_set_in_environment_) {
-    return omp_get_max_threads();
-  }
   if (enabled_) {
+    if (omp_num_threads_set_in_environment_) {
+      return omp_get_max_threads();
+    }
 
 Review comment:
   this is the correct behavior, but i wonder if this will cause performance issues, for certain scenarios where clients of this class depend on GetRecommendedOMPThreadCount() to return the right number even if set_enabled is false. I only see set_enabled used in initialize so i think it should be fine. Other (probably less riskier) option is to set omp_num_threads_set_in_environment_ to false in the child process, but since it is private it will require additional setter and getter. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services