[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #15574: fix naive engine for multi-threaded inference

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

marcoabreu commented on a change in pull request #15574: fix naive engine for multi-threaded inference
URL: https://github.com/apache/incubator-mxnet/pull/15574#discussion_r305517571
 
 

 ##########
 File path: src/engine/naive_engine.cc
 ##########
 @@ -238,7 +238,11 @@ class NaiveEngine final : public Engine {
     static_cast<NaiveEngine*>(engine)->req_completed_ = true;
   }
   // whether action is completed
-  bool req_completed_;
+#if DMLC_CXX11_THREAD_LOCAL
+  static thread_local bool req_completed_;
+#else
+  static MX_THREAD_LOCAL bool req_completed_;
+#endif
 
 Review comment:
   Please not another build flag - at some point we're gonna spend more money on CI than the NASA :D
   
   Could we make some benchmarks to determine the impact here?
   
   I'm not that deep inside the engine code, so excuse me if that's a stupid question. But why are we declaring the internal variables of the engine as thread local instead of creating thread-local instantiations of the engines. Each thread gets its own engine and we switch all static variables to private ones. The current design of the engine doesn't seem to have the creation of multiple instantiations in mind so we should fix that instead of making all the static variables threadlocal.

----------------------------------------------------------------
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