From: Thomas Markwalder Date: Wed, 28 Feb 2024 17:02:47 +0000 (-0500) Subject: [#3253] Added in-place duration update X-Git-Tag: Kea-2.5.7~67 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fc3eb168deee70624a38c9df93eac1bcdefb86a2;p=thirdparty%2Fkea.git [#3253] Added in-place duration update src/hooks/dhcp/perfmon/monitored_duration.* MonitoredDuration::MonitoredDuration() - new copy ctor DurationDataInterval::operator==() - added for UTs src/hooks/dhcp/perfmon/monitored_duration_store.* MonitoredDurationStore::addDurationSample() - new function for adding sample to duration in-place MonitoredDurationStore::addDuration() - removed optional sample parameter src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc addDurationSampleTest(uint16_t family) - new test func and tests src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc TEST(MonitoredDuration, copyConstructors) - new test --- diff --git a/src/hooks/dhcp/perfmon/monitored_duration.cc b/src/hooks/dhcp/perfmon/monitored_duration.cc index 19fc5e0456..acd274fe44 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration.cc +++ b/src/hooks/dhcp/perfmon/monitored_duration.cc @@ -49,6 +49,17 @@ DurationDataInterval::getAverageDuration() const { return (total_duration_ / occurrences_); } +bool +DurationDataInterval::operator==(const DurationDataInterval& other) const { + return ( + (start_time_ == other.start_time_) && + (occurrences_ == other.occurrences_) && + (min_duration_ == other.min_duration_) && + (max_duration_ == other.max_duration_) && + (total_duration_ == other.total_duration_) + ); +} + // DurationKey methods DurationKey::DurationKey(uint16_t family, @@ -223,6 +234,20 @@ MonitoredDuration::MonitoredDuration(const DurationKey& key, } } +MonitoredDuration::MonitoredDuration(const MonitoredDuration& rhs) + : DurationKey(rhs), + interval_duration_(rhs.interval_duration_), + current_interval_(0), + previous_interval_(0) { + if (rhs.current_interval_) { + current_interval_.reset(new DurationDataInterval(*rhs.current_interval_)); + } + + if (rhs.previous_interval_) { + previous_interval_.reset(new DurationDataInterval(*rhs.previous_interval_)); + } +} + bool MonitoredDuration::addSample(const Duration& sample) { auto now = PktEvent::now(); diff --git a/src/hooks/dhcp/perfmon/monitored_duration.h b/src/hooks/dhcp/perfmon/monitored_duration.h index 7dc91b7831..0eb552ee60 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration.h +++ b/src/hooks/dhcp/perfmon/monitored_duration.h @@ -94,6 +94,15 @@ public: /// @return Duration containing the average. Duration getAverageDuration() const; + /// @brief Equality operator. + /// + /// Primarily used for testing. + /// + /// equality operator to compare two DurationDataInterval objects. + /// @param other DurationDataInterval to be compared against. + /// @return True the keys are equal + bool operator==(const DurationDataInterval& other) const; + private: /// @brief Timestamp at which this interval began. Timestamp start_time_; @@ -275,6 +284,11 @@ public: /// @param interval_duration the interval duration MonitoredDuration(const DurationKey& key, const Duration& interval_duration); + /// @brief Copy Constructor + /// + /// @param rhs duration to copy + MonitoredDuration(const MonitoredDuration& rhs); + /// @brief Destructor virtual ~MonitoredDuration() = default; diff --git a/src/hooks/dhcp/perfmon/monitored_duration_store.cc b/src/hooks/dhcp/perfmon/monitored_duration_store.cc index 7e20708b02..1f60e2f4de 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration_store.cc +++ b/src/hooks/dhcp/perfmon/monitored_duration_store.cc @@ -47,18 +47,44 @@ MonitoredDurationStore::validateKey(const std::string& label, DurationKeyPtr key } MonitoredDurationPtr -MonitoredDurationStore::addDuration(DurationKeyPtr key, - const Duration& sample /* = ZERO_DURATION()*/) { +MonitoredDurationStore::addDurationSample(DurationKeyPtr key, const Duration& sample) { + validateKey("addDurationSample", key); + + MultiThreadingLock lock(*mutex_); + 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: " + << key->getLabel()); + } + } + + // Nothing to report. + return(MonitoredDurationPtr()); +} + +MonitoredDurationPtr +MonitoredDurationStore::addDuration(DurationKeyPtr key) { validateKey("addDuration", key); // Create the duration instance. MonitoredDurationPtr mond; try { mond.reset(new MonitoredDuration(*key, interval_duration_)); - // Add the first sample if provided. - if (sample > DurationDataInterval::ZERO_DURATION()) { - mond->addSample(sample); - } } catch (const std::exception& ex) { isc_throw(BadValue, "MonitoredDurationStore::addDuration failed: " << ex.what()); } @@ -78,6 +104,7 @@ MonitoredDurationStore::addDuration(DurationKeyPtr key, return (MonitoredDurationPtr(new MonitoredDuration(*mond))); } + MonitoredDurationPtr MonitoredDurationStore::getDuration(DurationKeyPtr key) { validateKey("getDuration", key); diff --git a/src/hooks/dhcp/perfmon/monitored_duration_store.h b/src/hooks/dhcp/perfmon/monitored_duration_store.h index b5d9f9a912..4e25edfaa2 100644 --- a/src/hooks/dhcp/perfmon/monitored_duration_store.h +++ b/src/hooks/dhcp/perfmon/monitored_duration_store.h @@ -25,8 +25,8 @@ namespace isc { namespace perfmon { -/// @brief Exception thrown when an attempt was made to add a duplicate key -/// to either the duration or alarm stores. +/// @brief Exception thrown when an attempt was made to add a duplicate duration +/// to the store. class DuplicateDurationKey : public Exception { public: DuplicateDurationKey(const char* file, size_t line, const char* what) : @@ -82,16 +82,33 @@ public: /// @brief Destructor ~MonitoredDurationStore() = default; + /// @brief Adds a sample to a duration in-place. + /// + /// If the duration exists in the store then the MonitoredDuration::addSample() + /// is invoked on the in-store duration. If this returns true, indicating a reportable + /// condition, then a copy of the in-store duration is returned, otherwise an empty + /// pointer is returned. + /// + /// If The duration does not exist in the store, then one is created and inserted + /// into the store after adding the sample. An empty pointer is returned. + /// + /// This function does not/must not modify any index keys. + /// + /// @param key key value of the duration to which to add. + /// @param sample duration value to add + /// + /// @return A copy of the updated duration if it should be reported, an empty + /// pointer otherwise. + MonitoredDurationPtr addDurationSample(DurationKeyPtr key, const Duration& sample); + /// @brief Creates a new MonitoredDuration and adds it to the store /// /// @param key key value of the duration to create. - /// @param sample An initial sample to add to the duration if not zero. /// /// @return pointer to the newly created duration. /// @throw DuplicateDuration if a duration for the given key already exists in /// the store. - MonitoredDurationPtr addDuration(DurationKeyPtr key, const Duration& sample - = DurationDataInterval::ZERO_DURATION()); + MonitoredDurationPtr addDuration(DurationKeyPtr key); /// @brief Fetches a duration from the store for a given key. /// 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 5b94d04d3f..338fe01e40 100644 --- a/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/monitored_duration_store_unittests.cc @@ -98,8 +98,7 @@ public: // Add four durations with decreaing subnet ids. for (int subnet = 4; subnet > 0; --subnet) { MonitoredDurationPtr mond; - ASSERT_NO_THROW_LOG(mond = store.addDuration(makeKey(family, subnet), - interval_duration)); + ASSERT_NO_THROW_LOG(mond = store.addDuration(makeKey(family, subnet))); ASSERT_TRUE(mond); monds.push_back(mond); } @@ -137,11 +136,11 @@ public: // Add a duration. MonitoredDurationPtr mond; - ASSERT_NO_THROW_LOG(mond = store.addDuration(makeKey(family), interval_duration)); + ASSERT_NO_THROW_LOG(mond = store.addDuration(makeKey(family))); ASSERT_TRUE(mond); // Attempting to add it again should evoke a duplicate key exception. - ASSERT_THROW(store.addDuration(mond, interval_duration), DuplicateDurationKey); + ASSERT_THROW(store.addDuration(mond), DuplicateDurationKey); } /// @brief Verifies that duration key must be valid to add a duration to the store. @@ -153,12 +152,12 @@ public: MonitoredDurationStorePtr store(new MonitoredDurationStore(AF_INET, interval_duration)); // Attempting to add with an empty key should throw. - ASSERT_THROW_MSG(store->addDuration(DurationKeyPtr(), interval_duration), + ASSERT_THROW_MSG(store->addDuration(DurationKeyPtr()), BadValue, "MonitoredDurationStore::addDuration - key is empty"); // Attempting to a v6 key should fail. - ASSERT_THROW_MSG(store->addDuration(makeKey(AF_INET6), interval_duration), + ASSERT_THROW_MSG(store->addDuration(makeKey(AF_INET6)), BadValue, "MonitoredDurationStore::addDuration" " - family mismatch, key is v6, store is v4"); @@ -167,7 +166,7 @@ public: store.reset(new MonitoredDurationStore(AF_INET6, interval_duration)); // Attempting to add a v4 key should fail. - ASSERT_THROW_MSG(store->addDuration(makeKey(AF_INET), interval_duration), + ASSERT_THROW_MSG(store->addDuration(makeKey(AF_INET)), BadValue, "MonitoredDurationStore::addDuration" " - family mismatch, key is v4, store is v6"); @@ -280,6 +279,7 @@ public: // Verify it has the expected current interval. DurationDataIntervalPtr current; ASSERT_TRUE(current = found->getCurrentInterval()); + ASSERT_NE(current, mond->getCurrentInterval()); EXPECT_EQ(current->getOccurrences(), 1); EXPECT_EQ(current->getTotalDuration(), milliseconds(75)); } @@ -326,6 +326,106 @@ public: "MonitoredDurationStore::updateDuration" " - family mismatch, key is v4, store is v6"); } + + /// @brief Exercises addDurationSample() valid behavior. + /// + /// @param family protocol family to test, AF_INET or AF_INET6 + void addDurationSampleTest(uint16_t family) { + // Create a store. + Duration interval_duration(milliseconds(50)); + MonitoredDurationStore store(family, interval_duration); + + // Create valid key. + DurationKeyPtr key = makeKey(family); + + // Add a 5 ms sample for the key. + MonitoredDurationPtr mond; + Duration five_ms(milliseconds(5)); + ASSERT_NO_THROW_LOG(mond = store.addDurationSample(key, five_ms)); + + // Should return an empty pointer as nothing to report yet. + EXPECT_FALSE(mond); + + // Make sure the duration was created and stored, and has only + // the current interval with 1 occurrence and a total of 5 ms. + ASSERT_NO_THROW_LOG(mond = store.getDuration(key)); + ASSERT_TRUE(mond); + auto current_interval = mond->getCurrentInterval(); + ASSERT_TRUE(current_interval); + EXPECT_EQ(current_interval->getOccurrences(), 1); + EXPECT_EQ(current_interval->getTotalDuration(), (five_ms)); + auto previous_interval = mond->getPreviousInterval(); + ASSERT_FALSE(previous_interval); + + // Now lets add a second sample. We should still be inside the + // interval, so it still should return an empty pointer. + ASSERT_NO_THROW_LOG(mond = store.addDurationSample(key, five_ms)); + EXPECT_FALSE(mond); + + // Make sure the duration's current interval (only) was updated + ASSERT_NO_THROW_LOG(mond = store.getDuration(key)); + ASSERT_TRUE(mond); + current_interval = mond->getCurrentInterval(); + ASSERT_TRUE(current_interval); + EXPECT_EQ(current_interval->getOccurrences(), 2); + EXPECT_EQ(current_interval->getTotalDuration(), (five_ms * 2)); + previous_interval = mond->getPreviousInterval(); + ASSERT_FALSE(previous_interval); + + // Sleep til past the end of interval + usleep(60 * 1000); + + // Now lets add a third sample. We are past the end of the + // interval, so it still return the duration. + ASSERT_NO_THROW_LOG(mond = store.addDurationSample(key, five_ms)); + ASSERT_TRUE(mond); + + // Make sure the duration's current interval and prevous intervals correct. + current_interval = mond->getCurrentInterval(); + ASSERT_TRUE(current_interval); + EXPECT_EQ(current_interval->getOccurrences(), 1); + EXPECT_EQ(current_interval->getTotalDuration(), (five_ms)); + + previous_interval = mond->getPreviousInterval(); + ASSERT_TRUE(previous_interval); + EXPECT_EQ(previous_interval->getOccurrences(), 2); + EXPECT_EQ(previous_interval->getTotalDuration(), (five_ms) * 2); + } + + /// @brief Test tool for gauging speed. + /// + /// This test is really just a development tool for gauging performance. + /// It does not pass or fail. + /// + /// @param family protocol family to test, AF_INET or AF_INET6 + void speedCheckTest(uint16_t family) { + // Create a store. + Duration interval_duration(milliseconds(5)); + MonitoredDurationStore store(family, interval_duration); + + // Create keys. + size_t num_subnets = 100; + std::vector keys; + + for (int s = 0; s < num_subnets; ++s) { + keys.push_back(makeKey(family, s)); + } + size_t num_passes = 100; + size_t report_count = 0; + Duration two_us(microseconds(2)); + for (int p = 0; p < num_passes; ++p) { + for (auto k : keys) { + if (store.addDurationSample(k, two_us)) { + ++report_count; + } + } + } + + std::cout << "report count: " << report_count << std::endl; + auto durations = store.getAll(); + EXPECT_EQ(durations->size(), 100); + + } }; TEST_F(MonitoredDurationStoreTest, addDuration) { @@ -427,4 +527,41 @@ TEST_F(MonitoredDurationStoreTest, updateDurationInvalidMultiThreading) { updateDurationInvalidTest(); } +TEST_F(MonitoredDurationStoreTest, addDurationSample) { + addDurationSampleTest(AF_INET); +} + +TEST_F(MonitoredDurationStoreTest, addDurationSampleMultiThreading) { + MultiThreadingTest mt; + addDurationSampleTest(AF_INET); +} + +TEST_F(MonitoredDurationStoreTest, addDurationSample6) { + addDurationSampleTest(AF_INET6); +} + +TEST_F(MonitoredDurationStoreTest, addDurationSample6MultiThreading) { + MultiThreadingTest mt; + addDurationSampleTest(AF_INET6); +} + +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 diff --git a/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc b/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc index e9223472ac..4472902b1b 100644 --- a/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/monitored_duration_unittests.cc @@ -289,6 +289,72 @@ TEST(MonitoredDuration, validConstructors) { EXPECT_FALSE(mond->getPreviousInterval()); } +// Verifies Copy construction. Since current and preivous intervals are not +// exposed, this test relies on addSample() to alter them. +TEST(MonitoredDuration, copyConstructors) { + MonitoredDurationPtr mond; + Duration interval_duration(microseconds(10)); + + // Create valid v4 duration, verify contents and label. + ASSERT_NO_THROW_LOG(mond.reset(new MonitoredDuration(AF_INET, DHCPDISCOVER, DHCPOFFER, + "process_started", "process_completed", + SUBNET_ID_GLOBAL, interval_duration))); + + // Make a copy. + MonitoredDurationPtr duplicate; + duplicate.reset(new MonitoredDuration(*mond)); + + // Should have different pointers. + EXPECT_NE(duplicate, mond); + + // Key values should be equal (DurationKey::== operator). + EXPECT_EQ(*duplicate, *mond); + + // Check non-key members. + EXPECT_EQ(duplicate->getIntervalDuration(),mond->getIntervalDuration()); + EXPECT_FALSE(duplicate->getCurrentInterval()); + EXPECT_FALSE(duplicate->getPreviousInterval()); + + // Add a sample to the original. + EXPECT_FALSE(mond->addSample(microseconds(2))); + + // Make a new copy. + duplicate.reset(new MonitoredDuration(*mond)); + + // Current intervals should exist. + ASSERT_TRUE(mond->getCurrentInterval()); + ASSERT_TRUE(duplicate->getCurrentInterval()); + // They should not be the same object but the content should be equal. + EXPECT_NE(duplicate->getCurrentInterval(), mond->getCurrentInterval()); + EXPECT_EQ(*duplicate->getCurrentInterval(), *mond->getCurrentInterval()); + // Previous interval should not exist. + ASSERT_FALSE(mond->getPreviousInterval()); + ASSERT_FALSE(duplicate->getPreviousInterval()); + + // Sleep past interval duration. + usleep(20); + + // Add anoter sample to the original. + EXPECT_TRUE(mond->addSample(microseconds(2))); + + // Make a new copy. + duplicate.reset(new MonitoredDuration(*mond)); + + // Current intervals should exist. + ASSERT_TRUE(mond->getCurrentInterval()); + ASSERT_TRUE(duplicate->getCurrentInterval()); + // They should not be the same object but the content should be equal. + EXPECT_NE(duplicate->getCurrentInterval(), mond->getCurrentInterval()); + EXPECT_EQ(*duplicate->getCurrentInterval(), *mond->getCurrentInterval()); + + // Current previous should exist. + ASSERT_TRUE(mond->getPreviousInterval()); + ASSERT_TRUE(duplicate->getPreviousInterval()); + // They should not be the same object but the content should be equal. + EXPECT_NE(duplicate->getPreviousInterval(), mond->getPreviousInterval()); + EXPECT_EQ(*duplicate->getPreviousInterval(), *mond->getPreviousInterval()); +} + // Verifies MonitoredDuration invalid construction. TEST(MonitoredDuration, invalidConstructors) { MonitoredDurationPtr mond;