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

Proposed by Sam Spilsbury on 2012-02-29
Status: Superseded
Proposed branch: lp:~smspillaz/compiz-core/compiz-core.optimization-inlining
Merge into: lp:compiz-core
Diff against target: 338 lines (+129/-78)
9 files modified
include/core/region.h (+1/-1)
include/core/window.h (+11/-7)
src/privatewindow.h (+16/-0)
src/rect/include/core/rect.h (+19/-0)
src/region.cpp (+0/-6)
src/window.cpp (+9/-0)
src/window/geometry/include/core/windowgeometry.h (+1/-1)
src/window/geometry/src/windowgeometry.cpp (+0/-6)
src/windowgeometry.cpp (+72/-57)
To merge this branch: bzr merge lp:~smspillaz/compiz-core/compiz-core.optimization-inlining
Reviewer Review Type Date Requested Status
Daniel van Vugt Needs Fixing on 2012-03-05
Alan Griffiths 2012-02-29 Approve on 2012-02-29
Review via email: mp+95094@code.launchpad.net

This proposal has been superseded by a proposal from 2012-04-19.

Description of the change

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.

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

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) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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

Daniel van Vugt (vanvugt) wrote :

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) wrote :

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

Daniel van Vugt (vanvugt) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/core/region.h'
2--- include/core/region.h 2012-02-27 03:41:13 +0000
3+++ include/core/region.h 2012-02-29 03:40:25 +0000
4@@ -71,7 +71,7 @@
5 /**
6 * Returns the internal XRegion handle
7 */
8- Region handle () const;
9+ Region handle () const { return static_cast<Region> (priv); }
10
11 /**
12 * Returns true if the specified CompPoint falls within the
13
14=== modified file 'include/core/window.h'
15--- include/core/window.h 2012-02-16 09:55:06 +0000
16+++ include/core/window.h 2012-02-29 03:40:25 +0000
17@@ -317,19 +317,23 @@
18 /* With border */
19 int serverWidth () const;
20 int serverHeight () const;
21- const CompSize serverSize () const;
22+ CompSize serverSize () const;
23
24 /* effective decoration extents */
25- CompRect borderRect () const;
26- CompRect serverBorderRect () const;
27+ const CompRect & borderRect () const;
28+ const CompRect & serverBorderRect () const;
29
30 /* frame window geometry */
31- CompRect inputRect () const;
32- CompRect serverInputRect () const;
33+ const CompRect & inputRect () const;
34+ const CompRect & serverInputRect () const;
35
36 /* includes decorations and shadows */
37- CompRect outputRect () const;
38- CompRect serverOutputRect () const;
39+ const CompRect & outputRect () const;
40+ const CompRect & serverOutputRect () const;
41+
42+ /* only includes X11 borders */
43+ const CompRect & geometryBorders () const;
44+ const CompRect & serverGeometryBorders () const;
45
46 Window id ();
47 Window frame ();
48
49=== modified file 'src/privatewindow.h'
50--- src/privatewindow.h 2012-02-14 14:37:04 +0000
51+++ src/privatewindow.h 2012-02-29 03:40:25 +0000
52@@ -209,6 +209,13 @@
53
54 bool checkClear ();
55
56+ void recalcExtentsRect (CompRect &rect,
57+ const CompWindow::Geometry *geometry,
58+ const CompWindowExtents *extents);
59+
60+ void recalcGeometryBordersRect (CompRect &rect,
61+ const CompWindow::Geometry *geometry);
62+
63 public:
64
65 PrivateWindow *priv;
66@@ -251,6 +258,15 @@
67 CompRegion inputRegion;
68 CompRegion frameRegion;
69
70+ compiz::rect::RecalcRect borderRect;
71+ compiz::rect::RecalcRect serverBorderRect;
72+ compiz::rect::RecalcRect inputRect;
73+ compiz::rect::RecalcRect serverInputRect;
74+ compiz::rect::RecalcRect outputRect;
75+ compiz::rect::RecalcRect serverOutputRect;
76+ compiz::rect::RecalcRect geometryBorders;
77+ compiz::rect::RecalcRect serverGeometryBorders;
78+
79 unsigned int wmType;
80 unsigned int type;
81 unsigned int state;
82
83=== modified file 'src/rect/include/core/rect.h'
84--- src/rect/include/core/rect.h 2012-01-19 19:32:29 +0000
85+++ src/rect/include/core/rect.h 2012-02-29 03:40:25 +0000
86@@ -33,6 +33,8 @@
87 #include <X11/Xlib.h>
88 #include <X11/Xutil.h>
89 #include <X11/Xregion.h>
90+#include <boost/function.hpp>
91+#include <boost/bind.hpp>
92 #include "core/point.h"
93
94
95@@ -144,6 +146,23 @@
96 {
97 CompPoint wraparoundPoint (const CompRect &bounds,
98 const CompPoint &p);
99+
100+ class RecalcRect
101+ {
102+ public:
103+
104+ typedef boost::function <void (CompRect &rect)> RecalcFunc;
105+
106+ RecalcRect (const RecalcFunc &func);
107+
108+ const CompRect & rect () const { return mRect; }
109+ void refresh () { mRecalcFunc (mRect); }
110+
111+ private:
112+
113+ CompRect mRect;
114+ RecalcFunc mRecalcFunc;
115+ };
116 }
117 }
118
119
120=== modified file 'src/region.cpp'
121--- src/region.cpp 2012-02-27 08:05:51 +0000
122+++ src/region.cpp 2012-02-29 03:40:25 +0000
123@@ -118,12 +118,6 @@
124 priv = XCreateRegion ();
125 }
126
127-Region
128-CompRegion::handle () const
129-{
130- return static_cast<Region> (priv);
131-}
132-
133 CompRegion &
134 CompRegion::operator= (const CompRegion &c)
135 {
136
137=== modified file 'src/window.cpp'
138--- src/window.cpp 2012-02-20 02:30:34 +0000
139+++ src/window.cpp 2012-02-29 03:40:25 +0000
140@@ -6589,6 +6589,15 @@
141 width (0),
142 height (0),
143 region (),
144+ frameRegion (),
145+ borderRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &border)),
146+ serverBorderRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &border)),
147+ inputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &input)),
148+ serverInputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &serverInput)),
149+ outputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &geometry, &output)),
150+ serverOutputRect (boost::bind (&PrivateWindow::recalcExtentsRect, this, _1, &serverGeometry, &output)),
151+ geometryBorders (boost::bind (&PrivateWindow::recalcGeometryBordersRect, this, _1, &geometry)),
152+ serverGeometryBorders (boost::bind (&PrivateWindow::recalcGeometryBordersRect, this, _1, &serverGeometry)),
153 wmType (0),
154 type (CompWindowTypeUnknownMask),
155 state (0),
156
157=== modified file 'src/window/geometry/include/core/windowgeometry.h'
158--- src/window/geometry/include/core/windowgeometry.h 2012-01-19 20:08:32 +0000
159+++ src/window/geometry/include/core/windowgeometry.h 2012-02-29 03:40:25 +0000
160@@ -55,7 +55,7 @@
161 Geometry ();
162 Geometry (int x, int y, int width, int height, int border);
163
164- int border () const;
165+ int border () const { return mBorder; }
166
167 void set (int x, int y, int width, int height, int border);
168 void setBorder (int border);
169
170=== modified file 'src/window/geometry/src/windowgeometry.cpp'
171--- src/window/geometry/src/windowgeometry.cpp 2012-01-19 20:08:32 +0000
172+++ src/window/geometry/src/windowgeometry.cpp 2012-02-29 03:40:25 +0000
173@@ -41,12 +41,6 @@
174 {
175 }
176
177-int
178-compiz::window::Geometry::border () const
179-{
180- return mBorder;
181-}
182-
183 void
184 compiz::window::Geometry::setBorder (int border)
185 {
186
187=== modified file 'src/windowgeometry.cpp'
188--- src/windowgeometry.cpp 2012-01-23 05:44:19 +0000
189+++ src/windowgeometry.cpp 2012-02-29 03:40:25 +0000
190@@ -113,7 +113,7 @@
191 2 * priv->serverGeometry.border ();
192 }
193
194-const CompSize
195+CompSize
196 CompWindow::serverSize () const
197 {
198 return CompSize (priv->serverGeometry.width () +
199@@ -122,68 +122,83 @@
200 2 * priv->serverGeometry.border ());
201 }
202
203-CompRect
204+void
205+PrivateWindow::recalcExtentsRect (CompRect &rect, const CompWindow::Geometry *geometry, const CompWindowExtents *extents)
206+{
207+ rect.setGeometry (geometry->x () - geometry->border () - extents->left,
208+ geometry->y () - geometry->border () - extents->top,
209+ geometry->width () + geometry->border () * 2 +
210+ extents->left + extents->right,
211+ geometry->height () + geometry->border () * 2 +
212+ extents->top + extents->bottom);
213+}
214+
215+void
216+PrivateWindow::recalcGeometryBordersRect (CompRect &rect, const CompWindow::Geometry *geometry)
217+{
218+ rect.setGeometry (geometry->x () - geometry->border (),
219+ geometry->y () - geometry->border (),
220+ geometry->width () + geometry->border () * 2,
221+ geometry->height () + geometry->border () * 2);
222+}
223+
224+compiz::rect::RecalcRect::RecalcRect (const RecalcFunc &func) :
225+ mRecalcFunc (func)
226+{
227+}
228+
229+const CompRect &
230 CompWindow::borderRect () const
231 {
232- return CompRect (priv->geometry.x () - priv->geometry.border () - priv->border.left,
233- priv->geometry.y () - priv->geometry.border () - priv->border.top,
234- priv->geometry.width () + priv->geometry.border () * 2 +
235- priv->border.left + priv->border.right,
236- priv->geometry.height () + priv->geometry.border () * 2 +
237- priv->border.top + priv->border.bottom);
238-}
239-
240-CompRect
241+ priv->borderRect.refresh ();
242+ return priv->borderRect.rect ();
243+}
244+
245+const CompRect &
246+CompWindow::inputRect () const
247+{
248+ priv->inputRect.refresh ();
249+ return priv->inputRect.rect ();
250+}
251+
252+const CompRect &
253+CompWindow::outputRect () const
254+{
255+ priv->outputRect.refresh ();
256+ return priv->outputRect.rect ();
257+}
258+
259+const CompRect &
260 CompWindow::serverBorderRect () const
261 {
262- return CompRect (priv->serverGeometry.x () - priv->geometry.border () - priv->border.left,
263- priv->serverGeometry.y () - priv->geometry.border () - priv->border.top,
264- priv->serverGeometry.width () + priv->geometry.border () * 2 +
265- priv->border.left + priv->border.right,
266- priv->serverGeometry.height () + priv->geometry.border () * 2 +
267- priv->border.top + priv->border.bottom);
268-}
269-
270-CompRect
271-CompWindow::inputRect () const
272-{
273- return CompRect (priv->geometry.x () - priv->geometry.border () - priv->serverInput.left,
274- priv->geometry.y () - priv->geometry.border () - priv->serverInput.top,
275- priv->geometry.width () + priv->geometry.border () * 2 +
276- priv->serverInput.left + priv->serverInput.right,
277- priv->geometry.height () +priv->geometry.border () * 2 +
278- priv->serverInput.top + priv->serverInput.bottom);
279-}
280-
281-CompRect
282+ priv->serverBorderRect.refresh ();
283+ return priv->serverBorderRect.rect ();
284+}
285+
286+const CompRect &
287 CompWindow::serverInputRect () const
288 {
289- return CompRect (priv->serverGeometry.x () - priv->serverGeometry.border () - priv->serverInput.left,
290- priv->serverGeometry.y () - priv->serverGeometry.border () - priv->serverInput.top,
291- priv->serverGeometry.width () + priv->serverGeometry.border () * 2 +
292- priv->serverInput.left + priv->serverInput.right,
293- priv->serverGeometry.height () + priv->serverGeometry.border () * 2 +
294- priv->serverInput.top + priv->serverInput.bottom);
295-}
296-
297-CompRect
298-CompWindow::outputRect () const
299-{
300- return CompRect (priv->geometry.x () - priv->serverGeometry.border ()- priv->output.left,
301- priv->geometry.y () - priv->serverGeometry.border () - priv->output.top,
302- priv->geometry.width () + priv->serverGeometry.border () * 2 +
303- priv->output.left + priv->output.right,
304- priv->geometry.height () + priv->serverGeometry.border () * 2 +
305- priv->output.top + priv->output.bottom);
306-}
307-
308-CompRect
309+ priv->serverInputRect.refresh ();
310+ return priv->serverInputRect.rect ();
311+}
312+
313+const CompRect &
314 CompWindow::serverOutputRect () const
315 {
316- return CompRect (priv->serverGeometry.x () - priv->serverGeometry.border () - priv->output.left,
317- priv->serverGeometry.y () - priv->serverGeometry.border () - priv->output.top,
318- priv->serverGeometry.width () + priv->serverGeometry.border () * 2 +
319- priv->output.left + priv->output.right,
320- priv->serverGeometry.height () + priv->serverGeometry.border () * 2 +
321- priv->output.top + priv->output.bottom);
322+ priv->serverOutputRect.refresh ();
323+ return priv->serverOutputRect.rect ();
324+}
325+
326+const CompRect &
327+CompWindow::geometryBorders () const
328+{
329+ priv->geometryBorders.refresh ();
330+ return priv->geometryBorders.rect ();
331+}
332+
333+const CompRect &
334+CompWindow::serverGeometryBorders () const
335+{
336+ priv->serverGeometryBorders.refresh ();
337+ return priv->serverGeometryBorders.rect ();
338 }

Subscribers

People subscribed via source and target branches