From 8ab2236cddd301ff891a99aa99f5d45dc3d477cb Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 2 Dec 2016 17:18:46 -0800 Subject: [PATCH] Convert DeadlineTimer to chrono (RIPD-1189) --- Builds/VisualStudio2015/RippleD.vcxproj | 4 + .../VisualStudio2015/RippleD.vcxproj.filters | 3 + src/ripple/app/main/Application.cpp | 9 +- src/ripple/app/misc/NetworkOPs.cpp | 3 +- src/ripple/core/DeadlineTimer.h | 35 ++--- src/ripple/core/impl/DeadlineTimer.cpp | 82 ++++++------ src/test/core/DeadlineTimer_test.cpp | 123 ++++++++++++++++++ src/unity/core_test_unity.cpp | 1 + 8 files changed, 197 insertions(+), 63 deletions(-) create mode 100644 src/test/core/DeadlineTimer_test.cpp diff --git a/Builds/VisualStudio2015/RippleD.vcxproj b/Builds/VisualStudio2015/RippleD.vcxproj index 9015de5d81..145c44a454 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj +++ b/Builds/VisualStudio2015/RippleD.vcxproj @@ -4576,6 +4576,10 @@ True True + + True + True + True True diff --git a/Builds/VisualStudio2015/RippleD.vcxproj.filters b/Builds/VisualStudio2015/RippleD.vcxproj.filters index e8ff774f5a..c921aae40a 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj.filters +++ b/Builds/VisualStudio2015/RippleD.vcxproj.filters @@ -5313,6 +5313,9 @@ test\core + + test\core + test\core diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 7c43bdd1d8..0e68bac477 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -81,6 +81,7 @@ #include #include #include +#include #include namespace ripple { @@ -809,8 +810,9 @@ public: JLOG(m_journal.info()) << "Application starting. Version is " << BuildInfo::getVersionString(); - m_sweepTimer.setExpiration (10); - m_entropyTimer.setRecurringExpiration (300); + using namespace std::chrono_literals; + m_sweepTimer.setExpiration (10s); + m_entropyTimer.setRecurringExpiration (5min); m_io_latency_sampler.start(); @@ -910,7 +912,8 @@ public: cachedSLEs_.expire(); // VFALCO NOTE does the call to sweep() happen on another thread? - m_sweepTimer.setExpiration (config_->getSize (siSweepInterval)); + m_sweepTimer.setExpiration ( + std::chrono::seconds {config_->getSize (siSweepInterval)}); } diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 81a7c3c1f8..d5d218f03e 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -628,7 +628,8 @@ void NetworkOPsImp::setHeartbeatTimer () void NetworkOPsImp::setClusterTimer () { - m_clusterTimer.setExpiration (10.0); + using namespace std::chrono_literals; + m_clusterTimer.setExpiration (10s); } void NetworkOPsImp::onDeadlineTimer (DeadlineTimer& timer) diff --git a/src/ripple/core/DeadlineTimer.h b/src/ripple/core/DeadlineTimer.h index 01b9ae75d4..91ee7a3774 100644 --- a/src/ripple/core/DeadlineTimer.h +++ b/src/ripple/core/DeadlineTimer.h @@ -20,7 +20,6 @@ #ifndef RIPPLE_CORE_DEADLINETIMER_H_INCLUDED #define RIPPLE_CORE_DEADLINETIMER_H_INCLUDED -#include #include #include @@ -32,6 +31,10 @@ class DeadlineTimer : public beast::List ::Node { public: + using clock = std::chrono::steady_clock; + using duration = std::chrono::milliseconds; + using time_point = std::chrono::time_point; + /** Listener for a deadline timer. The listener is called on an auxiliary thread. It is suggested @@ -43,7 +46,7 @@ public: class Listener { public: - virtual void onDeadlineTimer (DeadlineTimer&) { } + virtual void onDeadlineTimer (DeadlineTimer&) = 0; }; public: @@ -67,30 +70,19 @@ public: If the timer is already active, this will reset it. @note If the timer is already active, the old one might go off before this function returns. - @param secondsUntilDeadline The number of seconds until the timer - will send a notification. This must be - greater than zero. + @param delay duration until the timer will send a notification. + This must be greater than zero. */ - /** @{ */ - void setExpiration (double secondsUntilDeadline); - - template - void setExpiration (std::chrono::duration const& amount) - { - setExpiration (std::chrono::duration_cast < - std::chrono::duration > (amount).count ()); - } - /** @} */ + void setExpiration (duration delay); /** Set the timer to go off repeatedly with the specified frequency. If the timer is already active, this will reset it. @note If the timer is already active, the old one might go off before this function returns. - @param secondsUntilDeadline The number of seconds until the timer - will send a notification. This must be - greater than zero. + @param interval duration until the timer will send a notification. + This must be greater than zero. */ - void setRecurringExpiration (double secondsUntilDeadline); + void setRecurringExpiration (duration interval); /** Equality comparison. Timers are equal if they have the same address. @@ -111,8 +103,9 @@ private: Listener* const m_listener; bool m_isActive; - beast::RelativeTime m_notificationTime; - double m_secondsRecurring; // non zero if recurring + + time_point notificationTime_; + duration recurring_; // > 0ms if recurring. }; } diff --git a/src/ripple/core/impl/DeadlineTimer.cpp b/src/ripple/core/impl/DeadlineTimer.cpp index 58117c56ff..3e34fa36b5 100644 --- a/src/ripple/core/impl/DeadlineTimer.cpp +++ b/src/ripple/core/impl/DeadlineTimer.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -59,11 +60,13 @@ public: // However, an extra notification may still happen due to concurrency. // void activate (DeadlineTimer& timer, - double secondsRecurring, beast::RelativeTime const& when) + duration recurring, + time_point when) { - assert (secondsRecurring >= 0); + using namespace std::chrono_literals; + assert (recurring >= 0ms); - std::lock_guard lock (m_mutex); + std::lock_guard lock {m_mutex}; if (timer.m_isActive) { @@ -72,8 +75,8 @@ public: timer.m_isActive = false; } - timer.m_secondsRecurring = secondsRecurring; - timer.m_notificationTime = when; + timer.recurring_ = recurring; + timer.notificationTime_ = when; insertSorted (timer); timer.m_isActive = true; @@ -86,7 +89,7 @@ public: // void deactivate (DeadlineTimer& timer) { - std::lock_guard lock (m_mutex); + std::lock_guard lock {m_mutex}; if (timer.m_isActive) { @@ -106,16 +109,16 @@ public: void runImpl () { + using namespace std::chrono; while (! threadShouldExit ()) { - beast::RelativeTime const currentTime ( - beast::RelativeTime::fromStartup ()); + auto const currentTime = time_point_cast(clock::now()); - double seconds (0); - DeadlineTimer* timer (nullptr); + auto nextDeadline = currentTime; + DeadlineTimer* timer {nullptr}; { - std::lock_guard lock (m_mutex); + std::lock_guard lock {m_mutex}; // See if a timer expired if (! m_items.empty ()) @@ -123,18 +126,18 @@ public: timer = &m_items.front (); // Has this timer expired? - if (timer->m_notificationTime <= currentTime) + if (timer->notificationTime_ <= currentTime) { // Expired, remove it from the list. assert (timer->m_isActive); m_items.pop_front (); // Is the timer recurring? - if (timer->m_secondsRecurring > 0) + if (timer->recurring_ > 0ms) { // Yes so set the timer again. - timer->m_notificationTime = - currentTime + timer->m_secondsRecurring; + timer->notificationTime_ = + currentTime + timer->recurring_; // Put it back into the list as active insertSorted (*timer); @@ -145,18 +148,18 @@ public: timer->m_isActive = false; } + // NOTE! Called *inside* lock! timer->m_listener->onDeadlineTimer (*timer); // re-loop - seconds = -1; + nextDeadline = currentTime - 1s; } else { - seconds = ( - timer->m_notificationTime - currentTime).inSeconds (); + nextDeadline = timer->notificationTime_; // Can't be zero and come into the else clause. - assert (seconds != 0); + assert (nextDeadline > currentTime); // Don't call the listener timer = nullptr; @@ -166,16 +169,19 @@ public: // Note that we have released the lock here. - if (seconds > 0) + if (nextDeadline > currentTime) { // Wait until interrupt or next timer. // - int const milliSeconds (std::max ( - static_cast (seconds * 1000 + 0.5), 1)); - assert (milliSeconds > 0); - wait (milliSeconds); + auto const waitCountMilliSeconds = nextDeadline - currentTime; + static_assert( + std::ratio_equal::value, + "Call to wait() requires units of milliseconds."); + + wait (static_cast(waitCountMilliSeconds.count())); } - else if (seconds == 0) + else if (nextDeadline == currentTime) { // Wait until interrupt // @@ -195,11 +201,11 @@ public: { if (! m_items.empty ()) { - Items::iterator before = m_items.begin (); + Items::iterator before {m_items.begin()}; for (;;) { - if (before->m_notificationTime >= timer.m_notificationTime) + if (before->notificationTime_ >= timer.notificationTime_) { m_items.insert (before, timer); break; @@ -243,24 +249,24 @@ void DeadlineTimer::cancel () Manager::instance().deactivate (*this); } -void DeadlineTimer::setExpiration (double secondsUntilDeadline) +void DeadlineTimer::setExpiration (std::chrono::milliseconds delay) { - assert (secondsUntilDeadline != 0); + using namespace std::chrono; + assert (delay > 0ms); - beast::RelativeTime const when ( - beast::RelativeTime::fromStartup() + secondsUntilDeadline); + auto const when = time_point_cast(clock::now() + delay); - Manager::instance().activate (*this, 0, when); + Manager::instance().activate (*this, 0ms, when); } -void DeadlineTimer::setRecurringExpiration (double secondsUntilDeadline) +void DeadlineTimer::setRecurringExpiration (std::chrono::milliseconds interval) { - assert (secondsUntilDeadline != 0); + using namespace std::chrono; + assert (interval > 0ms); - beast::RelativeTime const when ( - beast::RelativeTime::fromStartup() + secondsUntilDeadline); + auto const when = time_point_cast(clock::now() + interval); - Manager::instance().activate (*this, secondsUntilDeadline, when); + Manager::instance().activate (*this, interval, when); } -} // beast +} // ripple diff --git a/src/test/core/DeadlineTimer_test.cpp b/src/test/core/DeadlineTimer_test.cpp new file mode 100644 index 0000000000..05681bd6b6 --- /dev/null +++ b/src/test/core/DeadlineTimer_test.cpp @@ -0,0 +1,123 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2016 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include + +namespace ripple { + +//------------------------------------------------------------------------------ + +class DeadlineTimer_test : public beast::unit_test::suite +{ +public: + struct TestCallback : DeadlineTimer::Listener + { + TestCallback() = default; + + void onDeadlineTimer (DeadlineTimer&) override + { + ++count; + } + + std::atomic count {0}; + }; + + void testExpiration() + { + using clock = DeadlineTimer::clock; + + using namespace std::chrono_literals; + using namespace std::this_thread; + + TestCallback cb; + DeadlineTimer dt {&cb}; + + // There are parts of this test that are somewhat race conditional. + // The test is designed to avoid spurious failures, rather than + // fail occasionally but randomly, where ever possible. So there may + // be occasional gratuitous passes. Unfortunately, since it is a + // time-based test, there may also be occasional spurious failures + // on low-powered continuous integration platforms. + { + testcase("Expiration"); + + // Set a deadline timer that should only fire once in 5ms. + cb.count = 0; + auto const startTime = clock::now(); + dt.setExpiration (5ms); + + // Make sure the timer didn't fire immediately. + int const count = cb.count.load(); + if (clock::now() < startTime + 4ms) + { + BEAST_EXPECT (count == 0); + } + + // Wait until the timer should have fired and check that it did. + // In fact, we wait long enough that if it were to fire multiple + // times we'd see that. + sleep_until (startTime + 50ms); + BEAST_EXPECT (cb.count.load() == 1); + } + { + testcase("RecurringExpiration"); + + // Set a deadline timer that should fire once every 5ms. + cb.count = 0; + auto const startTime = clock::now(); + dt.setRecurringExpiration (5ms); + + // Make sure the timer didn't fire immediately. + { + int const count = cb.count.load(); + if (clock::now() < startTime + 4ms) + { + BEAST_EXPECT (count == 0); + } + } + + // Wait until the timer should have fired several times and + // check that it did. + sleep_until (startTime + 100ms); + { + auto const count = cb.count.load(); + BEAST_EXPECT ((count > 1) && (count < 21)); + } + + // Cancel the recurring timer and it should not fire any more. + dt.cancel(); + auto const count = cb.count.load(); + sleep_for (50ms); + BEAST_EXPECT (count == cb.count.load()); + } + } + + void run() + { + testExpiration(); + } +}; + +BEAST_DEFINE_TESTSUITE(DeadlineTimer, core, ripple); + +} \ No newline at end of file diff --git a/src/unity/core_test_unity.cpp b/src/unity/core_test_unity.cpp index 2873bf0f3e..468e614503 100644 --- a/src/unity/core_test_unity.cpp +++ b/src/unity/core_test_unity.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include \ No newline at end of file