Merge lp:~vanvugt/compiz-core/bettertimers into lp:compiz-core/0.9.5

[Daniel van Vugt]

Add support for CompTimers with a minTime of 0.

Previously, calling setTimes(0,0) would cause an infinite loop in
CompTimeoutSource::callback and/or non-zero timers to never be called.
This new code avoids infinite loops, avoids non-zero timer starvation and
improves timer accuracy in general.

[lp:~vanvugt/compiz-core/bettertimers]

2895. By Daniel van Vugt on 2011-11-21

Fixed implicit parameter handling in setExpiryTimes.

[Marco Trevisan (Treviño) (3v1n0)]

Shouldn't you include the changes for this:

+ /*
+ * Note the use of delay = 1 instead of 0, even though 0 would be better.
+ * A delay of zero is presently broken due to CompTimer bugs;
+ * 1. Infinite loop in CompTimeoutSource::callback when a zero
+ * timer is set.
+ * 2. Priority set too high in CompTimeoutSource::CompTimeoutSource
+ * causing the glib main event loop to be starved of X events.
+ * Fixes for both of these issues are being worked on separately.
+ */
+ paintTimer.start
+ (boost::bind (&CompositeScreen::handlePaintTimeout, cScreen),
+ delay);

And setting lp:~vanvugt/compiz-core/fix-880707.2 as a prerequisite of this branch?

[Daniel van Vugt (vanvugt)]

No, that comment is misleading as mentioned in the other merge proposal. Changing the delay to 0 instead of 1 actually does not seem improve performance. So no need for a prereq.

We really do not need the complexity of mashing two large proposals together when they can be safely dealt with separately.

[Daniel van Vugt (vanvugt)]

And I did experiment with putting the 2 fixes together. They did not affect each other in any way.

[Sam Spilsbury (smspillaz)]

156 + gint64 now = g_get_monotonic_time ();
157 + priv->mMinDeadline = now + ((gint64)min * 1000);
158 + priv->mMaxDeadline = now + ((gint64)(max >= min ? max : min) * 1000);

Glib::Source::get_time wraps g_get_monotonic_time () - you can use get_time () here.

Also, put spaces betwene casts and variables.

+#include <ctime>
199 +
200 +static time_t starttime = 0;

No need to have a global variable there - it can go inside of the testing base class, and get_time () can probably be re-used here. I can fix these when I merge it.

review: Approve

[Daniel van Vugt (vanvugt)]

I used g_get_monotonic_time because get_time is not available outside of Source classes. Is it??
And if you have to use g_get_monotonic_time in at least one place then it should be used everywhere so all locations/classes agree on the current time.

The global variable was just to avoid changing headers and increasing the size of the diff :)

=== modified file 'src/privatetimeoutsource.h'
--- src/privatetimeoutsource.h	2011-07-04 16:25:20 +0000
+++ src/privatetimeoutsource.h	2011-11-21 02:45:27 +0000
@@ -47,10 +47,6 @@
 	explicit CompTimeoutSource (Glib::RefPtr <Glib::MainContext> &ctx);
 	virtual ~CompTimeoutSource ();
 
-    private:
-
-	gint64 mLastTime;
-
     friend class CompTimer;
 };
 

=== modified file 'src/privatetimer.h'
--- src/privatetimer.h	2011-07-05 19:15:16 +0000
+++ src/privatetimer.h	2011-11-21 02:45:27 +0000
@@ -38,8 +38,8 @@
 	bool         		 mActive;
 	unsigned int 		 mMinTime;
 	unsigned int 		 mMaxTime;
-	int          		 mMinLeft;
-	int          		 mMaxLeft;
+	gint64                   mMinDeadline;
+	gint64                   mMaxDeadline;
 	CompTimer::CallBack      mCallBack;
 };
 

=== modified file 'src/timer.cpp'
--- src/timer.cpp	2011-07-05 19:15:24 +0000
+++ src/timer.cpp	2011-11-21 02:45:27 +0000
@@ -38,8 +38,6 @@
     set_priority (G_PRIORITY_HIGH);
     attach (ctx);
 
-    mLastTime = get_time ();
-    
     connect (sigc::mem_fun <bool, CompTimeoutSource> (this, &CompTimeoutSource::callback));
 }
 
@@ -95,51 +93,20 @@
 		timeout = t->maxLeft ();
 	    it++;
 	}
-
-	mLastTime = get_time ();
-
-	return false;
     }
     else
     {
 	timeout = 0;
-
-	mLastTime = get_time ();
-
-	return true;
     }
+
+    return timeout <= 0;
 }
 
 bool
 CompTimeoutSource::check ()
 {
-    gint64		    fixedTimeDiff;
-    gdouble		    timeDiff;
-    gint64		    currentTime = get_time ();
-    bool		    ready = false;
-
-    timeDiff = (currentTime - mLastTime) / 1000.0;
-
-    mLastTime = currentTime;
-
-    /* prefer over-estimating rather than waking up */
-    fixedTimeDiff = ceil (timeDiff);
-
-    /* Handle clock rollback */
-    if (fixedTimeDiff < 0)
-    {
-	fixedTimeDiff = 0;
-    }
-
-    foreach (CompTimer *t, TimeoutHandler::Default ()->timers ())
-    {
-	t->decrement (fixedTimeDiff);
-    }
-
-    ready = (!TimeoutHandler::Default ()->timers ().empty () &&
+    return (!TimeoutHandler::Default ()->timers ().empty () &&
 	     TimeoutHandler::Default ()->timers ().front ()->minLeft () <= 0);
-
-    return ready;
 }
 
 bool
@@ -152,29 +119,37 @@
 bool
 CompTimeoutSource::callback ()
 {
-    while (TimeoutHandler::Default ()->timers ().begin () != TimeoutHandler::Default ()->timers ().end () &&
-	   TimeoutHandler::Default ()->timers ().front ()->minLeft () <= 0)
+    TimeoutHandler *handler = TimeoutHandler::Default ();
+    std::list<CompTimer*> requeue, &timers = handler->timers ();
+
+    while (!timers.empty ())
     {
-	CompTimer *t = TimeoutHandler::Default ()->timers ().front ();
-	TimeoutHandler::Default ()->timers ().pop_front ();
-
+	CompTimer *t = timers.front ();
+	if (t->minLeft () > 0)
+	    break;
+	timers.pop_front ();
 	t->setActive (false);
 	if (t->triggerCallback ())
-	{
-	    TimeoutHandler::Default ()->addTimer (t);
-	    t->setActive (true);
-	}
-    }
-
-    return !TimeoutHandler::Default ()->timers ().empty ();
+	    requeue.push_back (t);
+    }
+
+    std::list<CompTimer*>::const_iterator i = requeue.begin ();
+    for (; i != requeue.end (); i++)
+    {
+	CompTimer *t = *i;
+	handler->addTimer (t);
+	t->setActive (true);
+    }
+
+    return !timers.empty ();
 }
 
 PrivateTimer::PrivateTimer () :
     mActive (false),
     mMinTime (0),
     mMaxTime (0),
-    mMinLeft (0),
-    mMaxLeft (0),
+    mMinDeadline (0),
+    mMaxDeadline (0),
     mCallBack (NULL)
 {
 }
@@ -211,15 +186,15 @@
 void
 CompTimer::setExpiryTimes (unsigned int min, unsigned int max)
 {
-    priv->mMinLeft = min;
-    priv->mMaxLeft = (min <= max) ? max : min;
+    gint64 now = g_get_monotonic_time ();
+    priv->mMinDeadline = now + ((gint64)min * 1000);
+    priv->mMaxDeadline = now + ((gint64)(max >= min ? max : min) * 1000);
 }
 
 void
 CompTimer::decrement (unsigned int diff)
 {
-    priv->mMinLeft -= diff;
-    priv->mMaxLeft -= diff;
+    /* deprecated */
 }
 
 void
@@ -305,13 +280,17 @@
 unsigned int
 CompTimer::minLeft ()
 {
-    return (priv->mMinLeft < 0)? 0 : priv->mMinLeft;
+    gint64 now = g_get_monotonic_time ();
+    return (priv->mMinDeadline > now) ?
+	(unsigned int)(priv->mMinDeadline - now + 500) / 1000 : 0;
 }
 
 unsigned int
 CompTimer::maxLeft ()
 {
-    return (priv->mMaxLeft < 0)? 0 : priv->mMaxLeft;
+    gint64 now = g_get_monotonic_time ();
+    return (priv->mMaxDeadline > now) ?
+	(unsigned int)(priv->mMaxDeadline - now + 500) / 1000 : 0;
 }
 
 bool

=== modified file 'tests/timer/callbacks/test-timer-callbacks.cpp'
--- tests/timer/callbacks/test-timer-callbacks.cpp	2011-07-06 18:58:47 +0000
+++ tests/timer/callbacks/test-timer-callbacks.cpp	2011-11-21 02:45:27 +0000
@@ -24,42 +24,85 @@
  */
 
 #include "test-timer.h"
+#include <ctime>
+
+static time_t starttime = 0;
 
 bool
 CompTimerTestCallbacks::cb (int timernum)
 {
-    if (lastTimerTriggered == 0 && timernum == 1)
-    {
-	std::cout << "FAIL: timer with a higher timeout value triggered before the timer with the lower timeout" << std::endl;
-	exit (1);
-	return false;
-    }
-    else if (lastTimerTriggered == 2 && timernum != 1)
-    {
-	std::cout << "FAIL: the second timer should have triggered" << std::endl;
-	exit (1);
-	return false;
-    }
-
-    std::cout << "INFO: triggering timer " << timernum << std::endl;
+    static bool complete = false;
+    static int count[4] = {0, 0, 0, 0};
+
+    if (timernum < 4)
+	count[timernum]++;
+
+    if (complete)
+	return false;
+
+    if (time (NULL) - starttime > 5)  /* Wait no more than 5 seconds */
+    {
+	std::cout << "FAIL: some timers are never being triggered" << std::endl;
+	exit (1);
+	return false;
+    }
+    else if (lastTimerTriggered == 0 && timernum != 3)
+    {
+	std::cout << "FAIL: timer 3 was not triggered first" << std::endl;
+	exit (1);
+	return false;
+    }
+    else if (lastTimerTriggered == 2 && timernum != 3)
+    {
+	std::cout << "FAIL: timer 3 was not after 2" << std::endl;
+	exit (1);
+	return false;
+    }
+    else if (timernum == 1 && count[2] < 1)
+    {
+	std::cout << "FAIL: timer 1 was not preceded by 2" << std::endl;
+	exit (1);
+	return false;
+    }
+
+    if (timernum < 3 || lastTimerTriggered != 3)
+    {
+	std::cout
+	    << "INFO: triggering timer "
+	    << timernum
+	    << ((timernum == 3) ? " (many times)" : "")
+	    << std::endl;
+    }
 
     lastTimerTriggered = timernum;
 
     if (timernum == 1)
     {
-	std::cout << "PASS: basic timers" << std::endl;
+	complete = true;
+	std::cout
+	    << "PASS: basic timers. Total calls: "
+	    << count[1]
+	    << ", "
+	    << count[2]
+	    << ", "
+	    << count[3]
+	    << std::endl;
 	ml->quit ();
     }
 
-    return false;
+    return !complete;
 }
 
 void
 CompTimerTestCallbacks::precallback ()
 {
+    starttime = time (NULL);
+
     /* Test 2: Adding timers */
     std::cout << "-= TEST: adding timers and callbacks" << std::endl;
-    timers.push_back (new CompTimer ());
+
+    CompTimer *t1 = new CompTimer ();
+    timers.push_back (t1);
     timers.front ()->setTimes (100, 110);
     timers.front ()->setCallback (boost::bind (&CompTimerTestCallbacks::cb, this, 1));
 
@@ -70,13 +113,20 @@
 	exit (1);
     }
 
-    timers.push_back (new CompTimer ());
+    CompTimer *t2 = new CompTimer ();
+    timers.push_back (t2);
     timers.back ()->setTimes (50, 90);
     timers.back ()->setCallback (boost::bind (&CompTimerTestCallbacks::cb, this, 2));
 
-    /* Start both timers */
-    timers.front ()->start ();
-    timers.back ()->start ();
+    CompTimer *t3 = new CompTimer ();
+    timers.push_back (t3);
+    timers.back ()->setTimes (0, 0);
+    timers.back ()->setCallback (boost::bind (&CompTimerTestCallbacks::cb, this, 3));
+
+    /* Start all timers */
+    t1->start ();
+    t2->start ();
+    t3->start ();
 
     /* TimeoutHandler::timers should have the timer that
      * is going to trigger first at the front of the