Merge lp:~vanvugt/compiz/XDamageReport into lp:compiz/0.9.8

[Daniel van Vugt]

Use the XDamage extension more efficiently (the way it was designed to be
used). This dramatically reduces CPU usage, reduces wakeups, and increases
frame rates. It also solves at least one observed performance bug
(LP: #1007299) and probably several more.

Results!

--- Intel driver ---

BEFORE:
Application FPS = 5500
Compiz XDamageNotify per sec = 11000
CPU usage = 4%
Compiz frame rate = 30 FPS

AFTER:
Application FPS = 5500
Compiz XDamageNotify per sec = 120
CPU usage = 1-2%
Compiz frame rate = 60 FPS

RESULT:
2x frame rate
50-75% reduction in CPU
99% reduction in X event traffic

--- NVIDIA driver ---

BEFORE:
Application FPS = 3700
Compiz XDamageNotify per sec = 7400
CPU usage = 12%
Compiz frame rate = 60 FPS

AFTER:
Application FPS = 3700
Compiz XDamageNotify per sec = 120
CPU usage = 9-10%
Compiz frame rate = 60 FPS

RESULT:
Frame rate change not noticeable.
10-25% reduction in CPU
99% reduction in X event traffic

[Sam Spilsbury (smspillaz)]

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

I'm happy with the change to using XDamageReportBoundingRects . In
fact, it really shouldn't be an option - BoundingRects will be just
as effecient as RawRectangles when only one rectangle is being
delivered per-paint. And since the rectangles
are per-drawable anyways, the performance penalty in that case
will only arrive if you have a large window with two small
sections that are updating (and even then, redrawing the whole
large window isn't really that expensive)

> Daniel van Vugt has proposed merging lp:~vanvugt/compiz/XDamageReport into lp:compiz.
>
> Requested reviews:
> Compiz Maintainers (compiz-team)
> Related bugs:
> Bug #1007299 in Compiz: "Compiz frame rate decreases if application frame rates are too high (unthrottled)"
> https://bugs.launchpad.net/compiz/+bug/1007299
>
> For more details, see:
> https://code.launchpad.net/~vanvugt/compiz/XDamageReport/+merge/108307
>
> Results!
>
> --- Intel driver ---
>
> BEFORE:
> Application FPS = 5500
> Compiz XDamageNotify per sec = 11000
> CPU usage = 4%
> Compiz frame rate = 30 FPS
>
> AFTER:
> Application FPS = 5500
> Compiz XDamageNotify per sec = 120
> CPU usage = 1-2%
> Compiz frame rate = 60 FPS
>
> RESULT:
> 2x frame rate
> 50-75% reduction in CPU
> 99% reduction in X event traffic
>
> --- NVIDIA driver ---
>
> BEFORE:
> Application FPS = 3700
> Compiz XDamageNotify per sec = 7400
> CPU usage = 12%
> Compiz frame rate = 60 FPS
>
> AFTER:
> Application FPS = 3700
> Compiz XDamageNotify per sec = 120
> CPU usage = 9-10%
> Compiz frame rate = 60 FPS
>
> RESULT:
> Frame rate change not noticeable.
> 10-25% reduction in CPU
> 99% reduction in X event traffic
> --
> https://code.launchpad.net/~vanvugt/compiz/XDamageReport/+merge/108307
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/XDamageReport into lp:compiz.
>

[Tim Penhey (thumper)]

The actual code changes look fine to me.

The two plugin changes at the bottom almost had me recommending having a factory method for Damage. Two uses is my general guideline limit before refactoring :)

review: Approve

[Sam Spilsbury (smspillaz)]

If we are defaulting to bounding box, can we remove the option? It doesn't make sense to expose stuff like this, and we'll be dealing with bug reports when DeltaRectangles involves artefacts.

[Daniel van Vugt (vanvugt)]

I would like to remove the option, because it makes the diff fantastically simple. But I added the option so people could:
  (1) Go back to RawRectangles if there was any regression.
  (2) Experiment with what mode is fastest.
  (3) Most important: Turn on RawRectangles to compare the performance of before and after.

Though they're all short-term requirements. Longer term it makes sense to not be an option.

[Sam Spilsbury (smspillaz)]

> I would like to remove the option, because it makes the diff fantastically
> simple. But I added the option so people could:
> (1) Go back to RawRectangles if there was any regression.
> (2) Experiment with what mode is fastest.
> (3) Most important: Turn on RawRectangles to compare the performance of
> before and after.
>

If there is a regression, we can just revert the branch ;-) In any case, if we want to keep it as an option so we can get feedback, please make the option between BoundingBox and RawRectangles and propose a merge to remove the other one within the next two weeks.

> Though they're all short-term requirements. Longer term it makes sense to not
> be an option.

[Daniel van Vugt (vanvugt)]

I'd rather only merge this stuff once, so will remove the option to change the reporting mode.

[Sam Spilsbury (smspillaz)]

Conditional approval once the option is removed.

review: Approve

[Daniel van Vugt (vanvugt)]

Done.

[Sam Spilsbury (smspillaz)]

review: Approve

[Sam Spilsbury (smspillaz)]

Before merging this - could you explain what the XDamageSubtract is for? It seems like on reading the documentation it is a no-op with None, None, although the documentation is very breif on this matter.

[Daniel van Vugt (vanvugt)]

XDamageSubtract is the most critical part of this proposal :)

It tells the X server that we have redrawn/repaired the regions that were damaged. Without the call the XDamageSubtract, most of the screen never gets redrawn. Because in the more efficient reporting modes, the X server only reports damage events for those regions it hasn't yet told you about. That's why we need to "subtract" all the damaged regions once per frame -- to tell the server we've repaired them and it can now send more events if the same region is damaged again.

"None, None" tells the server we have repaired the entire drawable (so the first None means infinite-region), and don't care to keep a copy of what the old damage region was (the second None).

[http://cgit.freedesktop.org/xorg/proto/damageproto/plain/damageproto.txt]

[Sam Spilsbury (smspillaz)]

Great, thanks. (And we're using std::set so there's no dupe calls). Approved.

review: Approve

[Sam Spilsbury (smspillaz)]

With regards to the potential race condition - I had a look at mutter and kwin, and it seems like the race condition is that further damage may be accumulated on the server before we call glXBindTexImage, for example:

    if (damaged && pixmap)
    {
        XGrabServer (screen->dpy ());
        XDamageSubtract (screen->dpy (), damage, None, None);
        XSync (screen->dpy (), false);
        XUngrabServer (screen->dpy ());
        XSync (screen->dpy (), false);
 (*GL::releaseTexImage) (screen->dpy (), pixmap, GLX_FRONT_LEFT_EXT);
 (*GL::bindTexImage) (screen->dpy (), pixmap, GLX_FRONT_LEFT_EXT, NULL);
    }

I'm not able to test it, but it seems to me that you have to flush all drawing to the kernel before calling glXBindTexImage.

(Also, we should remove the damage event tracking on both the composite and decor plugins and just make opengl track the damage. There isn't much sense in tracking it multiple times, and that probably causes more issues)

[Daniel van Vugt (vanvugt)]

This completed proposal is not the correct place for this discussion ^. But thanks, I suspected something like that. Unfortunately it's not a priority to revisit for a while.

=== modified file 'plugins/composite/src/privates.h'
--- plugins/composite/src/privates.h	2012-05-25 03:54:23 +0000
+++ plugins/composite/src/privates.h	2012-06-05 07:54:18 +0000
@@ -30,6 +30,7 @@
 
 #include <composite/composite.h>
 #include <core/atoms.h>
+#include <set>
 
 #include "composite_options.h"
 
@@ -106,6 +107,8 @@
 
 	Atom cmSnAtom;
 	Window newCmSnOwner;
+
+	std::set<Damage> damages;
 };
 
 class PrivateCompositeWindow : WindowInterface

=== modified file 'plugins/composite/src/screen.cpp'
--- plugins/composite/src/screen.cpp	2012-03-27 12:14:56 +0000
+++ plugins/composite/src/screen.cpp	2012-06-05 07:54:18 +0000
@@ -98,6 +98,11 @@
 		    }
 		}
 	    }
+	    else if (event->type == damageEvent + XDamageNotify)
+	    {
+		XDamageNotifyEvent *de = (XDamageNotifyEvent*)event;
+		damages.insert (de->damage);
+	    }
 	    break;
     }
 
@@ -791,6 +796,10 @@
 		damageScreen ();
 	}
 
+	foreach (Damage d, priv->damages)
+	    XDamageSubtract (screen->dpy(), d, None, None);
+	priv->damages.clear ();
+
 	priv->damage = CompRegion ();
 
 	int mask = priv->damageMask;

=== modified file 'plugins/composite/src/window.cpp'
--- plugins/composite/src/window.cpp	2012-05-25 03:54:23 +0000
+++ plugins/composite/src/window.cpp	2012-06-05 07:54:18 +0000
@@ -36,7 +36,7 @@
     if (w->windowClass () != InputOnly)
     {
 	priv->damage = XDamageCreate (s->dpy (), w->id (),
-				      XDamageReportRawRectangles);
+	                              XDamageReportBoundingBox);
     }
     else
     {

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-05-27 04:32:55 +0000
+++ plugins/decor/src/decor.cpp	2012-06-05 07:54:18 +0000
@@ -326,7 +326,7 @@
 	textures[0]->setMipmap (false);
 
     damage = XDamageCreate (screen->dpy (), pixmap->getPixmap (),
-			     XDamageReportRawRectangles);
+                            XDamageReportBoundingBox);
 }
 
 /*

=== modified file 'plugins/opengl/src/texture.cpp'
--- plugins/opengl/src/texture.cpp	2012-01-19 18:12:31 +0000
+++ plugins/opengl/src/texture.cpp	2012-06-05 07:54:18 +0000
@@ -581,7 +581,7 @@
     glBindTexture (texTarget, 0);
 
     tex->damage = XDamageCreate (screen->dpy (), pixmap,
-			         XDamageReportRawRectangles);
+                                 XDamageReportBoundingBox);
     boundPixmapTex[tex->damage] = tex;
 
     return rv;