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

Proposed by Daniel van Vugt
Status: Rejected
Rejected by: Daniel van Vugt
Proposed branch: lp:~vanvugt/compiz-core/fix-764330-trunk
Merge into: lp:compiz-core/0.9.5
Diff against target: 202 lines (+42/-45)
3 files modified
plugins/move/move.xml.in (+7/-0)
plugins/move/src/move.cpp (+28/-38)
plugins/move/src/move.h (+7/-7)
To merge this branch: bzr merge lp:~vanvugt/compiz-core/fix-764330-trunk
Reviewer Review Type Date Requested Status
Sam Spilsbury Needs Fixing
Review via email: mp+86497@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

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
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

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.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
43 <_long>Do not update the server-side position of windows until finished moving</_long>43 <_long>Do not update the server-side position of windows until finished moving</_long>
44 <default>true</default>44 <default>true</default>
45 </option>45 </option>
46 <option name="sample_rate" type="int">
47 <_short>Sample Rate</_short>
48 <_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>
49 <default>200</default>
50 <min>0</min>
51 <max>1000</max>
52 </option>
46 </options>53 </options>
47 </plugin>54 </plugin>
48</compiz>55</compiz>
4956
=== 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 @@
96 ms->x = 0;96 ms->x = 0;
97 ms->y = 0;97 ms->y = 0;
9898
99 lastPointerX = x;99 ms->lastRootX = x;
100 lastPointerY = y;100 ms->lastRootY = y;
101101
102 sourceExternalApp =102 sourceExternalApp =
103 CompOption::getBoolOptionNamed (options, "external", false);103 CompOption::getBoolOptionNamed (options, "external", false);
@@ -321,8 +321,10 @@
321 wHeight = w->geometry ().height () +321 wHeight = w->geometry ().height () +
322 w->geometry ().border () * 2;322 w->geometry ().border () * 2;
323323
324 ms->x += xRoot - lastPointerX;324 ms->x += xRoot - ms->lastRootX;
325 ms->y += yRoot - lastPointerY;325 ms->y += yRoot - ms->lastRootY;
326 ms->lastRootX = xRoot;
327 ms->lastRootY = yRoot;
326328
327 if (w->type () & CompWindowTypeFullscreenMask)329 if (w->type () & CompWindowTypeFullscreenMask)
328 {330 {
@@ -487,20 +489,8 @@
487 w->move (wX + dx - w->geometry ().x (),489 w->move (wX + dx - w->geometry ().x (),
488 wY + dy - w->geometry ().y (), false);490 wY + dy - w->geometry ().y (), false);
489491
490 if (ms->optionGetLazyPositioning () &&492 if (!ms->optionGetLazyPositioning ())
491 ms->hasCompositing &&
492 !MoveWindow::get (ms->w)->mLocked)
493 {
494 /* FIXME: This form of lazy positioning is broken and should
495 be replaced asap. Current code exists just to avoid a
496 major performance regression in the 0.5.2 release. */
497 w->serverGeometry ().setX (w->geometry ().x ());
498 w->serverGeometry ().setY (w->geometry ().y ());
499 }
500 else
501 {
502 w->syncPosition ();493 w->syncPosition ();
503 }
504494
505 ms->x -= dx;495 ms->x -= dx;
506 ms->y -= dy;496 ms->y -= dy;
@@ -508,30 +498,26 @@
508 }498 }
509}499}
510500
511/* FIXME: This is a hack to prevent a race condition501bool
512 * when core is processing ConfigureNotify events. It502MoveScreen::updateMotion ()
513 * MUST be removed after 0.9.6 when we can break ABI503{
514 * and do lazy positioning correctly ! */504 if (grab)
505 moveHandleMotionEvent (screen, pointerX, pointerY);
506 return false;
507}
515508
516void509void
517MoveScreen::handleCompizEvent (const char *plugin, const char *event, CompOption::Vector &options)510MoveScreen::onMotion ()
518{511{
519 if (w)512 if (grab && !delay.active ())
520 {513 {
521 if (std::string ("core") == std::string (plugin))514 updateMotion ();
522 {
523 if (std::string ("lock_position") == std::string (event))
524 {
525 Window xid = CompOption::getIntOptionNamed (options, "window", 0);
526 int lock = CompOption::getIntOptionNamed (options, "active", 0);
527515
528 if (xid == ROOTPARENT (w))516 int rate = optionGetSampleRate ();
529 MoveWindow::get (w)->mLocked = lock ? true : false;517 if (rate > 0)
530 }518 delay.start (boost::bind (&MoveScreen::updateMotion, this),
531 }519 1000/rate);
532 }520 }
533
534 screen->handleCompizEvent (plugin, event, options);
535}521}
536522
537void523void
@@ -577,12 +563,12 @@
577 break;563 break;
578 case MotionNotify:564 case MotionNotify:
579 if (event->xmotion.root == screen->root ())565 if (event->xmotion.root == screen->root ())
580 moveHandleMotionEvent (screen, pointerX, pointerY);566 onMotion ();
581 break;567 break;
582 case EnterNotify:568 case EnterNotify:
583 case LeaveNotify:569 case LeaveNotify:
584 if (event->xcrossing.root == screen->root ())570 if (event->xcrossing.root == screen->root ())
585 moveHandleMotionEvent (screen, pointerX, pointerY);571 onMotion ();
586 break;572 break;
587 case ClientMessage:573 case ClientMessage:
588 if (event->xclient.message_type == Atoms::wmMoveResize)574 if (event->xclient.message_type == Atoms::wmMoveResize)
@@ -634,7 +620,7 @@
634 moveInitiate (&optionGetInitiateButton (),620 moveInitiate (&optionGetInitiateButton (),
635 CompAction::StateInitButton, o);621 CompAction::StateInitButton, o);
636622
637 moveHandleMotionEvent (screen, pointerX, pointerY);623 onMotion ();
638 }624 }
639 }625 }
640 }626 }
@@ -721,6 +707,8 @@
721 PluginClassHandler<MoveScreen,CompScreen> (screen),707 PluginClassHandler<MoveScreen,CompScreen> (screen),
722 cScreen (CompositeScreen::get (screen)),708 cScreen (CompositeScreen::get (screen)),
723 w (0),709 w (0),
710 lastRootX (0),
711 lastRootY (0),
724 region (NULL),712 region (NULL),
725 status (RectangleOut),713 status (RectangleOut),
726 releaseButton (0),714 releaseButton (0),
@@ -755,6 +743,8 @@
755743
756MoveScreen::~MoveScreen ()744MoveScreen::~MoveScreen ()
757{745{
746 delay.stop ();
747
758 if (region)748 if (region)
759 XDestroyRegion (region);749 XDestroyRegion (region);
760750
761751
=== 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 @@
64 void updateOpacity ();64 void updateOpacity ();
6565
66 void handleEvent (XEvent *);66 void handleEvent (XEvent *);
67 void handleCompizEvent (const char *plugin,
68 const char *event,
69 CompOption::Vector &options);
7067
71 bool registerPaintHandler (compiz::composite::PaintHandler *pHnd);68 bool registerPaintHandler (compiz::composite::PaintHandler *pHnd);
72 void unregisterPaintHandler ();69 void unregisterPaintHandler ();
7370
71 bool updateMotion ();
72 void onMotion ();
73
74 CompTimer delay;
74 CompWindow *w;75 CompWindow *w;
75 int savedX;76 int savedX;
76 int savedY;77 int savedY;
77 int x;78 int x;
78 int y;79 int y;
80 int lastRootX;
81 int lastRootY;
79 Region region;82 Region region;
80 int status;83 int status;
81 KeyCode key[NUM_KEYS];84 KeyCode key[NUM_KEYS];
@@ -107,8 +110,7 @@
107 PluginClassHandler<MoveWindow,CompWindow> (window),110 PluginClassHandler<MoveWindow,CompWindow> (window),
108 window (window),111 window (window),
109 gWindow (GLWindow::get (window)),112 gWindow (GLWindow::get (window)),
110 cWindow (CompositeWindow::get (window)),113 cWindow (CompositeWindow::get (window))
111 mLocked (false)
112 {114 {
113 if (gWindow)115 if (gWindow)
114 GLWindowInterface::setHandler (gWindow, false);116 GLWindowInterface::setHandler (gWindow, false);
@@ -120,8 +122,6 @@
120 CompWindow *window;122 CompWindow *window;
121 GLWindow *gWindow;123 GLWindow *gWindow;
122 CompositeWindow *cWindow;124 CompositeWindow *cWindow;
123
124 bool mLocked;
125};125};
126126
127#define MOVE_SCREEN(s) \127#define MOVE_SCREEN(s) \

Subscribers

People subscribed via source and target branches