ChangeLogTag: Thu Feb 25 22:07:25 UTC 2010 Chris Cleeland <cleeland@ociweb.com>\nBug 3104

6 files changed, 147 insertions, 12 deletions

diff --git a/ACE/ChangeLog b/ACE/ChangeLog
index 2a6d312fd7c..bf1f26996c7 100644
--- a/ACE/ChangeLog
+++ b/ACE/ChangeLog
@@ -1,3 +1,32 @@
+Thu Feb 25 22:07:25 UTC 2010  Chris Cleeland  <cleeland@ociweb.com>
+
+        * tests/Timer_Queue_Test.cpp (test_interval_timer):
+
+          Added new function to test interval timing functionality in
+          preparation for merging in a fix for bug 3104.  Note that while
+          bug 3104 focuses on performance issues, this test doesn't
+          specifically test performance, but rather tests that the
+          interval timer feature works properly so that we know that
+          any performance-related changes made in fixing bug 3104 don't
+          cause a regression.
+
+        * ace/Timer_Queue_T.h:
+        * ace/Timer_Queue_T.cpp:
+
+          Integrate code proposed to fix bug 3104, but factored out into
+          a new method, recompute_next_abs_interval_time, so that the
+          more efficient computation can be re-used in Timer_Hash and
+          Timer_Wheel.
+
+        * ace/Timer_Queue_T.cpp (dispatch_info_i):
+
+          Change to use recompute_next_abs_interval_time.
+
+        * ace/Timer_Hash_T.cpp (expire):
+        * ace/Timer_Wheel_T.cpp (expire):
+
+          Change to use recompute_next_abs_interval_time.
+
 Thu Feb 25 20:33:27 UTC 2010  Steve Huston  <shuston@riverace.com>
 
         * tests/Bug_2609_Regression_Test.cpp: Correct to use documented ACE
diff --git a/ACE/ace/Timer_Hash_T.cpp b/ACE/ace/Timer_Hash_T.cpp
index a2a81fc3773..86dec8fe378 100644
--- a/ACE/ace/Timer_Hash_T.cpp
+++ b/ACE/ace/Timer_Hash_T.cpp
@@ -823,10 +823,7 @@ ACE_Timer_Hash_T<TYPE, FUNCTOR, ACE_LOCK, BUCKET>::expire (const ACE_Time_Value
             {
               // Make sure that we skip past values that have already
               // "expired".
-              do
-                expired->set_timer_value (expired->get_timer_value ()
-                                          + expired->get_interval ());
-              while (expired->get_timer_value () <= cur_time);
+              this->recompute_next_abs_interval_time (expired, cur_time);
 
               // Since this is an interval timer, we need to
               // reschedule it.
diff --git a/ACE/ace/Timer_Queue_T.cpp b/ACE/ace/Timer_Queue_T.cpp
index 2ac9a29e9aa..23d2628970d 100644
--- a/ACE/ace/Timer_Queue_T.cpp
+++ b/ACE/ace/Timer_Queue_T.cpp
@@ -283,6 +283,66 @@ ACE_Timer_Queue_T<TYPE, FUNCTOR, ACE_LOCK>::expire (const ACE_Time_Value &cur_ti
   return number_of_timers_expired;
 }
 
+template <class TYPE, class FUNCTOR, class ACE_LOCK> void
+ACE_Timer_Queue_T<TYPE, FUNCTOR, ACE_LOCK>::recompute_next_abs_interval_time
+    (ACE_Timer_Node_T<TYPE> *expired,
+     const ACE_Time_Value &cur_time)
+{
+  if ( expired->get_timer_value () <= cur_time )
+    {
+      /*
+       * Somehow the current time is past when this time was
+       * supposed to expire (e.g., timer took too long,
+       * somebody changed system time, etc.).  There used to
+       * be a simple loop here that skipped ahead one timer
+       * interval at a time, but that was horribly inefficient
+       * (an O(n) algorithm) when the timer duration was small
+       * relative to the amount of time skipped.
+       *
+       * So, we replace the loop with a simple computation,
+       * which also happens to be O(1).  All times get
+       * normalized in the computation to microseconds.
+       *
+       * For reference, the loop looked like this:
+       *
+       *   do
+       *     expired->set_timer_value (expired->get_timer_value () +
+       *                               expired->get_interval ());
+       *   while (expired->get_timer_value () <= cur_time);
+       *
+       */
+
+      // Compute the duration of the timer's interval
+      ACE_UINT64 interval_usec;
+      expired->get_interval ().to_usec (interval_usec);
+
+      // Compute the span between the current time and when
+      // the timer would have expired in the past (and
+      // normalize to microseconds).
+      ACE_Time_Value old_diff = cur_time - expired->get_timer_value ();
+      ACE_UINT64 old_diff_usec;
+      old_diff.to_usec (old_diff_usec);
+
+      // Compute the delta time in the future when the timer
+      // should fire as if it had advanced incrementally.  The
+      // modulo arithmetic accomodates the likely case that
+      // the current time doesn't fall precisely on a timer
+      // firing interval.
+      ACE_UINT64 new_timer_usec =
+        interval_usec - (old_diff_usec % interval_usec);
+
+      // Compute the absolute time in the future when this
+      // interval timer should expire.
+      ACE_Time_Value new_timer_value
+        (cur_time.sec ()
+         + static_cast<time_t>(new_timer_usec / ACE_ONE_SECOND_IN_USECS),
+         cur_time.usec ()
+         + static_cast<suseconds_t>(new_timer_usec % ACE_ONE_SECOND_IN_USECS));
+
+      expired->set_timer_value (new_timer_value);
+    }
+}
+
 template <class TYPE, class FUNCTOR, class ACE_LOCK> int
 ACE_Timer_Queue_T<TYPE, FUNCTOR, ACE_LOCK>::dispatch_info_i (const ACE_Time_Value &cur_time,
                                                              ACE_Timer_Node_Dispatch_Info_T<TYPE> &info)
@@ -306,10 +366,7 @@ ACE_Timer_Queue_T<TYPE, FUNCTOR, ACE_LOCK>::dispatch_info_i (const ACE_Time_Valu
         {
           // Make sure that we skip past values that have already
           // "expired".
-          do
-            expired->set_timer_value (expired->get_timer_value () +
-                                      expired->get_interval ());
-          while (expired->get_timer_value () <= cur_time);
+          this->recompute_next_abs_interval_time (expired, cur_time);
 
           // Since this is an interval timer, we need to reschedule
           // it.
diff --git a/ACE/ace/Timer_Queue_T.h b/ACE/ace/Timer_Queue_T.h
index 8aef637579e..4ed6fd4a30f 100644
--- a/ACE/ace/Timer_Queue_T.h
+++ b/ACE/ace/Timer_Queue_T.h
@@ -431,6 +431,10 @@ protected:
   virtual int dispatch_info_i (const ACE_Time_Value &current_time,
                                ACE_Timer_Node_Dispatch_Info_T<TYPE> &info);
 
+  /// Recompute when the next time is that this interval timer should fire.
+  void recompute_next_abs_interval_time (ACE_Timer_Node_T<TYPE>* expired,
+                                         const ACE_Time_Value &cur_time);
+
   /// Synchronization variable for ACE_Timer_Queue.
   /// @note The right name would be lock_, but HP/C++ will choke on that!
   ACE_LOCK mutex_;
diff --git a/ACE/ace/Timer_Wheel_T.cpp b/ACE/ace/Timer_Wheel_T.cpp
index f306905365c..d7734e6b120 100644
--- a/ACE/ace/Timer_Wheel_T.cpp
+++ b/ACE/ace/Timer_Wheel_T.cpp
@@ -834,10 +834,7 @@ ACE_Timer_Wheel_T<TYPE, FUNCTOR, ACE_LOCK>::expire (const ACE_Time_Value& cur_ti
         {
           // Make sure that we skip past values that have already
           // "expired".
-          do
-            n->set_timer_value (n->get_timer_value () +
-                                n->get_interval ());
-          while (n->get_timer_value () <= cur_time);
+          this->recompute_next_abs_interval_time (n, cur_time);
 
           this->reschedule (n);
         }
diff --git a/ACE/tests/Timer_Queue_Test.cpp b/ACE/tests/Timer_Queue_Test.cpp
index f6a76ee3465..0ea1eb23fa4 100644
--- a/ACE/tests/Timer_Queue_Test.cpp
+++ b/ACE/tests/Timer_Queue_Test.cpp
@@ -87,6 +87,56 @@ public:
   // Keeps track of the number of times that <handle_close> is called.
 };
 
+
+struct Interval_Handler : public ACE_Event_Handler
+{
+  Interval_Handler (void) : trip_count_ (0) { }
+
+  virtual int handle_timeout (const ACE_Time_Value & , const void *arg)
+  {
+    ++trip_count_;
+    return 0;
+  }
+
+  unsigned trip_count_; // number of times handle_timeout has been tripped.
+};
+
+static void
+test_interval_timer (ACE_Timer_Queue *tq)
+{
+  /*
+    The strategy:
+
+    Set up a timer to fire on a 50ms interval.
+  */
+  Interval_Handler ih;
+  ACE_Time_Value interval (0, 50 * 1000 /* number of usec in millisecond */);
+  const unsigned NUM_INTERVAL_FIRINGS = 50;
+  ACE_Time_Value loop_stop_time =
+    tq->gettimeofday () + (NUM_INTERVAL_FIRINGS * interval);
+  const unsigned EXPECTED_TRIP_COUNT =
+    NUM_INTERVAL_FIRINGS + 1 /* for the first immediate firing */;
+
+  long id = tq->schedule (&ih, 0 /* no act */, ACE_Time_Value::zero, interval);
+  ACE_ASSERT (id != -1);
+
+  do
+    {
+      tq->expire ();
+    }
+  while (tq->gettimeofday () < loop_stop_time);
+
+  ACE_DEBUG((LM_DEBUG,
+             ACE_TEXT("after interval loop, timer fired %d ")
+             ACE_TEXT("times out of %d expected: %s\n"),
+             ih.trip_count_, EXPECTED_TRIP_COUNT,
+             ih.trip_count_ == EXPECTED_TRIP_COUNT 
+             ? ACE_TEXT ("success") : ACE_TEXT ("FAIL")
+             ));
+
+  tq->cancel (id);
+}
+
 static void
 test_functionality (ACE_Timer_Queue *tq)
 {
@@ -646,6 +696,7 @@ run_main (int argc, ACE_TCHAR *argv[])
       ACE_DEBUG ((LM_DEBUG,
                   ACE_TEXT ("**** starting test of %s\n"),
                   tq_ptr->name_));
+      test_interval_timer (tq_ptr->queue_);
       test_functionality (tq_ptr->queue_);
       test_performance (tq_ptr->queue_,
                         tq_ptr->name_);