Merge lp:~vanvugt/unity/fix-917210-trunk into lp:unity

[Daniel van Vugt]

Fix the autoscroll timer waking up 50 times per sec when no scrolling needs to be done (LP: #917210)

Fix the autoscroll timer waking up 50 times per sec when no scrolling needs to be done (LP: #917210)

Also removed some unused variables.

[Tim Penhey (thumper)]

Hi Daniel,

How do we test that this has been fixed?

review: Needs Information

[Daniel van Vugt (vanvugt)]

Sorry for the delay, it's a public holiday...

To test the fix use "powertop" or "eventstat" to display the number of kernel wakeups per second each process is causing. As discussed in bug 917210. Though, you will also need the compiz fix installed (fix-880707.2 available in ppa:vanvugt/compiz) to avoid the constant wakeups caused by the old composite code.

A well-behaving "idle" compiz shouldn't be waking up more than about 10 times per second, maximum. Ideally less.

[Daniel van Vugt (vanvugt)]

You may also need to hover the mouse over either end of the launcher to make the autoscroll timer start. Then when you move the mouse off the launcher, it keeps spinning. That's the bug.

[Tim Penhey (thumper)]

Hi Daniel,

I've been trying to test this. I installed powertop and made my launcher icons large so they fold.

On an idle system, I have between 15 and 20 events / second from compiz.

Holding the mouse pointer over the launcher at the bottom makes compiz go from 25 to 38 events / second.

However leaving it and going idle again, it drops back to 15 to 20 events / second.

Do you have the launcher always out?

[Daniel van Vugt (vanvugt)]

Yes, my launcher is always visible. Maybe that's the difference?

[Tim Penhey (thumper)]

On Tue 14 Feb 2012 16:11:19 NZDT, Daniel van Vugt wrote:
> Yes, my launcher is always visible. Maybe that's the difference?
>

I'll try that

[Daniel van Vugt (vanvugt)]

If it looks too much, I remember there was an ultra-simplified version of this fix that was possible. However the simplified version could not detect when the mouse "happens to be left idle over one of the autoscroll hotspots on the launcher".

[Tim Penhey (thumper)]

On Tue 14 Feb 2012 16:21:18 NZDT, Daniel van Vugt wrote:
> If it looks too much, I remember there was an ultra-simplified version of this fix that was possible. However the simplified version could not detect when the mouse "happens to be left idle over one of the autoscroll hotspots on the launcher".

Can you still reproduce this behaviour with trunk? I'm compiling and
testing here.

[Daniel van Vugt (vanvugt)]

Sorry, that was developed on oneiric and then ported to trunk by hand. I haven't tested unity trunk yet.

If you can't reproduce the problem then maybe demote the bug to Medium and defer it till I (or someone) can revisit it?

[Tim Penhey (thumper)]

It seems that this has since been fixed.

I can't get compiz to keep the high number of wakeups after moving the mouse off the launcher.

Do you think we should say the bug is fixed? or just demote and comment?

[Daniel van Vugt (vanvugt)]

I think just demote and comment. Leave it up to Colin King or myself to retest later.

The primary problem when the mouse is not over the launcher may have been fixed already. Thought given the original issue was obvious upon simple code review I can't imagine how it might have been fixed if there are no conflicts caused by this branch.

Also, your comment "Holding the mouse pointer over the launcher at the bottom makes compiz go from 25 to 38 events / second." shows that you are still able to reproduce part of the bug.

[Tim Penhey (thumper)]

So this reduces the number of wake-ups only while the cursor is in the auto-scroll position?

[Daniel van Vugt (vanvugt)]

No, it definitely does both.

From memory my test scenario was:
1. Have the launcher always visible.
2. Hover over the top or bottom of the launcher to start the timer.
3. Move the pointer back to the regular desktop.
4. The timer keeps spinning at full speed even though I'm not hovering over any part of the launcher any more.

The primary cause of unwanted wakeups was the timer callback returning TRUE when it should have been FALSE (to cancel the timer). This appears to be a very simple and obvious logic error. The important changes are lines 49-->52 and 62-->75. The important parts are to return FALSE (and set _autoscroll_handle = 0) to stop the timer as soon as it is no longer required.

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

The logic I see here seems right. There's no reason to keep the timer running, if it's not needed.

[Tim Penhey (thumper)]

My concern here now is that the logic has changed:

Before:
  if (self->_keynav_activated || self->_key_switcher_activated || !self->_hovered ||
      self->GetActionState() == ACTION_DRAG_LAUNCHER)
    return TRUE

Now:
  all the above, + self->_scroll_limit_reached
  -> effectively return FALSE;

This feels wrong. If I'm wrong, can you explain it?

[Daniel van Vugt (vanvugt)]

Tim, you have spotted the primary cause of bug 917210. It should return FALSE there to cancel the timer. As I mentioned in my previous comment.

[Tim Penhey (thumper)]

OK, I understand now.

I'd like to make sure we have some test coverage of it though.

[Daniel van Vugt (vanvugt)]

Fixed conflicts. Added test case. Re-tested against unity_5.6.0-0ubuntu3. Works!

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

Looks good to me.

Would be possible to add some introspection data and test with Autopilot when the timeout is running and when not?
(to check that is there only when it should).

review: Needs Information

[Alex Launi (alexlauni)]

Could you investigate automating this? It should be very simple once you figure out how to get wake ups.

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

Let's approve it for now... We'll automate the test ASAP. With didrock's approval ;)

review: Approve

=== added file 'manual-tests/Wakeups.txt'
--- manual-tests/Wakeups.txt	1970-01-01 00:00:00 +0000
+++ manual-tests/Wakeups.txt	2012-03-14 07:38:39 +0000
@@ -0,0 +1,23 @@
+Launcher autoscroll wake-ups
+----------------------------
+This test makes sure that the timer controlling autoscroll animations does
+not keep running (at 50Hz) when it's not required to animate anything. This
+is LP: #917210.
+
+#. Ensure the screen is idle, with very little changing.
+#. Open a terminal and start a tool that can measure process wakeups per
+   second. You can choose one of:
+     - gnome-power-statistics (click on Processor)
+     - powertop
+     - eventstat
+#. Hover the mouse pointer over the top end of the Launcher. That's just above
+   the Ubuntu icon.
+#. Hover the mouse pointer over the bottom end of the Launcher. That's over
+   the bottom of the trash icon.
+#. Move the mouse pointer back to the centre of the screen and leave it there.
+#. Wait 30-60 seconds, leaving the screen idle before verifying the Outcome.
+
+Outcome
+  The number of wakeups per second (events per second) reported for the
+  compiz process should be much lower than 50.
+

=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2012-03-13 17:20:30 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-03-14 07:38:39 +0000
@@ -201,8 +201,7 @@
   _folded_angle           = 1.0f;
   _neg_folded_angle       = -1.0f;
   _space_between_icons    = 5;
-  _launcher_top_y         = 0;
-  _launcher_bottom_y      = 0;
+  _last_delta_y           = 0.0f;
   _folded_z_distance      = 10.0f;
   _launcher_action_state  = ACTION_NONE;
   _icon_under_mouse       = NULL;
@@ -227,6 +226,7 @@
   _shortcuts_shown        = false;
   _hovered                = false;
   _hidden                 = false;
+  _scroll_limit_reached   = false;
   _render_drag_window     = false;
   _drag_edge_touching     = false;
   _steal_drag             = false;
@@ -1203,6 +1203,9 @@
     delta_y *= hover_progress;
     center.y += delta_y;
     folding_threshold += delta_y;
+
+    _scroll_limit_reached = (delta_y == _last_delta_y);
+    _last_delta_y = delta_y;
   }
   else
   {
@@ -1669,12 +1672,18 @@
 {
   Launcher* self = (Launcher*) data;
   nux::Geometry geo = self->GetGeometry();
+  gboolean anim = TRUE;
 
+  //
+  // Always check _scroll_limit_reached to ensure we don't keep spinning
+  // this timer if the mouse happens to be left idle over one of the autoscroll
+  // hotspots on the launcher.
+  //
   if (self->IsInKeyNavMode() || !self->_hovered ||
+      self->_scroll_limit_reached ||
       self->GetActionState() == ACTION_DRAG_LAUNCHER)
-    return TRUE;
-
-  if (self->MouseOverTopScrollArea())
+    anim = FALSE;
+  else if (self->MouseOverTopScrollArea())
   {
     if (self->MouseOverTopScrollExtrema())
       self->_launcher_drag_delta += 6;
@@ -1688,9 +1697,18 @@
     else
       self->_launcher_drag_delta -= 3;
   }
-
-  self->EnsureAnimation();
-  return TRUE;
+  else
+    anim = FALSE;
+
+  if (anim)
+    self->EnsureAnimation();
+  else
+  {
+    self->_autoscroll_handle = 0;
+    self->_scroll_limit_reached = false;
+  }
+
+  return anim;
 }
 
 void Launcher::EnsureScrollTimer()

=== modified file 'plugins/unityshell/src/Launcher.h'
--- plugins/unityshell/src/Launcher.h	2012-03-13 02:07:54 +0000
+++ plugins/unityshell/src/Launcher.h	2012-03-14 07:38:39 +0000
@@ -315,6 +315,7 @@
 
   bool  _hovered;
   bool  _hidden;
+  bool  _scroll_limit_reached;
   bool  _render_drag_window;
 
   bool          _shortcuts_shown;
@@ -324,8 +325,7 @@
   float _folded_angle;
   float _neg_folded_angle;
   float _folded_z_distance;
-  float _launcher_top_y;
-  float _launcher_bottom_y;
+  float _last_delta_y;
   float _edge_overcome_pressure;
 
   LauncherActionState _launcher_action_state;