Merge lp:~smspillaz/compiz-core/compiz-core.optimization-inlining into lp:compiz-core

[Sam Spilsbury]

Optimize and inline some hot-spots in CompWindow.

server*Rect and *Rect were being called quite a lot and unnecessarily being recalculated and copy-constructed (eg XCreateRegion madness). This branch inlines some functions that were being called a ton (compiz::window::Geometry::border () about 600,000 times) and adds a new RecalcRect class. RecalcRect takes a function object to recalculate a CompRect. At the moment, we use the old behaviour of recalculating server*Rect and *Rect every time they are called but once https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.work_923683 is merged then we can optimize there.

[Alan Griffiths (alan-griffiths)]

Not really relevant to the change, but as "Region handle () const" compiles there's already a declaration of Region visible - so why is priv a "void*" and cast all the while?

review: Approve

[Daniel van Vugt (vanvugt)]

priv is a void* because in the penultimate revision before that change was merged, priv could represent different types (Region or PrivateRegion). Now that PrivateRegion is gone however you are correct that priv could be simply "Region priv".

[Daniel van Vugt (vanvugt)]

I haven't tested it yet, but I'm pretty sure this branch should contain a CORE_ABIVERSION bump.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> I haven't tested it yet, but I'm pretty sure this branch should contain a
> CORE_ABIVERSION bump.

Good catch

[Daniel van Vugt (vanvugt)]

8 - Region handle () const;
9 + Region handle () const { return static_cast<Region> (priv); }

I noticed also that CompRegion::handle is the most commonly called function in compiz. However it is also tiny and only accounts for 1% of compiz CPU usage. Which is why I didn't touch it.

Another problem with changing handle() is that if we change the implementation of CompRegion in future then we will need to change handle() again and that would be another ABI bump.

I think a better solution to solve all of these problems is to keep handle() unchanged, and add a new method:
    private:
        Region privHandle () const { return static_cast<Region> (priv); }
for use in region.cpp only.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Or simply:
    #define REGIONHANDLE (static_cast<Region> (priv))
in region.cpp

[Daniel van Vugt (vanvugt)]

Also, testing this code I notice another windows-shifted-horizontally regression (though I suspect it's the same one and we haven't fixed the root cause yet). When a window is maximized or restored, it's contents are shifted to the right/left of where they should be. When you move the window, the shift corrects itself.

:(

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Though it's possible the shift regression would be fixed by merging the other branch Sam mentions...

[Daniel van Vugt (vanvugt)]

Confirmed: The shift regression seen with this branch is fixed if you also merge:
lp:~smspillaz/compiz-core/compiz-core.work_923683
But that branch has its own problems.

[Sam Spilsbury (smspillaz)]

> 8 - Region handle () const;
> 9 + Region handle () const { return static_cast<Region> (priv); }
>
> I noticed also that CompRegion::handle is the most commonly called function in
> compiz. However it is also tiny and only accounts for 1% of compiz CPU usage.
> Which is why I didn't touch it.
>
> Another problem with changing handle() is that if we change the implementation
> of CompRegion in future then we will need to change handle() again and that
> would be another ABI bump.

We change the ABI for other reasons too, and I really don't think the implementation of CompRegion is going to change (except, perhaps to cairo_region_t), so this is just a total non-issue.

>
> I think a better solution to solve all of these problems is to keep handle()
> unchanged, and add a new method:
> private:
> Region privHandle () const { return static_cast<Region> (priv); }
> for use in region.cpp only.

[Daniel van Vugt (vanvugt)]

CompRegion::handle()
I think this may be worth inlining privately. As mentioned earlier. We shouldn't make handle() inline for external users of the class. FYI, CompRegion::handle() is consistently the single most called function in compiz, however it never accounts for more than 1.3% of the total CPU usage.

compiz::window::Geometry::border ()
Makes no sense to inline AFAICS. It may well be around the 25th most called function, however it only accounts for 0.07% of the CPU usage.

CompWindow::borderRect()
Does not need to optimizing at all. Is almost never called and contributes absolutely nothing to the total CPU usage.

*Rect()
Contribute almost nothing measurable to the CPU usage.

All-in-all, these optimizations don't seem to provide any significant benefit. I think CompRegion::handle() could be optimized using a private inline version of that function. But even then, the maximum gain is going to be about 1%. The rest of the changes break the ABI, cause a regression, and do not appear to be required at all.

If I'm wrong, then please log a bug with the profile data showing why all this existing code needs optimizing.

review: Disapprove

[Sam Spilsbury (smspillaz)]

On Mon, 5 Mar 2012, Daniel van Vugt wrote:

> Review: Disapprove
>
> CompRegion::handle()
> I think this may be worth inlining privately. As mentioned earlier. We shouldn't make handle() inline for external users of the class. FYI, CompRegion::handle() is consistently the single most called function in compiz, however it never accounts for more than 1.3% of the total CPU usage.
>

Ack, although other plugins use the handle directly.

> compiz::window::Geometry::border ()
> Makes no sense to inline AFAICS. It may well be around the 25th most called function, however it only accounts for 0.07% of the CPU usage.
>

Why not ? Avoids the JMP

> CompWindow::borderRect()
> Does not need to optimizing at all. Is almost never called and contributes absolutely nothing to the total CPU usage.
>

> *Rect()
> Contribute almost nothing measurable to the CPU usage.
>

They account for about 5% ish of usage, and are called during paint
functions. There is no reason for them to operate in the way that they
currently do. The constructors also involve XCreateRegion and
XDestroyRegion pairs.

Just because the optimization isn't "significant" that doesn't mean that
it is useless. In fact, the code as it was written makes no sense
whatsoever, and there isn't any reason to keep it as it is.

> All-in-all, these optimizations don't seem to provide any significant benefit. I think CompRegion::handle() could be optimized using a private inline version of that function. But even then, the maximum gain is going to be about 1%. The rest of the changes break the ABI, cause a regression, and do not appear to be required at all.
>

The regression is unrelated, see
lp:~smspillaz/compiz-core/compiz-core.backport_2988_work_923683

> If I'm wrong, then please log a bug with the profile data showing why all this existing code needs optimizing.
> --
> https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.optimization-inlining/+merge/95094
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.optimization-inlining.
>

[Daniel van Vugt (vanvugt)]

Still causes the shift regression, despite us thinking it was fixed by:
https://code.launchpad.net/+branch/~smspillaz/compiz-core/compiz-core.backport_2988_work_923683
It seems that fix has not worked.

Aside from the shift regression which still needs fixing, I strongly recommend not breaking the ABI (now and again in future) by inlining handle() or border(). If anything, I suggest adding new inline functions "inlineHandle" and "inlineBorder". Ideally they should be private. If not private, then they should be documented as "only safe to use in core or core plugins".

CORE_ABIVERSION still needs bumping too.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

And yes, when I say "causes the shift regression" I actually mean "triggers the shift regression". I'm pretty sure the bug is not in the below diff, only exploited by it.

Unmerged revisions

3027. By Sam Spilsbury on 2012-02-29

Added RecalcRect, a class which has a function to recalculate a rectangle on demand,
currently replaces *Rect () in window and allows them to be const &, soon we will be
able to only recalculate these when necessary and not all the time

3026. By Sam Spilsbury on 2012-02-28

Inline some heavily called CompRegion and compiz::window::Geometry types

=== modified file 'include/core/region.h'
--- include/core/region.h	2012-02-27 03:41:13 +0000
+++ include/core/region.h	2012-02-29 03:40:25 +0000
@@ -71,7 +71,7 @@
 	/**
 	 * Returns the internal XRegion handle
 	 */
-	Region handle () const;
+	Region handle () const { return static_cast<Region> (priv); }
 
 	/**
 	 * Returns true if the specified CompPoint falls within the
 
=== modified file 'include/core/window.h'
--- include/core/window.h	2012-02-16 09:55:06 +0000
+++ include/core/window.h	2012-02-29 03:40:25 +0000
@@ -317,19 +317,23 @@
 	/* With border */
 	int serverWidth () const;
 	int serverHeight () const;
-	const CompSize serverSize () const;
+	CompSize serverSize () const;
 
 	/* effective decoration extents */
-	CompRect borderRect () const;
-	CompRect serverBorderRect () const;
+	const CompRect & borderRect () const;
+	const CompRect & serverBorderRect () const;
 
 	/* frame window geometry */
-	CompRect inputRect () const;
-	CompRect serverInputRect () const;
+	const CompRect & inputRect () const;
+	const CompRect & serverInputRect () const;
 
 	/* includes decorations and shadows */
-	CompRect outputRect () const;
-	CompRect serverOutputRect () const;
+	const CompRect & outputRect () const;
+	const CompRect & serverOutputRect () const;
+
+	/* only includes X11 borders */
+	const CompRect & geometryBorders () const;
+	const CompRect & serverGeometryBorders () const;
 
 	Window id ();
 	Window frame ();
 
=== modified file 'src/privatewindow.h'
--- src/privatewindow.h	2012-02-14 14:37:04 +0000
+++ src/privatewindow.h	2012-02-29 03:40:25 +0000
@@ -209,6 +209,13 @@
 
 	bool checkClear ();
 
+	void recalcExtentsRect (CompRect &rect,
+				const CompWindow::Geometry *geometry,
+				const CompWindowExtents *extents);
+
+	void recalcGeometryBordersRect (CompRect &rect,
+				        const CompWindow::Geometry *geometry);
+
     public:
 
 	PrivateWindow *priv;
@@ -251,6 +258,15 @@
 	CompRegion inputRegion;
 	CompRegion frameRegion;
 
+	compiz::rect::RecalcRect borderRect;
+	compiz::rect::RecalcRect serverBorderRect;
+	compiz::rect::RecalcRect inputRect;
+	compiz::rect::RecalcRect serverInputRect;
+	compiz::rect::RecalcRect outputRect;
+	compiz::rect::RecalcRect serverOutputRect;
+	compiz::rect::RecalcRect geometryBorders;
+	compiz::rect::RecalcRect serverGeometryBorders;
+
 	unsigned int wmType;
 	unsigned int type;
 	unsigned int state;
 
=== modified file 'src/rect/include/core/rect.h'
--- src/rect/include/core/rect.h	2012-01-19 19:32:29 +0000
+++ src/rect/include/core/rect.h	2012-02-29 03:40:25 +0000
@@ -33,6 +33,8 @@
 #include <X11/Xlib.h>
 #include <X11/Xutil.h>
 #include <X11/Xregion.h>
+#include <boost/function.hpp>
+#include <boost/bind.hpp>
 #include "core/point.h"
 
 
@@ -144,6 +146,23 @@
     {
 	CompPoint wraparoundPoint (const CompRect &bounds,
 				   const CompPoint &p);
+
+	class RecalcRect
+	{
+	    public:
+
+		typedef boost::function <void (CompRect &rect)> RecalcFunc;
+
+		RecalcRect (const RecalcFunc &func);
+
+		const CompRect & rect () const { return mRect; }
+		void refresh () { mRecalcFunc (mRect); }
+
+	    private:
+
+		CompRect mRect;
+		RecalcFunc mRecalcFunc;
+	};
     }
 }
 
 
=== modified file 'src/region.cpp'
--- src/region.cpp	2012-02-27 08:05:51 +0000
+++ src/region.cpp	2012-02-29 03:40:25 +0000
@@ -118,12 +118,6 @@
     priv = XCreateRegion ();
 }
 
-Region
-CompRegion::handle () const
-{
-    return static_cast<Region> (priv);
-}
-
 CompRegion &
 CompRegion::operator= (const CompRegion &c)
 {
 
=== modified file 'src/window.cpp'
--- src/window.cpp	2012-02-20 02:30:34 +0000
+++ src/window.cpp	2012-02-29 03:40:25 +0000
@@ -6589,6 +6589,15 @@
     width (0),
     height (0),
     region (),
+    frameRegion (),
+    borderRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &border)),
+    serverBorderRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &border)),
+    inputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &input)),
+    serverInputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &serverInput)),
+    outputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &output)),
+    serverOutputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &output)),
+    geometryBorders (boost::bind (&PrivateWindow::recalcGeometryBordersRect, this, _1, &geometry)),
+    serverGeometryBorders (boost::bind (&PrivateWindow::recalcGeometryBordersRect, this, _1, &serverGeometry)),
     wmType (0),
     type (CompWindowTypeUnknownMask),
     state (0),
 
=== modified file 'src/window/geometry/include/core/windowgeometry.h'
--- src/window/geometry/include/core/windowgeometry.h	2012-01-19 20:08:32 +0000
+++ src/window/geometry/include/core/windowgeometry.h	2012-02-29 03:40:25 +0000
@@ -55,7 +55,7 @@
     Geometry ();
     Geometry (int x, int y, int width, int height, int border);
 
-    int border () const;
+    int border () const { return mBorder; }
 
     void set (int x, int y, int width, int height, int border);
     void setBorder (int border);
 
=== modified file 'src/window/geometry/src/windowgeometry.cpp'
--- src/window/geometry/src/windowgeometry.cpp	2012-01-19 20:08:32 +0000
+++ src/window/geometry/src/windowgeometry.cpp	2012-02-29 03:40:25 +0000
@@ -41,12 +41,6 @@
 {
 }
 
-int
-compiz::window::Geometry::border () const
-{
-    return mBorder;
-}
-
 void
 compiz::window::Geometry::setBorder (int border)
 {
 
=== modified file 'src/windowgeometry.cpp'
--- src/windowgeometry.cpp	2012-01-23 05:44:19 +0000
+++ src/windowgeometry.cpp	2012-02-29 03:40:25 +0000
@@ -113,7 +113,7 @@
 	   2 * priv->serverGeometry.border ();
 }
 
-const CompSize
+CompSize
 CompWindow::serverSize () const
 {
     return CompSize (priv->serverGeometry.width () +
@@ -122,68 +122,83 @@
 		     2 * priv->serverGeometry.border ());
 }
 
-CompRect
+void
+PrivateWindow::recalcExtentsRect (CompRect &rect, const CompWindow::Geometry *geometry, const CompWindowExtents *extents)
+{
+    rect.setGeometry (geometry->x () - geometry->border () - extents->left,
+		      geometry->y () - geometry->border () - extents->top,
+		      geometry->width () + geometry->border () * 2 +
+		      extents->left + extents->right,
+		      geometry->height () + geometry->border () * 2 +
+		      extents->top + extents->bottom);
+}
+
+void
+PrivateWindow::recalcGeometryBordersRect (CompRect &rect, const CompWindow::Geometry *geometry)
+{
+    rect.setGeometry (geometry->x () - geometry->border (),
+		      geometry->y () - geometry->border (),
+		      geometry->width () + geometry->border () * 2,
+		      geometry->height () + geometry->border () * 2);
+}
+
+compiz::rect::RecalcRect::RecalcRect (const RecalcFunc &func) :
+    mRecalcFunc (func)
+{
+}
+
+const CompRect &
 CompWindow::borderRect () const
 {
-    return CompRect (priv->geometry.x () - priv->geometry.border () - priv->border.left,
-		     priv->geometry.y () - priv->geometry.border () - priv->border.top,
-		     priv->geometry.width () + priv->geometry.border () * 2 +
-		     priv->border.left + priv->border.right,
-		     priv->geometry.height () + priv->geometry.border () * 2 +
-		     priv->border.top + priv->border.bottom);
-}
-
-CompRect
+    priv->borderRect.refresh ();
+    return priv->borderRect.rect ();
+}
+
+const CompRect &
+CompWindow::inputRect () const
+{
+    priv->inputRect.refresh ();
+    return priv->inputRect.rect ();
+}
+
+const CompRect &
+CompWindow::outputRect () const
+{
+    priv->outputRect.refresh ();
+    return priv->outputRect.rect ();
+}
+
+const CompRect &
 CompWindow::serverBorderRect () const
 {
-    return CompRect (priv->serverGeometry.x () - priv->geometry.border () - priv->border.left,
-		     priv->serverGeometry.y () - priv->geometry.border () - priv->border.top,
-		     priv->serverGeometry.width () + priv->geometry.border () * 2 +
-		     priv->border.left + priv->border.right,
-		     priv->serverGeometry.height () + priv->geometry.border () * 2 +
-		     priv->border.top + priv->border.bottom);
-}
-
-CompRect
-CompWindow::inputRect () const
-{
-    return CompRect (priv->geometry.x () - priv->geometry.border () - priv->serverInput.left,
-		     priv->geometry.y () - priv->geometry.border () - priv->serverInput.top,
-		     priv->geometry.width () + priv->geometry.border () * 2 +
-		     priv->serverInput.left + priv->serverInput.right,
-		     priv->geometry.height () +priv->geometry.border () * 2 +
-		     priv->serverInput.top + priv->serverInput.bottom);
-}
-
-CompRect
+    priv->serverBorderRect.refresh ();
+    return priv->serverBorderRect.rect ();
+}
+
+const CompRect &
 CompWindow::serverInputRect () const
 {
-    return CompRect (priv->serverGeometry.x () - priv->serverGeometry.border () - priv->serverInput.left,
-		     priv->serverGeometry.y () - priv->serverGeometry.border () - priv->serverInput.top,
-		     priv->serverGeometry.width () + priv->serverGeometry.border () * 2 +
-		     priv->serverInput.left + priv->serverInput.right,
-		     priv->serverGeometry.height () + priv->serverGeometry.border () * 2 +
-		     priv->serverInput.top + priv->serverInput.bottom);
-}
-
-CompRect
-CompWindow::outputRect () const
-{
-    return CompRect (priv->geometry.x () - priv->serverGeometry.border ()- priv->output.left,
-		     priv->geometry.y () - priv->serverGeometry.border () - priv->output.top,
-		     priv->geometry.width () + priv->serverGeometry.border () * 2 +
-		     priv->output.left + priv->output.right,
-		     priv->geometry.height () + priv->serverGeometry.border () * 2 +
-		     priv->output.top + priv->output.bottom);
-}
-
-CompRect
+    priv->serverInputRect.refresh ();
+    return priv->serverInputRect.rect ();
+}
+
+const CompRect &
 CompWindow::serverOutputRect () const
 {
-    return CompRect (priv->serverGeometry.x () - priv->serverGeometry.border () -  priv->output.left,
-		     priv->serverGeometry.y () - priv->serverGeometry.border () - priv->output.top,
-		     priv->serverGeometry.width () + priv->serverGeometry.border () * 2 +
-		     priv->output.left + priv->output.right,
-		     priv->serverGeometry.height () + priv->serverGeometry.border () * 2 +
-		     priv->output.top + priv->output.bottom);
+    priv->serverOutputRect.refresh ();
+    return priv->serverOutputRect.rect ();
+}
+
+const CompRect &
+CompWindow::geometryBorders () const
+{
+    priv->geometryBorders.refresh ();
+    return priv->geometryBorders.rect ();
+}
+
+const CompRect &
+CompWindow::serverGeometryBorders () const
+{
+    priv->serverGeometryBorders.refresh ();
+    return priv->serverGeometryBorders.rect ();
 }