Merge lp:~vanvugt/compiz-core/fix-764330-trunk into lp:compiz-core/0.9.5

[Daniel van Vugt]

Fix severe lag and "freezing" when dragging windows (LP: #764330)

This fixes the two root causes:
 1. Too many input events for slow systems to keep up with. Often compiz
    receives 1000 MotionNotify's per second. This is many more than is
    perceptible on screen. So the new default is to sample the mouse position
    a maximum of 200 times per second (also configurable).
 2. Lazy Positioning was sometimes not being used even when it's enabled.
    This caused long and slow sync calls, and amplified the flooded event
    queue problem above.

[Sam Spilsbury (smspillaz)]

Hey Daniel,

I fixed this bug properly in precise already, have a look at https://code.launchpad.net/~smspillaz/compiz-core/fix-893467/+merge/84054

The reason why window movement sometimes becomes choppy in oneiric is because core didn't differentiate between synchronous and asynchronous position updates. Because of that, the following race condition could occurr:

1 => 2 => 3 => 4 => server
.... 1 => 2 => 3 => 4=> client

What the move plugin was doing was this:

1 => 2 => 3 => 4 server
1 => 2 => 3 => 4 client

At which point, the 1, 2, 3 that we sent to the server are now redundant because we already updated it on the client side. However, because clients can move themselves if they are unmanaged, that 1 => 2 => 3 =>4 will end up calling moveNotify and updating w->geometry () again. So you'll get

1 => 2 => 3 => 4 => 1 => 2 => 3 => 4 client

That generally causes chaos, and caused problems with window stacking because I meant that I couldn't track pending requests correctly, which was necessary in order to fix an edge case where a window had been restacked, and any restack relative to it would be a no-op (in error).

The move plugin immediately updates the client side position in order to create a perceived immediacy to movement because we're not waiting on the server to deliver the new position to us.

In oneiric at least, the only way I could reconcile these two differences was to disallow synchronous updates when it is known that there are pending configure notifies on that window. If there happens to be a condition where we don't match a ConfigureNotify event to an XConfigureWindow request, that event queue becomes stale and needs to be unblocked after a few seconds. (obviously that's a hack, but given time constraints at the time, it was the only way it could feasibly be worked around).

The correct fix that was implemented in the branch I mentioned earlier was to separate the concept of the paint position and the real window position with an API break. Now the window position can be immediately updated to the user while we wait for the server to catch up, without having the side effect of every position update coming in twice and filling the pending event buffer which the stacking code needed to search with bogus events.

As such, part two of this branch is more-or-less redundant in lieu of that. If you're not getting warnings about unhandled ConfigureNotify events on the stdout, I'm happy to SRU it though.

I think that part 1 is an interesting idea, but it should be implemented in a more direct way - instead of not processing movement we should make the event queue in core fill up once every 25ms if there are pending motion notify events, and discard all motion notify events. At the moment, we're discarding all but the last motion event every single time the event handler source is called, and theoretically, if compiz is running fast enough, it will wake up and return to poll () for every single event processed rather than doing them all at once.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Actually, part 2 in my experience provided most of the fix for this bug. And I honestly can't tell if your proposal will resolve that problem in the same way.

Unfortunately your proposals are now so complex and inter-dependent that I am no longer willing to try and apply/test them myself. So I can't confirm your proposal fixes bug 764330.

The best I can suggest is that you take over bug 764330 and provide a PPA for the users to test. So we can be sure bug 764330 is fixed by your changes instead.

[Sam Spilsbury (smspillaz)]

Part 2 of your fix may indeed fix most of the problem. However, its dangerous. The whole reason why that locking code was there in the first place is because the move plugin was incorrectly updating the internal window geometry, which is only supposed to be updated whenever the server sends a ConfigureNotify event. There is a race condition where if we have pending configure notify events and the move plugin goes ahead and writes directly to w->geometry () or w->serverGeometry (), incoming events won't be matched because w->geometry () is not being updated in the same order that the server is sending us requests in.

The API reworking that I did in order to correctly fix this problem's root cause are still unfortunately blocked pending review. We have someone who might be able to review them this week, but that really depends on availability.

[Daniel van Vugt (vanvugt)]

This proposal is now on hold. Waiting for Sam's alternative fixes to be reviewed and tested.

[Daniel van Vugt (vanvugt)]

I am rejecting my own proposal... There's not only Sam's alternative fix (needs work), but I've also proposed a better (and tiny) fix for this bug if you look at the branch links of bug 764330.

Unmerged revisions

2898. By Daniel van Vugt on 2011-12-21

Fix severe lag and "freezing" when dragging windows (LP: #764330)

This fixes the two root causes:
 1. Too many input events for slow systems to keep up with. Often compiz
    receives 1000 MotionNotify's per second. This is many more than is
    perceptible on screen. So the new default is to sample the mouse position
    a maximum of 200 times per second (also configurable).
 2. Lazy Positioning was sometimes not being used even when it's enabled.
    This caused long and slow sync calls, and amplified the flooded event
    queue problem above.

=== modified file 'plugins/move/move.xml.in'
--- plugins/move/move.xml.in	2010-05-25 06:26:27 +0000
+++ plugins/move/move.xml.in	2011-12-21 06:08:24 +0000
@@ -43,6 +43,13 @@
 		<_long>Do not update the server-side position of windows until finished moving</_long>
 		<default>true</default>
 	    </option>
+	    <option name="sample_rate" type="int">
+		<_short>Sample Rate</_short>
+		<_long>The (maximum) number of times per second the mouse position will be sampled while moving. Higher values cause higher CPU usage of compiz and X. Lower values may cause stuttering. A value of zero means infinity (respond to every single mouse event - not recommended).</_long>
+		<default>200</default>
+		<min>0</min>
+		<max>1000</max>
+	    </option>
 	</options>
     </plugin>
 </compiz>

=== modified file 'plugins/move/src/move.cpp'
--- plugins/move/src/move.cpp	2011-10-15 11:00:51 +0000
+++ plugins/move/src/move.cpp	2011-12-21 06:08:24 +0000
@@ -96,8 +96,8 @@
 	ms->x = 0;
 	ms->y = 0;
 
-	lastPointerX = x;
-	lastPointerY = y;
+	ms->lastRootX = x;
+	ms->lastRootY = y;
 
 	sourceExternalApp =
 	    CompOption::getBoolOptionNamed (options, "external", false);
@@ -321,8 +321,10 @@
 	wHeight = w->geometry ().height () +
 		  w->geometry ().border () * 2;
 
-	ms->x += xRoot - lastPointerX;
-	ms->y += yRoot - lastPointerY;
+	ms->x += xRoot - ms->lastRootX;
+	ms->y += yRoot - ms->lastRootY;
+	ms->lastRootX = xRoot;
+	ms->lastRootY = yRoot;
 
 	if (w->type () & CompWindowTypeFullscreenMask)
 	{
@@ -487,20 +489,8 @@
 	    w->move (wX + dx - w->geometry ().x (),
 		     wY + dy - w->geometry ().y (), false);
 
-	    if (ms->optionGetLazyPositioning () &&
-		ms->hasCompositing &&
-		!MoveWindow::get (ms->w)->mLocked)
-	    {
-		/* FIXME: This form of lazy positioning is broken and should
-		   be replaced asap. Current code exists just to avoid a
-		   major performance regression in the 0.5.2 release. */
-		w->serverGeometry ().setX (w->geometry ().x ());
-		w->serverGeometry ().setY (w->geometry ().y ());
-	    }
-	    else
-	    {
+	    if (!ms->optionGetLazyPositioning ())
 		w->syncPosition ();
-	    }
 
 	    ms->x -= dx;
 	    ms->y -= dy;
@@ -508,30 +498,26 @@
     }
 }
 
-/* FIXME: This is a hack to prevent a race condition
- * when core is processing ConfigureNotify events. It
- * MUST be removed after 0.9.6 when we can break ABI
- * and do lazy positioning correctly ! */
+bool
+MoveScreen::updateMotion ()
+{
+    if (grab)
+	moveHandleMotionEvent (screen, pointerX, pointerY);
+    return false;
+}
 
 void
-MoveScreen::handleCompizEvent (const char *plugin, const char *event, CompOption::Vector &options)
+MoveScreen::onMotion ()
 {
-    if (w)
+    if (grab && !delay.active ())
     {
-	if (std::string ("core") == std::string (plugin))
-	{
-	    if (std::string ("lock_position") == std::string (event))
-	    {
-		Window xid = CompOption::getIntOptionNamed (options, "window", 0);
-		int    lock = CompOption::getIntOptionNamed (options, "active", 0);
+	updateMotion ();
 
-		if (xid == ROOTPARENT (w))
-		    MoveWindow::get (w)->mLocked = lock ? true : false;
-	    }
-	}
+	int rate = optionGetSampleRate ();
+	if (rate > 0)
+	    delay.start (boost::bind (&MoveScreen::updateMotion, this),
+	                 1000/rate);
     }
-
-    screen->handleCompizEvent (plugin, event, options);
 }
 
 void
@@ -577,12 +563,12 @@
 	    break;
 	case MotionNotify:
 	    if (event->xmotion.root == screen->root ())
-		moveHandleMotionEvent (screen, pointerX, pointerY);
+		onMotion ();
 	    break;
 	case EnterNotify:
 	case LeaveNotify:
 	    if (event->xcrossing.root == screen->root ())
-		moveHandleMotionEvent (screen, pointerX, pointerY);
+		onMotion ();
 	    break;
 	case ClientMessage:
 	    if (event->xclient.message_type == Atoms::wmMoveResize)
@@ -634,7 +620,7 @@
 				moveInitiate (&optionGetInitiateButton (),
 					      CompAction::StateInitButton, o);
 
-				moveHandleMotionEvent (screen, pointerX, pointerY);
+				onMotion ();
 			    }
 			}
 		    }
@@ -721,6 +707,8 @@
     PluginClassHandler<MoveScreen,CompScreen> (screen),
     cScreen (CompositeScreen::get (screen)),
     w (0),
+    lastRootX (0),
+    lastRootY (0),
     region (NULL),
     status (RectangleOut),
     releaseButton (0),
@@ -755,6 +743,8 @@
 
 MoveScreen::~MoveScreen ()
 {
+    delay.stop ();
+
     if (region)
 	XDestroyRegion (region);
 

=== modified file 'plugins/move/src/move.h'
--- plugins/move/src/move.h	2011-10-15 11:00:51 +0000
+++ plugins/move/src/move.h	2011-12-21 06:08:24 +0000
@@ -64,18 +64,21 @@
 	void updateOpacity ();
 
 	void handleEvent (XEvent *);
-	void handleCompizEvent (const char *plugin,
-				const char *event,
-				CompOption::Vector &options);
 
 	bool registerPaintHandler (compiz::composite::PaintHandler *pHnd);
 	void unregisterPaintHandler ();
 
+	bool updateMotion ();
+	void onMotion ();
+
+	CompTimer  delay;
 	CompWindow *w;
 	int        savedX;
 	int        savedY;
 	int        x;
 	int        y;
+	int        lastRootX;
+	int        lastRootY;
 	Region     region;
 	int        status;
 	KeyCode    key[NUM_KEYS];
@@ -107,8 +110,7 @@
 	    PluginClassHandler<MoveWindow,CompWindow> (window),
 	    window (window),
 	    gWindow (GLWindow::get (window)),
-	    cWindow (CompositeWindow::get (window)),
-	    mLocked (false)
+	    cWindow (CompositeWindow::get (window))
 	{
 	    if (gWindow)
 		GLWindowInterface::setHandler (gWindow, false);
@@ -120,8 +122,6 @@
 	CompWindow      *window;
 	GLWindow        *gWindow;
 	CompositeWindow *cWindow;
-
-	bool            mLocked;
 };
 
 #define MOVE_SCREEN(s) \