If the performance tracker (implemented as global singleton) is enabled and tests are run in parallel, race conditions may occur if more then one thread want to add a tracking entry (multiple concurrent push_back calls to a std::vector). To prevent this, a std::scoped_lock is inserted into each modifying performance tracker function. One downside being: the copy and move onstructors and assignment operators must now be deleted.

Author: breyerml

include/plssvm/detail/tracking/performance_tracker.hpp

@@ -36,6 +36,7 @@
 
 #include <chrono>       // std::chrono::steady_clock::time_point
 #include <map>          // std::map
+#include <mutex>        // std::mutex, std::scoped_lock
 #include <optional>     // std::optional
 #include <ostream>      // std::ostream
 #include <string>       // std::string
@@ -145,23 +146,23 @@ class performance_tracker {
      */
     performance_tracker();
     /**
-     * @brief Default copy-constructor. **Must** be implemented in .cpp file.
+     * @brief Delete copy-constructor due to usage of a std::mutex.
      */
-    performance_tracker(const performance_tracker &);
+    performance_tracker(const performance_tracker &) = delete;
     /**
-     * @brief Default move-constructor. **Must** be implemented in .cpp file.
+     * @brief Delete move-constructordue to usage of a std::mutex.
      */
-    performance_tracker(performance_tracker &&) noexcept;
+    performance_tracker(performance_tracker &&) noexcept = delete;
     /**
-     * @brief Default copy-assignment operator. **Must** be implemented in .cpp file.
+     * @brief Delete copy-assignment operator due to usage of a std::mutex.
      * @return `*this`
      */
-    performance_tracker &operator=(const performance_tracker &);
+    performance_tracker &operator=(const performance_tracker &) = delete;
     /**
-     * @brief Default move-assignment operator. **Must** be implemented in .cpp file.
+     * @brief Delete move-assignment operator due to usage of a std::mutex.
      * @return `*this`
      */
-    performance_tracker &operator=(performance_tracker &&) noexcept;
+    performance_tracker &operator=(performance_tracker &&) noexcept = delete;
     /**
      * @brief Default destructor. **Must** be implemented in .cpp file.
      */
@@ -299,12 +300,16 @@ class performance_tracker {
     events events_{};
     /// Flag indicating whether tracking is currently enabled or disabled. Tracking is enabled by default.
     bool is_tracking_{ true };
+    /// Standard mutex to prevent race conditions if multiple threads try to add a tracking entry simultaneously.
+    std::mutex mutex_;
 };
 
 template <typename T>
 void performance_tracker::add_tracking_entry(const tracking_entry<T> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         std::string entry_value_str{};
         if constexpr (std::is_same_v<T, std::string> || std::is_same_v<T, std::string_view>) {
             // escape strings with "" since they may contain whitespaces
@@ -336,6 +341,8 @@ template <typename T>
 void performance_tracker::add_tracking_entry(const tracking_entry<std::vector<T>> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         std::string entry_value_str{};
         if constexpr (std::is_same_v<T, std::string> || std::is_same_v<T, std::string_view>) {
             // escape strings with "" since they may contain whitespaces

src/plssvm/detail/tracking/performance_tracker.cpp

@@ -74,6 +74,7 @@
 #include <fstream>      // std::ofstream
 #include <iostream>     // std::ios_base::app, std::ostream, std::clog, std::endl
 #include <map>          // std::map
+#include <mutex>        // std::mutex, std::scoped_lock
 #include <string>       // std::string
 #include <string_view>  // std::string_view
 #include <utility>      // std::move
@@ -135,15 +136,13 @@ namespace plssvm::detail::tracking {
 
 // Must be explicitly defaulted in the cpp file to prevent linker errors!
 performance_tracker::performance_tracker() = default;
-performance_tracker::performance_tracker(const performance_tracker &) = default;
-performance_tracker::performance_tracker(performance_tracker &&) noexcept = default;
-performance_tracker &performance_tracker::operator=(const performance_tracker &) = default;
-performance_tracker &performance_tracker::operator=(performance_tracker &&) noexcept = default;
 performance_tracker::~performance_tracker() = default;
 
 void performance_tracker::add_tracking_entry(const tracking_entry<plssvm::parameter> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         // create category
         tracking_entries_.emplace(entry.entry_category, std::map<std::string, std::vector<std::string>>{});
         // fill category with value
@@ -161,6 +160,8 @@ void performance_tracker::add_tracking_entry([[maybe_unused]] const tracking_ent
 #if defined(PLSSVM_HAS_MPI_ENABLED)
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         // create category
         tracking_entries_.emplace(entry.entry_category, std::map<std::string, std::vector<std::string>>{});
         // fill category with value
@@ -177,6 +178,8 @@ void performance_tracker::add_tracking_entry([[maybe_unused]] const tracking_ent
 void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_train> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         // create category
         tracking_entries_.emplace(entry.entry_category, std::map<std::string, std::vector<std::string>>{});
         // fill category with value
@@ -204,6 +207,8 @@ void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_tr
 void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_predict> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         // create category
         tracking_entries_.emplace(entry.entry_category, std::map<std::string, std::vector<std::string>>{});
         // fill category with value
@@ -224,6 +229,8 @@ void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_pr
 void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_scale> &entry) {
     // check whether entries should currently be tracked
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
         // create category
         tracking_entries_.emplace(entry.entry_category, std::map<std::string, std::vector<std::string>>{});
         // fill category with value
@@ -243,8 +250,10 @@ void performance_tracker::add_tracking_entry(const tracking_entry<cmd::parser_sc
 #if defined(PLSSVM_HARDWARE_SAMPLING_ENABLED)
 void performance_tracker::add_hws_entry(const hws::system_hardware_sampler &entry) {
     // check whether entries should currently be tracked
-    const std::string entry_category{ "hardware_sampler" };
     if (this->is_tracking()) {
+        const std::scoped_lock guard{ mutex_ };
+
+        const std::string entry_category{ "hardware_sampler" };
         for (const std::unique_ptr<hws::hardware_sampler> &sampler : entry.samplers()) {
             // get the sample string and append two newlines to each line
             std::string sample_str = sampler->samples_only_as_yaml_string();
@@ -266,10 +275,14 @@ void performance_tracker::add_hws_entry(const hws::system_hardware_sampler &entr
 #endif
 
 void performance_tracker::add_event(std::string name) {
+    const std::scoped_lock guard{ mutex_ };
+
     events_.add_event(std::chrono::steady_clock::now(), std::move(name));
 }
 
 void performance_tracker::set_reference_time(const std::chrono::steady_clock::time_point time) noexcept {
+    const std::scoped_lock guard{ mutex_ };
+
     reference_time_ = time;
 }
 
@@ -493,17 +506,29 @@ void performance_tracker::save(std::ostream &out) {
     }
 }
 
-void performance_tracker::pause_tracking() noexcept { is_tracking_ = false; }
+void performance_tracker::pause_tracking() noexcept {
+    const std::scoped_lock guard{ mutex_ };
+
+    is_tracking_ = false;
+}
 
-void performance_tracker::resume_tracking() noexcept { is_tracking_ = true; }
+void performance_tracker::resume_tracking() noexcept {
+    const std::scoped_lock guard{ mutex_ };
+
+    is_tracking_ = true;
+}
 
 bool performance_tracker::is_tracking() const noexcept { return is_tracking_; }
 
 const std::map<std::string, std::map<std::string, std::vector<std::string>>> &performance_tracker::get_tracking_entries() const noexcept { return tracking_entries_; }
 
 const events &performance_tracker::get_events() const noexcept { return events_; }
 
-void performance_tracker::clear_tracking_entries() noexcept { tracking_entries_.clear(); }
+void performance_tracker::clear_tracking_entries() noexcept {
+    const std::scoped_lock guard{ mutex_ };
+
+    tracking_entries_.clear();
+}
 
 performance_tracker &global_performance_tracker() {
     static performance_tracker tracker;

tests/detail/tracking/performance_tracker.cpp

@@ -29,12 +29,10 @@
 #include <algorithm>   // std::transform
 #include <array>       // std::array
 #include <chrono>      // std::chrono_literals namespace
-#include <cstddef>     // std::size_t
 #include <filesystem>  // std::filesystem::is_empty
 #include <iostream>    // std::cout, std::clog
 #include <map>         // std::map
 #include <string>      // std::string
-#include <utility>     // std::move
 #include <vector>      // std::vector
 
 using namespace plssvm::detail::literals;  // NOLINT(google-build-using-namespace): only imports custom user-defined literals into this namespace
@@ -111,132 +109,6 @@ class PerformanceTracker : public ::testing::Test,
     plssvm::detail::tracking::performance_tracker tracker_;
 };
 
-TEST_F(PerformanceTracker, CopyConstruct) {
-    // get performance tracker from fixture class
-    plssvm::detail::tracking::performance_tracker &tracker = this->get_performance_tracker();
-
-    // add different tracking entries
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "bar", 42 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "baz", 3.1415 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "mem", 1_KiB });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'a' });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'b' });
-
-    // copy-construct new performance tracker
-    const plssvm::detail::tracking::performance_tracker tracker2{ tracker };
-
-    // check the contents
-    EXPECT_EQ(tracker2.get_tracking_entries(), tracker.get_tracking_entries());
-    ASSERT_EQ(tracker2.get_events().num_events(), tracker.get_events().num_events());
-    for (std::size_t i = 0; i < tracker.get_events().num_events(); ++i) {
-        EXPECT_EQ(tracker2.get_events()[i].time_point, tracker.get_events()[i].time_point);
-        EXPECT_EQ(tracker2.get_events()[i].name, tracker.get_events()[i].name);
-    }
-    EXPECT_EQ(tracker2.get_reference_time(), tracker.get_reference_time());
-    EXPECT_EQ(tracker2.is_tracking(), tracker.is_tracking());
-}
-
-TEST_F(PerformanceTracker, MoveConstruct) {
-    // get performance tracker from fixture class
-    plssvm::detail::tracking::performance_tracker &tracker = this->get_performance_tracker();
-
-    // add different tracking entries
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "bar", 42 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "baz", 3.1415 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "mem", 1_KiB });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'a' });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'b' });
-
-    // save (i.e. copy contents as ground truth)
-    const auto entries = tracker.get_tracking_entries();
-    const auto events = tracker.get_events();
-    const auto reference_time = tracker.get_reference_time();
-    const bool is_tracking = tracker.is_tracking();
-
-    // move-construct new performance tracker
-    const plssvm::detail::tracking::performance_tracker tracker2{ std::move(tracker) };
-
-    // check the contents
-    EXPECT_EQ(tracker2.get_tracking_entries(), entries);
-    ASSERT_EQ(tracker2.get_events().num_events(), events.num_events());
-    for (std::size_t i = 0; i < events.num_events(); ++i) {
-        EXPECT_EQ(tracker2.get_events()[i].time_point, events[i].time_point);
-        EXPECT_EQ(tracker2.get_events()[i].name, events[i].name);
-    }
-    EXPECT_EQ(tracker2.get_reference_time(), reference_time);
-    EXPECT_EQ(tracker2.is_tracking(), is_tracking);
-
-    // check moved-from state
-    // NOLINTBEGIN: use after move wanted since this is tested here
-    EXPECT_TRUE(tracker.get_tracking_entries().empty());
-    EXPECT_TRUE(tracker.get_events().empty());
-    // NOLINTEND
-}
-
-TEST_F(PerformanceTracker, CopyAssign) {
-    // get performance tracker from fixture class
-    plssvm::detail::tracking::performance_tracker &tracker = this->get_performance_tracker();
-
-    // add different tracking entries
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "bar", 42 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "baz", 3.1415 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "mem", 1_KiB });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'a' });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'b' });
-
-    // default-construct new performance tracker then copy-assign another performance tracker
-    plssvm::detail::tracking::performance_tracker tracker2{};
-    tracker2 = tracker;
-
-    // check the contents
-    EXPECT_EQ(tracker2.get_tracking_entries(), tracker.get_tracking_entries());
-    ASSERT_EQ(tracker2.get_events().num_events(), tracker.get_events().num_events());
-    for (std::size_t i = 0; i < tracker.get_events().num_events(); ++i) {
-        EXPECT_EQ(tracker2.get_events()[i].time_point, tracker.get_events()[i].time_point);
-        EXPECT_EQ(tracker2.get_events()[i].name, tracker.get_events()[i].name);
-    }
-    EXPECT_EQ(tracker2.get_reference_time(), tracker.get_reference_time());
-    EXPECT_EQ(tracker2.is_tracking(), tracker.is_tracking());
-}
-
-TEST_F(PerformanceTracker, MoveAssign) {
-    // get performance tracker from fixture class
-    plssvm::detail::tracking::performance_tracker &tracker = this->get_performance_tracker();
-
-    // add different tracking entries
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "bar", 42 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "baz", 3.1415 });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "foo", "mem", 1_KiB });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'a' });
-    tracker.add_tracking_entry(plssvm::detail::tracking::tracking_entry{ "", "foobar", 'b' });
-
-    // save (i.e. copy contents as ground truth)
-    const auto entries = tracker.get_tracking_entries();
-    const auto events = tracker.get_events();
-    const auto reference_time = tracker.get_reference_time();
-    const bool is_tracking = tracker.is_tracking();
-
-    // default-construct new performance tracker then move-assign another performance tracker
-    plssvm::detail::tracking::performance_tracker tracker2{};
-    tracker2 = std::move(tracker);
-
-    // check the contents
-    EXPECT_EQ(tracker2.get_tracking_entries(), entries);
-    ASSERT_EQ(tracker2.get_events().num_events(), events.num_events());
-    for (std::size_t i = 0; i < events.num_events(); ++i) {
-        EXPECT_EQ(tracker2.get_events()[i].time_point, events[i].time_point);
-        EXPECT_EQ(tracker2.get_events()[i].name, events[i].name);
-    }
-    EXPECT_EQ(tracker2.get_reference_time(), reference_time);
-    EXPECT_EQ(tracker2.is_tracking(), is_tracking);
-
-    // check moved-from state
-    // NOLINTBEGIN: use after move wanted since this is tested here
-    EXPECT_TRUE(tracker.get_tracking_entries().empty());
-    EXPECT_TRUE(tracker.get_events().empty());
-    // NOLINTEND
-}
-
 // the macros are only available if PLSSVM_PERFORMANCE_TRACKER_ENABLED is defined!
 #if defined(PLSSVM_PERFORMANCE_TRACKER_ENABLED)