[#3253] Use modify() to do inplace updates

src/hooks/dhcp/perfmon/alarm_store.cc
    AlarmStore::checkDurationSample() - revamped to use multi_index::modify

src/hooks/dhcp/perfmon/monitored_duration_store.*
    MonitoredDurationStore::addDurationSample() - revamped to use multi_index::modify
    Added index on current interval start time

src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc
    Took out speed check UTs, left function

diff --git a/src/hooks/dhcp/perfmon/alarm_store.cc b/src/hooks/dhcp/perfmon/alarm_store.cc
index d0d2e3e5de03b2996f0bd433dbd9e17b269cd2f6..41b76c06d51f80709f758d9403fdfdf090f9b511 100644 (file)
--- a/src/hooks/dhcp/perfmon/alarm_store.cc
+++ b/src/hooks/dhcp/perfmon/alarm_store.cc
@@ -50,10 +50,22 @@ AlarmStore::checkDurationSample(DurationKeyPtr key, const Duration& sample,
 
     // If we find an alarm the check the sample.  Alarm::checkSample()
     //does not alter the key so it can be done in-place.
-    if (alarm_iter != index.end() &&
-        (*alarm_iter)->checkSample(sample, report_interval)) {
-        // Alarm state needs to be reported, return a copy of the alarm.
-        return (AlarmPtr(new Alarm(**alarm_iter)));
+    if (alarm_iter != index.end()) {
+        bool should_report = false;
+        bool modified = index.modify(alarm_iter,
+                                     [sample, report_interval, &should_report](AlarmPtr alarm) {
+            should_report = alarm->checkSample(sample, report_interval);
+        });
+
+        if (!modified) {
+            // Possible but unlikely.
+            isc_throw(Unexpected, "AlarmStore::checkDurationSample - modify failed for: " << key->getLabel());
+        }
+
+        if (should_report) {
+            // Alarm state needs to be reported, return a copy of the alarm.
+            return (AlarmPtr(new Alarm(**alarm_iter)));
+        }
     }
 
     // Nothing to alarm.
@@ -62,14 +74,11 @@ AlarmStore::checkDurationSample(DurationKeyPtr key, const Duration& sample,
 
 AlarmPtr
 AlarmStore::addAlarm(AlarmPtr alarm) {
-    {
-        MultiThreadingLock lock(*mutex_);
-        auto ret = alarms_.insert(alarm);
-        if (ret.second == false) {
-            isc_throw(DuplicateAlarm,
-                      "AlarmStore::addAlarm: alarm already exists for: "
-                      << alarm->getLabel());
-        }
+    MultiThreadingLock lock(*mutex_);
+    auto ret = alarms_.insert(alarm);
+    if (ret.second == false) {
+        isc_throw(DuplicateAlarm, "AlarmStore::addAlarm: alarm already exists for: "
+                  << alarm->getLabel());
     }
 
     // Return a copy of what we inserted.

diff --git a/src/hooks/dhcp/perfmon/monitored_duration.h b/src/hooks/dhcp/perfmon/monitored_duration.h
index 0eb552ee608bac5edb6fb606ad4a53886822d510..c784bb613769db25a5f45f86c55ee623ce759dba 100644 (file)
--- a/src/hooks/dhcp/perfmon/monitored_duration.h
+++ b/src/hooks/dhcp/perfmon/monitored_duration.h
@@ -313,6 +313,14 @@ public:
         return (current_interval_);
     }
 
+    /// @brief Get the current interval.
+    ///
+    /// @return Pointer to the current interval if it exists or an empty pointer.
+    Timestamp getCurrentIntervalStart() const {
+        return (current_interval_ ? current_interval_->getStartTime()
+                                  : dhcp::PktEvent::EMPTY_TIME());
+    }
+
     /// @brief Add a sample to the duration's current interval.
     ///
     /// If there is no current interval start a new one otherwise if the current

diff --git a/src/hooks/dhcp/perfmon/monitored_duration_store.cc b/src/hooks/dhcp/perfmon/monitored_duration_store.cc
index 7de5f6cc9f68fcf831685e180896d26f5fdeb2b1..ce2e006c7b614d4d4895c7d762a0134efe8358d4 100644 (file)
--- a/src/hooks/dhcp/perfmon/monitored_duration_store.cc
+++ b/src/hooks/dhcp/perfmon/monitored_duration_store.cc
@@ -54,23 +54,34 @@ MonitoredDurationStore::addDurationSample(DurationKeyPtr key, const Duration& sa
     auto& index = durations_.get<DurationKeyTag>();
     auto duration_iter = index.find(*key);
     if (duration_iter != index.end()) {
-        // Add sample does not change key so it can be done in place.
-        // If adding the sample returns true, then its time to report
-        // so return a copy.
-        if ((*duration_iter)->addSample(sample)) {
-            return (MonitoredDurationPtr(new MonitoredDuration(**duration_iter)));
-        }
-    } else {
-        // It doesn't exist, add it.
-        MonitoredDurationPtr mond(new MonitoredDuration(*key, interval_duration_));
-        static_cast<void>(mond->addSample(sample));
-        auto ret = durations_.insert(mond);
-        if (ret.second == false) {
-            // Shouldn't be possible.
-            isc_throw(DuplicateDurationKey,
-                      "MonitoredDurationStore::addDurationSample: duration already exists for: "
+        bool should_report = false;
+        // Modify updates in place and only re-indexes if keys change.
+        bool modified = index.modify(duration_iter,
+                               [sample, &should_report](MonitoredDurationPtr mond){
+            should_report = mond->addSample(sample);
+        });
+
+        if (!modified) {
+            // Possible but unlikely.
+            isc_throw(Unexpected,
+                      "MonitoredDurationStore::addDurationSample - modify failed for: "
                       << key->getLabel());
         }
+
+        // If it's time to report return a copy otherwise an empty pointer.
+        return (should_report ? MonitoredDurationPtr(new MonitoredDuration(**duration_iter))
+                              : MonitoredDurationPtr());
+    }
+
+    // It doesn't exist, add it.
+    MonitoredDurationPtr mond(new MonitoredDuration(*key, interval_duration_));
+    static_cast<void>(mond->addSample(sample));
+    auto ret = durations_.insert(mond);
+    if (ret.second == false) {
+        // Shouldn't be possible.
+        isc_throw(DuplicateDurationKey,
+                  "MonitoredDurationStore::addDurationSample: duration already exists for: "
+                  << key->getLabel());
     }
 
     // Nothing to report.

diff --git a/src/hooks/dhcp/perfmon/monitored_duration_store.h b/src/hooks/dhcp/perfmon/monitored_duration_store.h
index f0acfba69f6e5d72fe0d57b10c034bd524b26c4a..e4f7536a9ec912167cfbc3d9a74ec478bd79effa 100644 (file)
--- a/src/hooks/dhcp/perfmon/monitored_duration_store.h
+++ b/src/hooks/dhcp/perfmon/monitored_duration_store.h
@@ -36,6 +36,9 @@ public:
 /// @brief Tag for index by primary key (DurationKey).
 struct DurationKeyTag { };
 
+/// @brief Tag for index by interval start time.
+struct IntervalStartTag { };
+
 /// @brief A multi index container holding pointers to MonitoredDurations.
 ///
 /// The durations in the container may be accessed using different indexes:
@@ -54,6 +57,14 @@ typedef boost::multi_index_container<
         boost::multi_index::ordered_unique<
             boost::multi_index::tag<DurationKeyTag>,
             boost::multi_index::identity<DurationKey>
+        >,
+
+        // Specification of the second index starts here.
+        // This index sorts durations by current interval start time.
+        boost::multi_index::ordered_non_unique<
+            boost::multi_index::tag<IntervalStartTag>,
+            boost::multi_index::const_mem_fun<MonitoredDuration, Timestamp,
+                                              &MonitoredDuration::getCurrentIntervalStart>
         >
     >
 > MonitoredDurationContainer;

diff --git a/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc b/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc
index 038d71120d118dad89162115380a46fcb43f76d6..6182a07627275184993e48359e273db2efde357e 100644 (file)
--- a/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc
@@ -376,7 +376,7 @@ public:
         usleep(60 * 1000);
 
         // Now lets add a third sample. We are past the end of the
-        // interval, so it still return the duration.
+        // interval, so it should return the duration.
         ASSERT_NO_THROW_LOG(mond = store.addDurationSample(key, five_ms));
         ASSERT_TRUE(mond);
 
@@ -392,16 +392,16 @@ public:
         EXPECT_EQ(previous_interval->getTotalDuration(), (five_ms) * 2);
     }
 
-    /// @todo TAKE THIS OUT
     /// @brief Test tool for gauging speed.
     ///
     /// This test is really just a development tool for gauging performance.
-    /// It does not pass or fail.
+    /// of adding duration samples. It does not pass or fail and thus is not
+    /// included in explicit UTs.
     ///
     /// @param family protocol family to test, AF_INET or AF_INET6
-    void speedCheckTest(uint16_t family) {
+    void speedCheck(uint16_t family) {
         // Create a store.
-        Duration interval_duration(milliseconds(5));
+        Duration interval_duration(microseconds(100));
         MonitoredDurationStore store(family, interval_duration);
 
         // Create keys.
@@ -437,7 +437,8 @@ public:
         auto durations = store.getAll();
         EXPECT_EQ(durations->size(), num_subnets);
 
-        std::cout << "add keys time   : " << (add_keys_time - start_time) << std::endl
+        std::cout << "report count: " << report_count << std::endl
+                  << "add keys time   : " << (add_keys_time - start_time) << std::endl
                   << "add samples time: " << (add_samples_time - add_keys_time) << std::endl
                   << "time per sample: "
                   << (add_samples_time - add_keys_time) / (num_subnets * num_passes) << std::endl;
@@ -561,23 +562,4 @@ TEST_F(MonitoredDurationStoreTest, addDurationSample6MultiThreading) {
     addDurationSampleTest(AF_INET6);
 }
 
-/// @todo TAKE THESE OUT
-TEST_F(MonitoredDurationStoreTest, speedCheck) {
-    speedCheckTest(AF_INET);
-}
-
-TEST_F(MonitoredDurationStoreTest, speedCheckMultiThreading) {
-    MultiThreadingTest mt;
-    speedCheckTest(AF_INET);
-}
-
-TEST_F(MonitoredDurationStoreTest, speedCheck6) {
-    speedCheckTest(AF_INET6);
-}
-
-TEST_F(MonitoredDurationStoreTest, speedCheck6MultiThreading) {
-    MultiThreadingTest mt;
-    speedCheckTest(AF_INET6);
-}
-
 } // end of anonymous namespace