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

[Sam Spilsbury]

Fixes needless list copying in event queues

[Daniel van Vugt (vanvugt)]

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)]

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)]

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)]

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)]

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)]

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)]

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)]

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)]

Done.

[Daniel van Vugt (vanvugt)]

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

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-05-23 07:37:37 +0000
+++ src/privatescreen.h	2012-06-05 09:07:25 +0000
@@ -608,8 +608,27 @@
 
 }} // namespace compiz::private_screen
 
+class FetchXEventInterface
+{
+    public:
+
+	virtual ~FetchXEventInterface () {}
+
+	virtual bool getNextXEvent (XEvent &) = 0;
+};
+
+class FetchEventInterface
+{
+    public:
+
+	virtual ~FetchEventInterface () {}
+	virtual bool getNextEvent (XEvent &) = 0;
+};
+
 class PrivateScreen :
-    public CoreOptions
+    public CoreOptions,
+    public FetchXEventInterface,
+    public FetchEventInterface
 {
 
     public:
@@ -623,7 +642,8 @@
 
 	bool setOption (const CompString &name, CompOption::Value &value);
 
-	std::list <XEvent> queueEvents ();
+	bool getNextEvent (XEvent &);
+	bool getNextXEvent (XEvent &);
 	void processEvents ();
 
 	bool triggerButtonPressBindings (CompOption::Vector &options,
 
=== modified file 'src/privatestackdebugger.h'
--- src/privatestackdebugger.h	2011-09-29 03:29:41 +0000
+++ src/privatestackdebugger.h	2012-06-05 09:07:25 +0000
@@ -27,20 +27,23 @@
  * so that this can be come a truly standalone
  * object */
 #include <core/core.h>
+#include <list>
 
 #ifndef _COMPIZ_PRIVATESTACKDEBUGGER_H
 #define _COMPIZ_PRIVATESTACKDEBUGGER_H
 
+class FetchXEventInterface;
+
 class StackDebugger
 {
     public:
 
-	typedef std::list<XEvent> eventList;
+	typedef std::vector<XEvent> eventList;
 
-	StackDebugger (Display *, Window, boost::function<eventList ()> evProc);
+	StackDebugger (Display *, Window, FetchXEventInterface *fetchXEvent);
 	~StackDebugger ();
 
-	eventList loadStack (CompWindowList &serverWindows, bool wait = false);
+	void loadStack (CompWindowList &serverWindows, bool wait = false);
 	void windowsChanged (bool change) { mWindowsChanged = change; };
 	void serverWindowsChanged (bool change) { mServerWindowsChanged = change; };
 	bool windowsChanged () { return mWindowsChanged; }
@@ -54,6 +57,7 @@
 		       CompWindowList &serverWindows,
 		       bool verbose = false);
 	bool timedOut ();
+	bool getNextEvent (XEvent &);
 
 	bool checkSanity (CompWindowList &serverWindows, bool verbose = false);
 
@@ -70,9 +74,10 @@
 	bool         mServerWindowsChanged;
 	Window       mRoot;
 	Display      *mDpy;
-	boost::function<eventList ()> getEventsProc;
+	FetchXEventInterface *mFetchXEvent;
 	bool         mTimeoutRequired;
 	CompWindowList mLastServerWindows;
+	std::list <XEvent> mEvents;
 };
 
 #endif
 
=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-05-25 07:37:42 +0000
+++ src/screen.cpp	2012-06-05 09:07:25 +0000
@@ -752,41 +752,48 @@
     return rv;
 }
 
-std::list <XEvent>
-PrivateScreen::queueEvents ()
+bool
+PrivateScreen::getNextXEvent (XEvent &ev)
 {
-    std::list <XEvent> events;
+    if (!XEventsQueued (dpy, QueuedAlready))
+	return false;
+    XNextEvent (dpy, &ev);
 
-    while (XPending (dpy))
+    /* Skip to the last MotionNotify
+     * event in this sequence */
+    if (ev.type == MotionNotify)
     {
-	XEvent event, peekEvent;
-	XNextEvent (dpy, &event);
-
-	/* Skip to the last MotionNotify
-	 * event in this sequence */
-	if (event.type == MotionNotify)
+	XEvent peekEvent;
+	while (XPending (dpy))
 	{
-	    while (XPending (dpy))
-	    {
-		XPeekEvent (dpy, &peekEvent);
-
-		if (peekEvent.type != MotionNotify)
-		    break;
-
-		XNextEvent (dpy, &event);
-	    }
+	    XPeekEvent (dpy, &peekEvent);
+
+	    if (peekEvent.type != MotionNotify)
+		break;
+
+	    XNextEvent (dpy, &peekEvent);
 	}
-
-	events.push_back (event);
-    }
-
-    return events;
+    }
+
+    return true;
+}
+
+bool
+PrivateScreen::getNextEvent (XEvent &ev)
+{
+    StackDebugger *dbg = StackDebugger::Default ();
+
+    if (StackDebugger::Default ())
+    {
+	return dbg->getNextEvent (ev);
+    }
+    else
+	return getNextXEvent (ev);
 }
 
 void
 PrivateScreen::processEvents ()
 {
-    std::list <XEvent> events;
     StackDebugger *dbg = StackDebugger::Default ();
 
     if (pluginManager.isDirtyPluginList ())
@@ -801,14 +808,14 @@
     {
 	dbg->windowsChanged (false);
 	dbg->serverWindowsChanged (false);
-	events = dbg->loadStack (windowManager.getServerWindows());
+	dbg->loadStack (windowManager.getServerWindows());
     }
-    else
-	events = queueEvents ();
 
     windowManager.invalidateServerWindows();
 
-    foreach (XEvent &event, events)
+    XEvent event;
+
+    while (getNextEvent (event))
     {
 	switch (event.type) {
 	case ButtonPress:
@@ -4792,7 +4799,7 @@
 		new StackDebugger (
 		    dpy (),
 		    root (),
-		    boost::bind (&PrivateScreen::queueEvents, &privateScreen)));
+		    &privateScreen));
 	}
 
 	return true;
 
=== modified file 'src/stackdebugger.cpp'
--- src/stackdebugger.cpp	2011-10-07 11:45:38 +0000
+++ src/stackdebugger.cpp	2012-06-05 09:07:25 +0000
@@ -24,6 +24,7 @@
  */
 
 #include "privatestackdebugger.h"
+#include "privatescreen.h"
 #include "privatewindow.h"
 #include <poll.h>
 
@@ -47,14 +48,14 @@
     gStackDebugger = dbg;
 }
 
-StackDebugger::StackDebugger (Display *dpy, Window root, boost::function <eventList ()> evProc) :
+StackDebugger::StackDebugger (Display *dpy, Window root, FetchXEventInterface *fetchXEvent) :
     mServerNChildren (0),
     mServerChildren (NULL),
     mWindowsChanged (false),
     mServerWindowsChanged (false),
     mRoot (root),
     mDpy (dpy),
-    getEventsProc (evProc)
+    mFetchXEvent (fetchXEvent)
 {
 }
 
@@ -118,11 +119,23 @@
 	mLastServerWindows.push_front (tl);
 }
 
-StackDebugger::eventList
+bool
+StackDebugger::getNextEvent (XEvent &ev)
+{
+    if (mEvents.empty ())
+	return false;
+
+    ev = mEvents.front ();
+
+    mEvents.pop_front ();
+
+    return true;
+}
+
+void
 StackDebugger::loadStack (CompWindowList &serverWindows, bool wait)
 {
     Window rootRet, parentRet;
-    eventList events;
 
     if (mServerChildren)
 	XFree (mServerChildren);
@@ -132,7 +145,16 @@
     XQueryTree (mDpy, mRoot, &rootRet, &parentRet,
 		&mServerChildren, &mServerNChildren);
 
-    events = getEventsProc ();
+    unsigned int n = XEventsQueued (mDpy, QueuedAfterFlush);
+    mEvents.clear ();
+    mEvents.resize (n);
+    std::list <XEvent>::iterator it = mEvents.begin ();
+
+    while (it != mEvents.end ())
+    {
+	mFetchXEvent->getNextXEvent ((*it));
+	it++;
+    }
 
     XSync (mDpy, FALSE);
 
@@ -145,7 +167,6 @@
 
     if (mServerNChildren != serverWindows.size () && wait)
     {
-	eventList moreEvents;
 	struct pollfd pfd;
 
 	pfd.events = POLLIN;
@@ -154,10 +175,10 @@
 
 	poll (&pfd, 1, 300);
 
-	moreEvents = getEventsProc ();
+	XEvent e;
 
-	foreach (XEvent e, moreEvents)
-	    events.push_back (e);
+	while (mFetchXEvent->getNextXEvent (e))
+	    mEvents.push_back (e);
 
 	mTimeoutRequired = true;
     }
@@ -166,8 +187,6 @@
 
     XUngrabServer (mDpy);
     XSync (mDpy, FALSE);
-
-    return events;
 }
 
 void