Fix lots of threading issues @sgiurgiu pointed out.

Also remove unnecessary GetTime() function and timerState.

src/mlpack/core/util/timers.cpp

@@ -66,25 +66,34 @@ void Timer::ResetAll()
 // Reset a Timers object.
 void Timers::Reset()
 {
+  lock_guard<mutex> lock(timersMutex);
   timers.clear();
   timerStartTime.clear();
-  timerState.clear();
 }
 
-map<string, microseconds>& Timers::GetAllTimers()
+map<string, microseconds> Timers::GetAllTimers()
 {
+  // Make a copy of the timer.
+  lock_guard<mutex> lock(timersMutex);
   return timers;
 }
 
 microseconds Timers::GetTimer(const string& timerName)
 {
+  if (!enabled)
+    return microseconds(0);
+
+  lock_guard<mutex> lock(timersMutex);
   return timers[timerName];
 }
 
 bool Timers::GetState(const string& timerName,
                       const thread::id& threadId)
 {
-  return timerState[threadId][timerName];
+  lock_guard<mutex> lock(timersMutex);
+  if (timerStartTime.count(threadId) == 0)
+    return 0;
+  return (timerStartTime[threadId].count(timerName) > 0);
 }
 
 void Timers::PrintTimer(const string& timerName)
@@ -150,27 +159,32 @@ void Timers::PrintTimer(const string& timerName)
 
 void Timers::StopAllTimers()
 {
-  // Terminate the program timers.
-  for (auto it : timerState)
-  {
-    for (auto it2 : it.second)
-    {
-      if (timerState[it.first][it2.first] == 1)
-        StopTimer(it2.first, it.first);
-    }
-  }
-}
+  // Terminate the program timers.  Don't use StopTimer() since that modifies
+  // the map and would invalidate our iterators.
+  lock_guard<mutex> lock(timersMutex);
 
-high_resolution_clock::time_point Timers::GetTime()
-{
-  return high_resolution_clock::now();
+  high_resolution_clock::time_point currTime = high_resolution_clock::now();
+  for (auto it : timerStartTime)
+    for (auto it2 : it.second)
+      timers[it2.first] += duration_cast<microseconds>(currTime - it2.second);
+
+  // If all timers are stopped, we can clear the maps.
+  timerStartTime.clear();
 }
 
 void Timers::StartTimer(const string& timerName,
                         const thread::id& threadId)
 {
-  timersMutex.lock();
-  if ((timerState[threadId][timerName]) && (timerName != "total_time"))
+  // Don't do anything if we aren't timing.
+  if (!enabled)
+    return;
+  if (timerName == "total_time")
+    return; // Ignore that timer.
+
+  lock_guard<mutex> lock(timersMutex);
+
+  if ((timerStartTime.count(threadId) > 0) &&
+      (timerStartTime[threadId].count(timerName)))
   {
     ostringstream error;
     error << "Timer::Start(): timer '" << timerName
@@ -178,7 +192,7 @@ void Timers::StartTimer(const string& timerName,
     throw runtime_error(error.str());
   }
 
-  high_resolution_clock::time_point currTime = GetTime();
+  high_resolution_clock::time_point currTime = high_resolution_clock::now();
 
   // If the timer is added for the first time.
   if (timers.count(timerName) == 0)
@@ -186,36 +200,37 @@ void Timers::StartTimer(const string& timerName,
     timers[timerName] = (microseconds) 0;
   }
 
-  timerState[threadId][timerName] = true;
   timerStartTime[threadId][timerName] = currTime;
-  timersMutex.unlock();
 }
 
 void Timers::StopTimer(const string& timerName,
                        const thread::id& threadId)
 {
-  timersMutex.lock();
-  if ((!timerState[threadId][timerName]) && (timerName != "total_time"))
+  // Don't do anything if we aren't timing.
+  if (!enabled)
+    return;
+  if (timerName == "total_time")
+    return; // Ignore that timer.
+
+  lock_guard<mutex> lock(timersMutex);
+
+  if ((timerStartTime.count(threadId) == 0) ||
+      (timerStartTime[threadId].count(timerName) == 0))
   {
     ostringstream error;
-    error << "Timer::Stop(): timer '" << timerName
-        << "' has already been stopped";
+    error << "Timer::Stop(): no timer with name '" << timerName
+        << "' currently running";
     throw runtime_error(error.str());
   }
 
-  high_resolution_clock::time_point currTime = GetTime();
+  high_resolution_clock::time_point currTime = high_resolution_clock::now();
 
   // Calculate the delta time.
-  timerState[threadId][timerName] = false;
   timers[timerName] += duration_cast<microseconds>(currTime -
       timerStartTime[threadId][timerName]);
 
   // Remove the entries.
-  timerState[threadId].erase(timerName);
   timerStartTime[threadId].erase(timerName);
-  if (timerState[threadId].empty())
-    timerState.erase(threadId);
   if (timerStartTime[threadId].empty())
     timerStartTime.erase(threadId);
-  timersMutex.unlock();
 }

src/mlpack/core/util/timers.hpp

@@ -19,6 +19,7 @@
 #include <thread> // std::thread is used for thread safety.
 #include <mutex>
 #include <list>
+#include <atomic>
 
 #if defined(_WIN32)
   // uint64_t isn't defined on every windows.
@@ -101,7 +102,7 @@ class Timers
   /**
    * Returns a copy of all the timers used via this interface.
    */
-  std::map<std::string, std::chrono::microseconds>& GetAllTimers();
+  std::map<std::string, std::chrono::microseconds> GetAllTimers();
 
   /**
    * Reset the timers.  This stops all running timers and removes them.  Whether
@@ -161,7 +162,7 @@ class Timers
   void StopAllTimers();
 
   //! Modify whether or not timing is enabled.
-  bool& Enabled() { return enabled; }
+  std::atomic<bool>& Enabled() { return enabled; }
   //! Get whether or not timing is enabled.
   bool Enabled() const { return enabled; }
 
@@ -170,16 +171,12 @@ class Timers
   std::map<std::string, std::chrono::microseconds> timers;
   //! A mutex for modifying the timers.
   std::mutex timersMutex;
-  //! A map that contains whether or not each timer is currently running.
-  std::map<std::thread::id, std::map<std::string, bool>> timerState;
   //! A map for the starting values of the timers.
   std::map<std::thread::id, std::map<std::string,
       std::chrono::high_resolution_clock::time_point>> timerStartTime;
 
-  std::chrono::high_resolution_clock::time_point GetTime();
-
   //! Whether or not timing is enabled.
-  bool enabled;
+  std::atomic<bool> enabled;
 };
 
 } // namespace mlpack

src/mlpack/tests/timer_test.cpp

@@ -98,7 +98,6 @@ BOOST_AUTO_TEST_CASE(TwiceStartTimerTest)
   Timer::DisableTiming();
 }
 
-#include <errno.h>
 BOOST_AUTO_TEST_CASE(MultithreadTimerTest)
 {
   Timer::EnableTiming();
@@ -132,4 +131,20 @@ BOOST_AUTO_TEST_CASE(MultithreadTimerTest)
   BOOST_REQUIRE(Timer::Get("thread_timer") > std::chrono::microseconds(50000));
 }
 
+BOOST_AUTO_TEST_CASE(DisabledTimingTest)
+{
+  // It should be disabled by default but let's be paranoid.
+  Timer::DisableTiming();
+
+  Timer::Start("test_timer");
+  #ifdef _WIN32
+  Sleep(20);
+  #else
+  usleep(20000);
+  #endif
+  Timer::Stop("test_timer");
+
+  BOOST_REQUIRE(Timer::Get("test_timer") == std::chrono::microseconds(0));
+}
+
 BOOST_AUTO_TEST_SUITE_END();