Improve locks for multithreading (#10)

* Use std::unique_lock instead of std::mutex lock/unlock methods * Improve std::vector initialization
Orange-OpenSource · Jun 5, 2024 · 5aaa926 · 5aaa926
2 parents bf78ed1 + 8cd9ff8
commit 5aaa926
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 33 deletions.
diff --git a/lib/monitors/nvidiamonitor.cpp b/lib/monitors/nvidiamonitor.cpp
@@ -133,16 +133,17 @@ void uprofile::NvidiaMonitor::watchGPU(int period)
                 int err = read_nvidia_smi_stdout(stdout_fd, gpuUsage, usedMem, totalMem);
                 if (err != 0) {
                     cerr << errorMsg << ": read_error = " << strerror(err) << endl;
-                    m_mutex.lock();
+                    unique_lock<mutex> lk(m_mutex);
                     m_watching = false;
-                    m_mutex.unlock();
+                    lk.unlock();
                     break;
                 }
-                m_mutex.lock();
+
+                unique_lock<mutex> lk(m_mutex);
                 m_gpuUsage = !gpuUsage.empty() ? stof(gpuUsage) : 0.f;
                 m_usedMem = !usedMem.empty() ? stoi(usedMem) * 1024 : 0;    // MiB to KiB
                 m_totalMem = !totalMem.empty() ? stoi(totalMem) * 1024 : 0; // MiB to KiB
-                m_mutex.unlock();
+                lk.unlock();
             }
         }));
     }
@@ -155,9 +156,9 @@ void uprofile::NvidiaMonitor::abortWatchGPU()
 {
 #if defined(__linux__)
     if (m_watcherThread) {
-        m_mutex.lock();
+        unique_lock<mutex> lk(m_mutex);
         m_watching = false;
-        m_mutex.unlock();
+        lk.unlock();
         m_watcherThread->join();
         m_watcherThread.reset();
     }

diff --git a/lib/uprofileimpl.cpp b/lib/uprofileimpl.cpp
@@ -67,17 +67,17 @@ void UProfileImpl::addGPUMonitor(IGPUMonitor* monitor)
         return;
     }
 
-    if (!m_gpuMonitor) {
-        removeGPUMonitor();
-    }
+    removeGPUMonitor();
 
     m_gpuMonitor = monitor;
 }
 
 void UProfileImpl::removeGPUMonitor()
 {
-    delete m_gpuMonitor;
-    m_gpuMonitor = NULL;
+    if (m_gpuMonitor) {
+        delete m_gpuMonitor;
+        m_gpuMonitor = NULL;
+    }
 }
 
 void UProfileImpl::timeBegin(const std::string& title)
@@ -91,14 +91,14 @@ void UProfileImpl::timeEnd(const std::string& title)
     unsigned long long beginTimestamp = 0;
 
     // Find step in the map
-    m_stepsMutex.lock();
+    unique_lock<mutex> lk(m_stepsMutex);
     auto it = m_steps.find(title);
     bool found = it != m_steps.end();
     if (found) {
         beginTimestamp = (*it).second;
         m_steps.erase(it);
     }
-    m_stepsMutex.unlock();
+    lk.unlock();
 
     if (found) {
         write(ProfilingType::TIME_EXEC, {std::to_string(beginTimestamp), title});
@@ -236,7 +236,6 @@ void UProfileImpl::setTimestampUnit(TimestampUnit tsUnit)
 
 void UProfileImpl::write(ProfilingType type, const std::list<std::string>& data)
 {
-    std::lock_guard<std::mutex> guard(m_fileMutex);
     if (m_file.is_open()) {
         const char csvSeparator = ';';
         std::string strType;
@@ -266,6 +265,7 @@ void UProfileImpl::write(ProfilingType type, const std::list<std::string>& data)
             strType = "undefined";
             break;
         }
+        std::lock_guard<std::mutex> guard(m_fileMutex);
         m_file << strType.c_str() << csvSeparator << getTimestamp();
         for (auto it = data.cbegin(); it != data.cend(); ++it) {
             m_file << csvSeparator << *it;

diff --git a/lib/util/cpumonitor.cpp b/lib/util/cpumonitor.cpp
@@ -14,10 +14,10 @@
 using namespace std;
 
 uprofile::CpuMonitor::CpuMonitor() :
-    m_nbCpus(getNumberOfCPUCores())
+    m_nbCpus(getNumberOfCPUCores()),
+    m_lastIdleTimes(m_nbCpus, 0),
+    m_lastTotalTimes(m_nbCpus, 0)
 {
-    m_lastIdleTimes.resize(m_nbCpus, 0);
-    m_lastTotalTimes.resize(m_nbCpus, 0);
 }
 
 uprofile::CpuMonitor::~CpuMonitor()
@@ -60,7 +60,7 @@ void uprofile::CpuMonitor::extractCpuTimes(const string& cpuInfo, size_t& idleTi
 
 vector<float> uprofile::CpuMonitor::getUsage()
 {
-    vector<float> usages;
+    vector<float> usages(m_nbCpus, 0);
 #if defined(__linux__)
     ifstream procStat("/proc/stat");
     // /proc/stat dumps the following info:
@@ -85,7 +85,7 @@ vector<float> uprofile::CpuMonitor::getUsage()
 
                 // To compute CPU load, we compute the time the CPU has been idle since the last read.
                 float cpuLoad = 100.0 * (1.0 - (float)(idleTime - m_lastIdleTimes[cpuIndex]) / (totalTime - m_lastTotalTimes[cpuIndex]));
-                usages.push_back(cpuLoad);
+                usages[cpuIndex] = cpuLoad;
 
                 // Save the times value for the next read
                 m_lastIdleTimes[cpuIndex] = idleTime;

diff --git a/lib/util/cpumonitor.h b/lib/util/cpumonitor.h
@@ -30,11 +30,10 @@ class CpuMonitor
     static size_t getNumberOfCPUCores();
     static void extractCpuTimes(const string& cpuInfo, size_t& idleTime, size_t& totalTime);
 
+    size_t m_nbCpus;
     // Store the last idle and total time for each CPU
     vector<size_t> m_lastIdleTimes;
     vector<size_t> m_lastTotalTimes;
-
-    size_t m_nbCpus;
 };
 }
 

diff --git a/lib/util/timer.cpp b/lib/util/timer.cpp
@@ -10,7 +10,6 @@
 #include "timer.h"
 
 Timer::Timer(int interval) :
-    m_th(NULL),
     m_interval(interval)
 {
 }
@@ -30,35 +29,34 @@ void Timer::setTimeout(const std::function<void(void)>& timeout)
     m_timeout = timeout;
 }
 
-bool Timer::isRunning()
+bool Timer::isRunning() const
 {
     std::lock_guard<std::mutex> lock(m_mutex);
     return m_running;
 }
 
 void Timer::start()
 {
-    if (m_interval > 0 && m_th == NULL) {
+    if (!m_th && m_interval > 0) {
         m_running = true;
-        m_th = new thread([=]() {
+        m_th = unique_ptr<thread>(new thread([=]() {
             while (isRunning()) {
                 this_thread::sleep_for(chrono::milliseconds(m_interval));
                 if (m_timeout) {
                     m_timeout();
                 }
             }
-        });
+        }));
     }
 }
 
 void Timer::stop()
 {
-    m_mutex.lock();
+    unique_lock<mutex> lk(m_mutex);
     m_running = false;
-    m_mutex.unlock();
+    lk.unlock();
     if (m_th) {
         m_th->join();
-        delete m_th;
-        m_th = NULL;
+        m_th.reset();
     }
 }
diff --git a/lib/util/timer.h b/lib/util/timer.h
@@ -13,6 +13,7 @@
 #include <chrono>
 #include <functional>
 #include <iostream>
+#include <memory>
 #include <mutex>
 #include <thread>
 
@@ -28,13 +29,13 @@ class Timer
     void setInterval(int interval);
     void start();
     void stop();
-    bool isRunning();
+    bool isRunning() const;
 
 private:
-    thread* m_th;
+    unique_ptr<thread> m_th;
     bool m_running;
     int m_interval;
-    std::mutex m_mutex;
+    mutable std::mutex m_mutex;
     std::function<void(void)> m_timeout;
 };