From 3d99b483dbdc260f69b2e8d11d3f545db6dda48e Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 4 Mar 2024 11:41:17 -0500 Subject: [PATCH] [#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 --- src/hooks/dhcp/perfmon/alarm_store.cc | 33 +++++++++------ src/hooks/dhcp/perfmon/monitored_duration.h | 8 ++++ .../dhcp/perfmon/monitored_duration_store.cc | 41 ++++++++++++------- .../dhcp/perfmon/monitored_duration_store.h | 11 +++++ .../monitored_duration_store_unittests.cc | 32 ++++----------- 5 files changed, 73 insertions(+), 52 deletions(-) diff --git a/src/hooks/dhcp/perfmon/alarm_store.cc b/src/hooks/dhcp/perfmon/alarm_store.cc index d0d2e3e5de..41b76c06d5 100644 --- 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 0eb552ee60..c784bb6137 100644 --- 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 7de5f6cc9f..ce2e006c7b 100644 --- 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(); 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(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(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 f0acfba69f..e4f7536a9e 100644 --- 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, boost::multi_index::identity + >, + + // 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, + boost::multi_index::const_mem_fun > > > 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 038d71120d..6182a07627 100644 --- 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 -- 2.47.3