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

Proposed by Daniel van Vugt on 2011-12-21
Status: Rejected
Rejected by: Daniel van Vugt on 2012-01-18
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 2011-12-21 Needs Fixing on 2011-12-21
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.
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
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.

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.

Daniel van Vugt (vanvugt) wrote :

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

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 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/move/move.xml.in'
2--- plugins/move/move.xml.in 2010-05-25 06:26:27 +0000
3+++ plugins/move/move.xml.in 2011-12-21 06:08:24 +0000
4@@ -43,6 +43,13 @@
5 <_long>Do not update the server-side position of windows until finished moving</_long>
6 <default>true</default>
7 </option>
8+ <option name="sample_rate" type="int">
9+ <_short>Sample Rate</_short>
10+ <_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>
11+ <default>200</default>
12+ <min>0</min>
13+ <max>1000</max>
14+ </option>
15 </options>
16 </plugin>
17 </compiz>
18
19=== modified file 'plugins/move/src/move.cpp'
20--- plugins/move/src/move.cpp 2011-10-15 11:00:51 +0000
21+++ plugins/move/src/move.cpp 2011-12-21 06:08:24 +0000
22@@ -96,8 +96,8 @@
23 ms->x = 0;
24 ms->y = 0;
25
26- lastPointerX = x;
27- lastPointerY = y;
28+ ms->lastRootX = x;
29+ ms->lastRootY = y;
30
31 sourceExternalApp =
32 CompOption::getBoolOptionNamed (options, "external", false);
33@@ -321,8 +321,10 @@
34 wHeight = w->geometry ().height () +
35 w->geometry ().border () * 2;
36
37- ms->x += xRoot - lastPointerX;
38- ms->y += yRoot - lastPointerY;
39+ ms->x += xRoot - ms->lastRootX;
40+ ms->y += yRoot - ms->lastRootY;
41+ ms->lastRootX = xRoot;
42+ ms->lastRootY = yRoot;
43
44 if (w->type () & CompWindowTypeFullscreenMask)
45 {
46@@ -487,20 +489,8 @@
47 w->move (wX + dx - w->geometry ().x (),
48 wY + dy - w->geometry ().y (), false);
49
50- if (ms->optionGetLazyPositioning () &&
51- ms->hasCompositing &&
52- !MoveWindow::get (ms->w)->mLocked)
53- {
54- /* FIXME: This form of lazy positioning is broken and should
55- be replaced asap. Current code exists just to avoid a
56- major performance regression in the 0.5.2 release. */
57- w->serverGeometry ().setX (w->geometry ().x ());
58- w->serverGeometry ().setY (w->geometry ().y ());
59- }
60- else
61- {
62+ if (!ms->optionGetLazyPositioning ())
63 w->syncPosition ();
64- }
65
66 ms->x -= dx;
67 ms->y -= dy;
68@@ -508,30 +498,26 @@
69 }
70 }
71
72-/* FIXME: This is a hack to prevent a race condition
73- * when core is processing ConfigureNotify events. It
74- * MUST be removed after 0.9.6 when we can break ABI
75- * and do lazy positioning correctly ! */
76+bool
77+MoveScreen::updateMotion ()
78+{
79+ if (grab)
80+ moveHandleMotionEvent (screen, pointerX, pointerY);
81+ return false;
82+}
83
84 void
85-MoveScreen::handleCompizEvent (const char *plugin, const char *event, CompOption::Vector &options)
86+MoveScreen::onMotion ()
87 {
88- if (w)
89+ if (grab && !delay.active ())
90 {
91- if (std::string ("core") == std::string (plugin))
92- {
93- if (std::string ("lock_position") == std::string (event))
94- {
95- Window xid = CompOption::getIntOptionNamed (options, "window", 0);
96- int lock = CompOption::getIntOptionNamed (options, "active", 0);
97+ updateMotion ();
98
99- if (xid == ROOTPARENT (w))
100- MoveWindow::get (w)->mLocked = lock ? true : false;
101- }
102- }
103+ int rate = optionGetSampleRate ();
104+ if (rate > 0)
105+ delay.start (boost::bind (&MoveScreen::updateMotion, this),
106+ 1000/rate);
107 }
108-
109- screen->handleCompizEvent (plugin, event, options);
110 }
111
112 void
113@@ -577,12 +563,12 @@
114 break;
115 case MotionNotify:
116 if (event->xmotion.root == screen->root ())
117- moveHandleMotionEvent (screen, pointerX, pointerY);
118+ onMotion ();
119 break;
120 case EnterNotify:
121 case LeaveNotify:
122 if (event->xcrossing.root == screen->root ())
123- moveHandleMotionEvent (screen, pointerX, pointerY);
124+ onMotion ();
125 break;
126 case ClientMessage:
127 if (event->xclient.message_type == Atoms::wmMoveResize)
128@@ -634,7 +620,7 @@
129 moveInitiate (&optionGetInitiateButton (),
130 CompAction::StateInitButton, o);
131
132- moveHandleMotionEvent (screen, pointerX, pointerY);
133+ onMotion ();
134 }
135 }
136 }
137@@ -721,6 +707,8 @@
138 PluginClassHandler<MoveScreen,CompScreen> (screen),
139 cScreen (CompositeScreen::get (screen)),
140 w (0),
141+ lastRootX (0),
142+ lastRootY (0),
143 region (NULL),
144 status (RectangleOut),
145 releaseButton (0),
146@@ -755,6 +743,8 @@
147
148 MoveScreen::~MoveScreen ()
149 {
150+ delay.stop ();
151+
152 if (region)
153 XDestroyRegion (region);
154
155
156=== modified file 'plugins/move/src/move.h'
157--- plugins/move/src/move.h 2011-10-15 11:00:51 +0000
158+++ plugins/move/src/move.h 2011-12-21 06:08:24 +0000
159@@ -64,18 +64,21 @@
160 void updateOpacity ();
161
162 void handleEvent (XEvent *);
163- void handleCompizEvent (const char *plugin,
164- const char *event,
165- CompOption::Vector &options);
166
167 bool registerPaintHandler (compiz::composite::PaintHandler *pHnd);
168 void unregisterPaintHandler ();
169
170+ bool updateMotion ();
171+ void onMotion ();
172+
173+ CompTimer delay;
174 CompWindow *w;
175 int savedX;
176 int savedY;
177 int x;
178 int y;
179+ int lastRootX;
180+ int lastRootY;
181 Region region;
182 int status;
183 KeyCode key[NUM_KEYS];
184@@ -107,8 +110,7 @@
185 PluginClassHandler<MoveWindow,CompWindow> (window),
186 window (window),
187 gWindow (GLWindow::get (window)),
188- cWindow (CompositeWindow::get (window)),
189- mLocked (false)
190+ cWindow (CompositeWindow::get (window))
191 {
192 if (gWindow)
193 GLWindowInterface::setHandler (gWindow, false);
194@@ -120,8 +122,6 @@
195 CompWindow *window;
196 GLWindow *gWindow;
197 CompositeWindow *cWindow;
198-
199- bool mLocked;
200 };
201
202 #define MOVE_SCREEN(s) \

Subscribers

People subscribed via source and target branches