Merge lp:~vanvugt/compiz-core/fix-940139 into lp:compiz-core

[Daniel van Vugt]

DOUBLED the CPU performance of compiz. Previously compiz was spending about
51% of its time constructing and destructing CompRegions. This proposal
reduces that figure to about 4.4%, resulting in a net reduction in compiz
CPU usage of almost 47%. And it is noticeable too. (LP: #940139)

[Sam Spilsbury (smspillaz)]

Good.

Two things I don't get here.

#if REGION_IS_A_POINTER

Not quite sure I understand why we need such ifdefs. Unless its for debugging, in which case I'd make it a little more clear thats the case.

26 +class CompRegionRef : public CompRegion
27 +{
28 + public:
29 + explicit CompRegionRef (Region);
30 + ~CompRegionRef ();
31 };

Is there any reason why you can't add a constructor for CompRegion using XRegion and ditto for const CompRect & ?

[Daniel van Vugt (vanvugt)]

Yeah, so much code assumes Region is a pointer already that we could easily delete all the #else clauses and not test for REGION_IS_A_POINTER at all. That would also remove the last remaining references to PrivateRegion so privateregion.h would then need to be removed etc. I only left REGION_IS_A_POINTER in there to show explicitly what changed, and to show anyone trying to port the code to a non-X environment later what they had to do.

I did add a constructor for CompRegion using Region (XRegion) already on line 21. The reason why it has to be protected and CompRegionRef a separate class is because "priv" no longer has room for a flag to indicate whether the CompRegion owns priv. So instead of squeezing a bool into CompRegion and breaking the ABI, I figured the new class CompRegionRef was an adequately clean solution.

[Sam Spilsbury (smspillaz)]

On Mon, 27 Feb 2012, Daniel van Vugt wrote:

> Yeah, so much code assumes Region is a pointer already that we could easily delete all the #else clauses and not test for REGION_IS_A_POINTER at all. That would also remove the last remaining references to PrivateRegion so privateregion.h would then need to be removed etc. I only left REGION_IS_A_POINTER in there to show explicitly what changed, and to show anyone trying to port the code to a non-X environment later what they had to do.
>

+1 , although X regions are all client side anyways, so thats not even a
problem.

> I did add a constructor for CompRegion using Region (XRegion) already on line 21. The reason why it has to be protected and CompRegionRef a separate class is because "priv" no longer has room for a flag to indicate whether the CompRegion owns priv. So instead of squeezing a bool into CompRegion and breaking the ABI, I figured the new class CompRegionRef was an adequately clean solution.

Ack.

I understand that you wanted to remove the pimpl and make CompRegion a
thin wrapper, although I wonder what the performance implication of having
the indirection through the pimpl there was. Surely if the pimpl was still
there a cleaner implementation would be to store the region pointer and a
refcount, no ?

I guess the reason for that though, now that I think of it is the
difficulty in expressing in C++ syntax whether or not the wrapper actually
"owns" the underlying resource or not.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-940139/+merge/94715
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-940139 into lp:compiz-core.
>

[Daniel van Vugt (vanvugt)]

You mean +1 to remove the #if's or should we keep them?

The performance implication is huge. Compiz was spending most of its time in malloc/free, which was mostly due to "new PrivateRegion" and "delete PrivateRegion". So PrivateRegion had to go. Although after this fix most of compiz' CPU usage is still due to excessive malloc/free/new/deletes, at least it's no longer mostly due to CompRegion.

[Sam Spilsbury (smspillaz)]

On Mon, 27 Feb 2012, Daniel van Vugt wrote:

> You mean +1 to remove the #if's or should we keep them?
>

Remove.

> The performance implication is huge. Compiz was spending most of its time in malloc/free, which was mostly due to "new PrivateRegion" and "delete PrivateRegion". So PrivateRegion had to go. Although after this fix most of compiz' CPU usage is still due to excessive malloc/free/new/deletes, at least it's no longer mostly due to CompRegion.
>

Ok, I can buy the malloc/free argument. The semantics behind CompRegionRef
still feel strange to me though (can't have more than one reference, this
is a thing, or this is a pointer to a thing etc). Have you looked into
what performance benefits could be gained with boost memory pools? It
should be possible to have a static memory pool private to CompRegion
something like this:
http://www.niksula.hut.fi/~hkankaan/Homepages/fastallocation.html

>
> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-940139/+merge/94715
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-940139 into lp:compiz-core.
>

[Daniel van Vugt (vanvugt)]

I will remove the #ifs.

CompRegionRef is necessary because where it is used the overhead of constructing a temporary CompRegion (which was only a copy of a CompRect's region) was very large. At least half of the performance gains are due to CompRegionRef. And I think its design makes a lot of sense -- it temporarily gives you a CompRegion interface to an existing Region.

I did think of many such memory pooling/caching solutions. However what you see below is much faster and much smaller. Boost is not required, and should be used sparingly anyway. Boost was the largest cause of code bloat last time I analyzed the binaries. And that's a problem I don't want to make worse.

[lp:~vanvugt/compiz-core/fix-940139]

3024. By Daniel van Vugt on 2012-02-27

Remove #if's that will probably never be used. Also remove subsequently
dead code.

[Sam Spilsbury (smspillaz)]

OK, that makes sense then.

One small thing, in some places you are using 8-spaces instead of 8-wide tabs for even indentation levels, please use 4-spaces, tab, tab-4spaces, tab-tab etc for indentation.

review: Approve

Unmerged revisions

=== modified file 'include/core/region.h'
--- include/core/region.h	2012-02-15 17:45:25 +0000
+++ include/core/region.h	2012-02-27 08:10:23 +0000
@@ -32,8 +32,6 @@
 #include <core/rect.h>
 #include <core/point.h>
 
-class PrivateRegion;
-
 /**
  * A 2D region with an (x,y) position and arbitrary dimensions similar to
  * an XRegion. It's data membmers are private and  must be manipulated with
@@ -180,8 +178,18 @@
 	const CompRegion operator| (const CompRegion &) const;
 	CompRegion & operator|= (const CompRegion &);
 
-    private:
-	PrivateRegion *priv;
+    protected:
+	/* Construct a CompRegion based on an externally managed Region */
+	explicit CompRegion (Region);
+	void init ();
+	void *priv;
+};
+
+class CompRegionRef : public CompRegion
+{
+    public:
+	explicit CompRegionRef (Region);
+	~CompRegionRef ();
 };
 
 extern const CompRegion infiniteRegion;

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-02-24 08:22:28 +0000
+++ plugins/decor/src/decor.cpp	2012-02-27 08:10:23 +0000
@@ -107,8 +107,6 @@
 
         for (it--; it != screen->windows ().end (); it--)
         {
-            CompRegion inter;
-
             if (!(*it)->isViewable ())
                 continue;
 
@@ -121,10 +119,11 @@
             if (!isAncestorTo (window, (*it)))
                 continue;
 
-	    inter = shadowRegion.intersected ((*it)->borderRect ());
+            CompRegion inter (shadowRegion);
+            inter &= (*it)->borderRect ();
 
             if (!inter.isEmpty ())
-		shadowRegion = shadowRegion.subtracted (inter);
+                shadowRegion -= inter;
         }
 
         /* If the region didn't change, then it is safe to
@@ -140,7 +139,7 @@
          * that will look a lot better.
          */
         if (window->type () == CompWindowTypeDropdownMenuMask &&
-	    shadowRegion == CompRegion (window->outputRect ()))
+	    shadowRegion == CompRegionRef (window->outputRect ().region ()))
         {
             CompRect area (window->outputRect ().x1 (),
                            window->outputRect ().y1 (),
@@ -148,7 +147,7 @@
 			   window->inputRect ().y1 () -
                            window->outputRect ().y1 ());
 
-	    shadowRegion = shadowRegion.subtracted (area);
+	    shadowRegion -= area;
         }
     }
 }
@@ -247,17 +246,25 @@
 		         const CompRegion   &region,
 		         unsigned int       mask)
 {
-    CompRegion reg = shadowRegion.intersected (region);
+    const CompRegion *preg = NULL;
 
     if ((mask & (PAINT_WINDOW_ON_TRANSFORMED_SCREEN_MASK |
 		 PAINT_WINDOW_WITH_OFFSET_MASK)))
-	reg = region;
+	preg = &region;
     else if (mask & PAINT_WINDOW_TRANSFORMED_MASK)
-	reg = infiniteRegion;
+	preg = &infiniteRegion;
+    else
+    {
+	tmpRegion = shadowRegion;
+	tmpRegion &= region;
+	preg = &tmpRegion;
+    }
 
     /* In case some plugin needs to paint us with an offset region */
-    if (reg.isEmpty ())
-	reg = region;
+    if (preg->isEmpty ())
+	preg = &region;
+
+    const CompRegion &reg (*preg);
 
     if (wd &&
 	wd->decor->type == WINDOW_DECORATION_TYPE_PIXMAP)
@@ -278,7 +285,8 @@
 	    if (box.width () > 0 && box.height () > 0)
 	    {
 		ml[0] = wd->quad[i].matrix;
-		gWindow->glAddGeometry (ml, CompRegion (box), reg);
+		const CompRegionRef boxRegion (box.region ());
+		gWindow->glAddGeometry (ml, boxRegion, reg);
 	    }
 	}
 

=== modified file 'plugins/decor/src/decor.h'
--- plugins/decor/src/decor.h	2012-02-09 07:48:57 +0000
+++ plugins/decor/src/decor.h	2012-02-27 08:10:23 +0000
@@ -272,6 +272,7 @@
 
 	CompRegion frameRegion;
 	CompRegion shadowRegion;
+	CompRegion tmpRegion;
 
 	Window inputFrame;
 	Window outputFrame;

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp	2012-01-30 05:12:24 +0000
+++ plugins/opengl/src/paint.cpp	2012-02-27 08:10:23 +0000
@@ -1179,8 +1179,8 @@
     WRAPABLE_HND_FUNCTN_RETURN (bool, glDraw, transform,
 			      fragment, region, mask)
 
-    const CompRegion reg = (mask & PAINT_WINDOW_TRANSFORMED_MASK) ?
-	                   infiniteRegion : region;
+    const CompRegion &reg = (mask & PAINT_WINDOW_TRANSFORMED_MASK) ?
+                            infiniteRegion : region;
 
     if (reg.isEmpty ())
 	return true;

=== removed file 'src/privateregion.h'
--- src/privateregion.h	2010-12-04 02:41:35 +0000
+++ src/privateregion.h	1970-01-01 00:00:00 +0000
@@ -1,41 +0,0 @@
-/*
- * Copyright © 2008 Dennis Kasprzyk
- *
- * Permission to use, copy, modify, distribute, and sell this software
- * and its documentation for any purpose is hereby granted without
- * fee, provided that the above copyright notice appear in all copies
- * and that both that copyright notice and this permission notice
- * appear in supporting documentation, and that the name of
- * Dennis Kasprzyk not be used in advertising or publicity pertaining to
- * distribution of the software without specific, written prior permission.
- * Dennis Kasprzyk makes no representations about the suitability of this
- * software for any purpose. It is provided "as is" without express or
- * implied warranty.
- *
- * DENNIS KASPRZYK DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
- * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
- * NO EVENT SHALL DENNIS KASPRZYK BE LIABLE FOR ANY SPECIAL, INDIRECT OR
- * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
- * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
- * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
- * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
- *
- * Authors: Dennis Kasprzyk <[e-mail]>
- */
-
-#ifndef _PRIVATEREGION_H
-#define _PRIVATEREGION_H
-
-#include <core/rect.h>
-#include <core/region.h>
-
-class PrivateRegion {
-    public:
-	PrivateRegion ();
-	~PrivateRegion ();
-
-    public:
-	Region region;
-};
-
-#endif

=== modified file 'src/region.cpp'
--- src/region.cpp	2012-02-15 17:45:25 +0000
+++ src/region.cpp	2012-02-27 08:10:23 +0000
@@ -23,13 +23,15 @@
  * Authors: Dennis Kasprzyk <[e-mail]>
  */
 
-#include "privateregion.h"
+#include "core/rect.h"
+#include "core/region.h"
 
 #include <X11/Xlib-xcb.h>
 #include <X11/Xutil.h>
 #include <X11/Xregion.h>
 
 #include <stdio.h>
+#include <cassert>
 
 #include <boost/foreach.hpp>
 #define foreach BOOST_FOREACH
@@ -38,20 +40,29 @@
 				           MAXSHORT * 2, MAXSHORT * 2));
 const CompRegion emptyRegion;
 
+/* Alternate emptyRegion, mostly just required to construct infiniteRegion
+   when emptyRegion may still be uninitialized */
+static const CompRegion &
+_emptyRegion ()
+{
+    static const CompRegion r;
+    return r;
+}
+
 CompRegion::CompRegion ()
 {
-    priv = new PrivateRegion ();
+    init ();
 }
 
 CompRegion::CompRegion (const CompRegion &c)
 {
-    priv = new PrivateRegion ();
-    XUnionRegion (CompRegion ().handle (), c.priv->region, priv->region);
+    init ();
+    XUnionRegion (_emptyRegion ().handle (), c.handle (), handle ());
 }
 
 CompRegion::CompRegion ( int x, int y, int w, int h)
 {
-    priv = new PrivateRegion ();
+    init ();
     
     XRectangle rect;
     
@@ -60,14 +71,12 @@
     rect.width = w;
     rect.height = h;
     
-    Region tmp = XCreateRegion ();
-    XUnionRectWithRegion (&rect, tmp, priv->region);
-    XDestroyRegion (tmp);
+    XUnionRectWithRegion (&rect, _emptyRegion ().handle (), handle ());
 }
 
 CompRegion::CompRegion (const CompRect &r)
 {
-    priv = new PrivateRegion ();
+    init ();
     
     XRectangle rect;
     
@@ -76,26 +85,49 @@
     rect.width = r.width ();
     rect.height = r.height ();
     
-    Region tmp = XCreateRegion ();
-    XUnionRectWithRegion (&rect, tmp, priv->region);
-    XDestroyRegion (tmp);
+    XUnionRectWithRegion (&rect, _emptyRegion ().handle (), handle ());
+}
+
+CompRegion::CompRegion (Region external)
+{
+    priv = external;
+}
+
+CompRegionRef::CompRegionRef (Region external) :
+    CompRegion (external)
+{
+}
+
+CompRegionRef::~CompRegionRef ()
+{
+    /* Ensure CompRegion::~CompRegion does not destroy the region, because
+       it's external and we don't own it. */
+    priv = NULL;
 }
 
 CompRegion::~CompRegion ()
 {
-    delete priv;
+    if (priv)
+        XDestroyRegion (static_cast<Region> (priv));
+}
+
+void
+CompRegion::init ()
+{
+    assert (sizeof (Region) == sizeof (void*));
+    priv = XCreateRegion ();
 }
 
 Region
 CompRegion::handle () const
 {
-    return priv->region;
+    return static_cast<Region> (priv);
 }
 
 CompRegion &
 CompRegion::operator= (const CompRegion &c)
 {
-    XUnionRegion (CompRegion ().handle (), c.priv->region, priv->region);
+    XUnionRegion (emptyRegion.handle (), c.handle (), handle ());
     return *this;
 }
 
@@ -195,9 +227,9 @@
 	return rv;
 
     BOX b;
-    for (int i = 0; i < priv->region->numRects; i++)
+    for (int i = 0; i < handle ()->numRects; i++)
     {
-	b = priv->region->rects[i];
+	b = handle ()->rects[i];
 	rv.push_back (CompRect (b.x1, b.y1, b.x2 - b.x1, b.y2 - b.y1));
     }
     return rv;
@@ -314,13 +346,16 @@
 CompRegion &
 CompRegion::operator&= (const CompRegion &r)
 {
-    return *this = *this & r;
+    XIntersectRegion (r.handle (), handle (), handle ());
+    return *this;
 }
 
 CompRegion &
 CompRegion::operator&= (const CompRect &r)
 {
-    return *this = *this & r;
+    CompRegionRef ref (r.region ());
+    XIntersectRegion (ref.handle (), handle (), handle ());
+    return *this;
 }
 
 const CompRegion
@@ -338,13 +373,16 @@
 CompRegion &
 CompRegion::operator+= (const CompRegion &r)
 {
-    return *this = *this + r;
+    XUnionRegion (handle (), r.handle (), handle ());
+    return *this;
 }
 
 CompRegion &
 CompRegion::operator+= (const CompRect &r)
 {
-    return *this = *this + r;
+    CompRegionRef ref (r.region ());
+    XUnionRegion (handle (), ref.handle (), handle ());
+    return *this;
 }
 
 const CompRegion
@@ -362,13 +400,16 @@
 CompRegion &
 CompRegion::operator-= (const CompRegion &r)
 {
-    return *this = *this - r;
+    XSubtractRegion (handle (), r.handle (), handle ());
+    return *this;
 }
 
 CompRegion &
 CompRegion::operator-= (const CompRect &r)
 {
-    return *this = *this - r;
+    CompRegionRef ref (r.region ());
+    XSubtractRegion (handle (), ref.handle (), handle ());
+    return *this;
 }
 
 const CompRegion
@@ -380,7 +421,8 @@
 CompRegion &
 CompRegion::operator^= (const CompRegion &r)
 {
-    return *this = *this ^ r;
+    XXorRegion (handle (), r.handle (), handle ());
+    return *this;
 }
 
 const CompRegion
@@ -392,16 +434,7 @@
 CompRegion &
 CompRegion::operator|= (const CompRegion &r)
 {
-    return *this = *this | r;
-}
-
-
-PrivateRegion::PrivateRegion ()
-{
-    region = XCreateRegion ();
-}
-
-PrivateRegion::~PrivateRegion ()
-{
-   XDestroyRegion (region);
-}
+    XUnionRegion (handle (), r.handle (), handle ());
+    return *this;
+}
+