Merge lp:~compiz-team/compiz/compiz.fix_1006335 into lp:compiz/0.9.8

Proposed by Sam Spilsbury on 2012-06-05
Status: Merged
Merged at revision: 3243
Proposed branch: lp:~compiz-team/compiz/compiz.fix_1006335
Merge into: lp:compiz/0.9.8
Diff against target: 303 lines (+98/-47)
4 files modified
src/privatescreen.h (+22/-2)
src/privatestackdebugger.h (+9/-4)
src/screen.cpp (+37/-30)
src/stackdebugger.cpp (+30/-11)
To merge this branch: bzr merge lp:~compiz-team/compiz/compiz.fix_1006335
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-06-05 Approve on 2012-06-06
Compiz Maintainers 2012-06-05 Pending
Review via email: mp+108706@code.launchpad.net

This proposal supersedes a proposal from 2012-06-02.

Description of the change

Fixes needless list copying in event queues

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

There's a risk of crashing, by assuming there are still only XEventsQueued by the time we consume them all. The server might have more events ready by then. More than we sized the event vector for, so either:
        XNextEvent (dpy, &(*it));
or
        it++
will crash. Because it is now past the end of the vector.

But it's a simple fix. Just use reserve() instead of resize() and push_back instead of writing to *it.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Also I'm still concerned by the use of the heap on every single event:

void
PrivateScreen::processEvents ()
{
    std::vector <XEvent> events;

The solution I had in mind for this bug didn't require that. Maybe I'll prototype it to see if it works.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

OK, my first comment is mistaken. But I'd like to see a solution that bypasses the heap for most events.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

That's a lot more complicated than the solution I had in mind, but if it works...

Also, I recommend not changing unrelated logic like the location of:
    windowManager.removeDestroyed ();
The change may be correct, but changing logic not related to the bug in question is best avoided.

review: Needs Fixing
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

On Fri, 1 Jun 2012, Daniel van Vugt wrote:

> Review: Needs Fixing
>
> That's a lot more complicated than the solution I had in mind, but if it works...
>
> Also, I recommend not changing unrelated logic like the location of:
> windowManager.removeDestroyed ();
> The change may be correct, but changing logic not related to the bug in question is best avoided.

Actually I changed it because we should be removing destroyed windows at
the end of every event processed. Without that change, we might never do
it.

Could you clarify what else needs fixing?

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1006335/+merge/107953
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~compiz-team/compiz/compiz.fix_1006335 into lp:compiz.
>

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

windowManager.removeDestroyed ();

is what needs fixing. Your reasoning for moving it sounds correct but it's not related to bug 1006335 so please remove it from the proposal.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Sorry, I just noticed:
    std::deque
should be:
    std::list

Because std::deque introduces extra complexity to implement random access. It's actually implemented as a red-black tree like std::map or std::set. But since we're not using random access and only need head/tail insertion and removal, a simple doubly linked list (std::list) will be faster and smaller.

This proposal is still much larger than it should be. It's an itch I have to scratch but I'm trying not to push the point too much. :/

review: Needs Fixing
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks, recompiling.

I don't think that the proposal is too large, however it does optimize bits that are only really used in debug mode, which pushed up the size a bit.

Sam Spilsbury (smspillaz) wrote :

Done.

Daniel van Vugt (vanvugt) wrote :

Looks OK and no obvious regressions.

I can't do a realistic comparative profile with callgrind right now because I've switched drivers and it would change my normal expected results. But this appears to solve the bug.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/privatescreen.h'
2--- src/privatescreen.h 2012-05-23 07:37:37 +0000
3+++ src/privatescreen.h 2012-06-05 09:07:25 +0000
4@@ -608,8 +608,27 @@
5
6 }} // namespace compiz::private_screen
7
8+class FetchXEventInterface
9+{
10+ public:
11+
12+ virtual ~FetchXEventInterface () {}
13+
14+ virtual bool getNextXEvent (XEvent &) = 0;
15+};
16+
17+class FetchEventInterface
18+{
19+ public:
20+
21+ virtual ~FetchEventInterface () {}
22+ virtual bool getNextEvent (XEvent &) = 0;
23+};
24+
25 class PrivateScreen :
26- public CoreOptions
27+ public CoreOptions,
28+ public FetchXEventInterface,
29+ public FetchEventInterface
30 {
31
32 public:
33@@ -623,7 +642,8 @@
34
35 bool setOption (const CompString &name, CompOption::Value &value);
36
37- std::list <XEvent> queueEvents ();
38+ bool getNextEvent (XEvent &);
39+ bool getNextXEvent (XEvent &);
40 void processEvents ();
41
42 bool triggerButtonPressBindings (CompOption::Vector &options,
43
44=== modified file 'src/privatestackdebugger.h'
45--- src/privatestackdebugger.h 2011-09-29 03:29:41 +0000
46+++ src/privatestackdebugger.h 2012-06-05 09:07:25 +0000
47@@ -27,20 +27,23 @@
48 * so that this can be come a truly standalone
49 * object */
50 #include <core/core.h>
51+#include <list>
52
53 #ifndef _COMPIZ_PRIVATESTACKDEBUGGER_H
54 #define _COMPIZ_PRIVATESTACKDEBUGGER_H
55
56+class FetchXEventInterface;
57+
58 class StackDebugger
59 {
60 public:
61
62- typedef std::list<XEvent> eventList;
63+ typedef std::vector<XEvent> eventList;
64
65- StackDebugger (Display *, Window, boost::function<eventList ()> evProc);
66+ StackDebugger (Display *, Window, FetchXEventInterface *fetchXEvent);
67 ~StackDebugger ();
68
69- eventList loadStack (CompWindowList &serverWindows, bool wait = false);
70+ void loadStack (CompWindowList &serverWindows, bool wait = false);
71 void windowsChanged (bool change) { mWindowsChanged = change; };
72 void serverWindowsChanged (bool change) { mServerWindowsChanged = change; };
73 bool windowsChanged () { return mWindowsChanged; }
74@@ -54,6 +57,7 @@
75 CompWindowList &serverWindows,
76 bool verbose = false);
77 bool timedOut ();
78+ bool getNextEvent (XEvent &);
79
80 bool checkSanity (CompWindowList &serverWindows, bool verbose = false);
81
82@@ -70,9 +74,10 @@
83 bool mServerWindowsChanged;
84 Window mRoot;
85 Display *mDpy;
86- boost::function<eventList ()> getEventsProc;
87+ FetchXEventInterface *mFetchXEvent;
88 bool mTimeoutRequired;
89 CompWindowList mLastServerWindows;
90+ std::list <XEvent> mEvents;
91 };
92
93 #endif
94
95=== modified file 'src/screen.cpp'
96--- src/screen.cpp 2012-05-25 07:37:42 +0000
97+++ src/screen.cpp 2012-06-05 09:07:25 +0000
98@@ -752,41 +752,48 @@
99 return rv;
100 }
101
102-std::list <XEvent>
103-PrivateScreen::queueEvents ()
104+bool
105+PrivateScreen::getNextXEvent (XEvent &ev)
106 {
107- std::list <XEvent> events;
108+ if (!XEventsQueued (dpy, QueuedAlready))
109+ return false;
110+ XNextEvent (dpy, &ev);
111
112- while (XPending (dpy))
113+ /* Skip to the last MotionNotify
114+ * event in this sequence */
115+ if (ev.type == MotionNotify)
116 {
117- XEvent event, peekEvent;
118- XNextEvent (dpy, &event);
119-
120- /* Skip to the last MotionNotify
121- * event in this sequence */
122- if (event.type == MotionNotify)
123+ XEvent peekEvent;
124+ while (XPending (dpy))
125 {
126- while (XPending (dpy))
127- {
128- XPeekEvent (dpy, &peekEvent);
129-
130- if (peekEvent.type != MotionNotify)
131- break;
132-
133- XNextEvent (dpy, &event);
134- }
135+ XPeekEvent (dpy, &peekEvent);
136+
137+ if (peekEvent.type != MotionNotify)
138+ break;
139+
140+ XNextEvent (dpy, &peekEvent);
141 }
142-
143- events.push_back (event);
144- }
145-
146- return events;
147+ }
148+
149+ return true;
150+}
151+
152+bool
153+PrivateScreen::getNextEvent (XEvent &ev)
154+{
155+ StackDebugger *dbg = StackDebugger::Default ();
156+
157+ if (StackDebugger::Default ())
158+ {
159+ return dbg->getNextEvent (ev);
160+ }
161+ else
162+ return getNextXEvent (ev);
163 }
164
165 void
166 PrivateScreen::processEvents ()
167 {
168- std::list <XEvent> events;
169 StackDebugger *dbg = StackDebugger::Default ();
170
171 if (pluginManager.isDirtyPluginList ())
172@@ -801,14 +808,14 @@
173 {
174 dbg->windowsChanged (false);
175 dbg->serverWindowsChanged (false);
176- events = dbg->loadStack (windowManager.getServerWindows());
177+ dbg->loadStack (windowManager.getServerWindows());
178 }
179- else
180- events = queueEvents ();
181
182 windowManager.invalidateServerWindows();
183
184- foreach (XEvent &event, events)
185+ XEvent event;
186+
187+ while (getNextEvent (event))
188 {
189 switch (event.type) {
190 case ButtonPress:
191@@ -4792,7 +4799,7 @@
192 new StackDebugger (
193 dpy (),
194 root (),
195- boost::bind (&PrivateScreen::queueEvents, &privateScreen)));
196+ &privateScreen));
197 }
198
199 return true;
200
201=== modified file 'src/stackdebugger.cpp'
202--- src/stackdebugger.cpp 2011-10-07 11:45:38 +0000
203+++ src/stackdebugger.cpp 2012-06-05 09:07:25 +0000
204@@ -24,6 +24,7 @@
205 */
206
207 #include "privatestackdebugger.h"
208+#include "privatescreen.h"
209 #include "privatewindow.h"
210 #include <poll.h>
211
212@@ -47,14 +48,14 @@
213 gStackDebugger = dbg;
214 }
215
216-StackDebugger::StackDebugger (Display *dpy, Window root, boost::function <eventList ()> evProc) :
217+StackDebugger::StackDebugger (Display *dpy, Window root, FetchXEventInterface *fetchXEvent) :
218 mServerNChildren (0),
219 mServerChildren (NULL),
220 mWindowsChanged (false),
221 mServerWindowsChanged (false),
222 mRoot (root),
223 mDpy (dpy),
224- getEventsProc (evProc)
225+ mFetchXEvent (fetchXEvent)
226 {
227 }
228
229@@ -118,11 +119,23 @@
230 mLastServerWindows.push_front (tl);
231 }
232
233-StackDebugger::eventList
234+bool
235+StackDebugger::getNextEvent (XEvent &ev)
236+{
237+ if (mEvents.empty ())
238+ return false;
239+
240+ ev = mEvents.front ();
241+
242+ mEvents.pop_front ();
243+
244+ return true;
245+}
246+
247+void
248 StackDebugger::loadStack (CompWindowList &serverWindows, bool wait)
249 {
250 Window rootRet, parentRet;
251- eventList events;
252
253 if (mServerChildren)
254 XFree (mServerChildren);
255@@ -132,7 +145,16 @@
256 XQueryTree (mDpy, mRoot, &rootRet, &parentRet,
257 &mServerChildren, &mServerNChildren);
258
259- events = getEventsProc ();
260+ unsigned int n = XEventsQueued (mDpy, QueuedAfterFlush);
261+ mEvents.clear ();
262+ mEvents.resize (n);
263+ std::list <XEvent>::iterator it = mEvents.begin ();
264+
265+ while (it != mEvents.end ())
266+ {
267+ mFetchXEvent->getNextXEvent ((*it));
268+ it++;
269+ }
270
271 XSync (mDpy, FALSE);
272
273@@ -145,7 +167,6 @@
274
275 if (mServerNChildren != serverWindows.size () && wait)
276 {
277- eventList moreEvents;
278 struct pollfd pfd;
279
280 pfd.events = POLLIN;
281@@ -154,10 +175,10 @@
282
283 poll (&pfd, 1, 300);
284
285- moreEvents = getEventsProc ();
286+ XEvent e;
287
288- foreach (XEvent e, moreEvents)
289- events.push_back (e);
290+ while (mFetchXEvent->getNextXEvent (e))
291+ mEvents.push_back (e);
292
293 mTimeoutRequired = true;
294 }
295@@ -166,8 +187,6 @@
296
297 XUngrabServer (mDpy);
298 XSync (mDpy, FALSE);
299-
300- return events;
301 }
302
303 void

Subscribers

People subscribed via source and target branches