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

Proposed by Daniel van Vugt on 2012-02-27
Status: Merged
Merged at revision: 3023
Proposed branch: lp:~vanvugt/compiz-core/fix-940139
Merge into: lp:compiz-core
Diff against target: 420 lines (+105/-96)
6 files modified
include/core/region.h (+12/-4)
plugins/decor/src/decor.cpp (+20/-12)
plugins/decor/src/decor.h (+1/-0)
plugins/opengl/src/paint.cpp (+2/-2)
src/privateregion.h (+0/-41)
src/region.cpp (+70/-37)
To merge this branch: bzr merge lp:~vanvugt/compiz-core/fix-940139
Reviewer Review Type Date Requested Status
Alan Griffiths 2012-02-27 Approve on 2012-02-27
Review via email: mp+94728@code.launchpad.net

This proposal supersedes a proposal from 2012-02-27.

Description of the change

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)

To post a comment you must log in.
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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.

Alan Griffiths (alan-griffiths) wrote :

No tests for CompRegionRef?

review: Approve
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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
Daniel van Vugt (vanvugt) wrote :

I will add tests for CompRegionRef. Forgot about that class (which was added) after I added and then removed other CompRegion test cases (for methods that I added, then decided to remove).

Re: Indentation, I always use the tab/space combinations that fit the context, using the same style as the existing nearby code does. I did get the impression that pure spaces was preferred to new code, but if you prefer the tab/space combination for new code then I can go back to that.

Daniel van Vugt (vanvugt) wrote :

Sam, you approved the superseded version of this proposal. I assume it was revision 3024 that you approved?

Daniel van Vugt (vanvugt) wrote :

Assuming Sam's approval above was revision 3024...

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-15 17:45:25 +0000
3+++ include/core/region.h 2012-02-27 08:13:20 +0000
4@@ -32,8 +32,6 @@
5 #include <core/rect.h>
6 #include <core/point.h>
7
8-class PrivateRegion;
9-
10 /**
11 * A 2D region with an (x,y) position and arbitrary dimensions similar to
12 * an XRegion. It's data membmers are private and must be manipulated with
13@@ -180,8 +178,18 @@
14 const CompRegion operator| (const CompRegion &) const;
15 CompRegion & operator|= (const CompRegion &);
16
17- private:
18- PrivateRegion *priv;
19+ protected:
20+ /* Construct a CompRegion based on an externally managed Region */
21+ explicit CompRegion (Region);
22+ void init ();
23+ void *priv;
24+};
25+
26+class CompRegionRef : public CompRegion
27+{
28+ public:
29+ explicit CompRegionRef (Region);
30+ ~CompRegionRef ();
31 };
32
33 extern const CompRegion infiniteRegion;
34
35=== modified file 'plugins/decor/src/decor.cpp'
36--- plugins/decor/src/decor.cpp 2012-02-24 08:22:28 +0000
37+++ plugins/decor/src/decor.cpp 2012-02-27 08:13:20 +0000
38@@ -107,8 +107,6 @@
39
40 for (it--; it != screen->windows ().end (); it--)
41 {
42- CompRegion inter;
43-
44 if (!(*it)->isViewable ())
45 continue;
46
47@@ -121,10 +119,11 @@
48 if (!isAncestorTo (window, (*it)))
49 continue;
50
51- inter = shadowRegion.intersected ((*it)->borderRect ());
52+ CompRegion inter (shadowRegion);
53+ inter &= (*it)->borderRect ();
54
55 if (!inter.isEmpty ())
56- shadowRegion = shadowRegion.subtracted (inter);
57+ shadowRegion -= inter;
58 }
59
60 /* If the region didn't change, then it is safe to
61@@ -140,7 +139,7 @@
62 * that will look a lot better.
63 */
64 if (window->type () == CompWindowTypeDropdownMenuMask &&
65- shadowRegion == CompRegion (window->outputRect ()))
66+ shadowRegion == CompRegionRef (window->outputRect ().region ()))
67 {
68 CompRect area (window->outputRect ().x1 (),
69 window->outputRect ().y1 (),
70@@ -148,7 +147,7 @@
71 window->inputRect ().y1 () -
72 window->outputRect ().y1 ());
73
74- shadowRegion = shadowRegion.subtracted (area);
75+ shadowRegion -= area;
76 }
77 }
78 }
79@@ -247,17 +246,25 @@
80 const CompRegion &region,
81 unsigned int mask)
82 {
83- CompRegion reg = shadowRegion.intersected (region);
84+ const CompRegion *preg = NULL;
85
86 if ((mask & (PAINT_WINDOW_ON_TRANSFORMED_SCREEN_MASK |
87 PAINT_WINDOW_WITH_OFFSET_MASK)))
88- reg = region;
89+ preg = &region;
90 else if (mask & PAINT_WINDOW_TRANSFORMED_MASK)
91- reg = infiniteRegion;
92+ preg = &infiniteRegion;
93+ else
94+ {
95+ tmpRegion = shadowRegion;
96+ tmpRegion &= region;
97+ preg = &tmpRegion;
98+ }
99
100 /* In case some plugin needs to paint us with an offset region */
101- if (reg.isEmpty ())
102- reg = region;
103+ if (preg->isEmpty ())
104+ preg = &region;
105+
106+ const CompRegion &reg (*preg);
107
108 if (wd &&
109 wd->decor->type == WINDOW_DECORATION_TYPE_PIXMAP)
110@@ -278,7 +285,8 @@
111 if (box.width () > 0 && box.height () > 0)
112 {
113 ml[0] = wd->quad[i].matrix;
114- gWindow->glAddGeometry (ml, CompRegion (box), reg);
115+ const CompRegionRef boxRegion (box.region ());
116+ gWindow->glAddGeometry (ml, boxRegion, reg);
117 }
118 }
119
120
121=== modified file 'plugins/decor/src/decor.h'
122--- plugins/decor/src/decor.h 2012-02-09 07:48:57 +0000
123+++ plugins/decor/src/decor.h 2012-02-27 08:13:20 +0000
124@@ -272,6 +272,7 @@
125
126 CompRegion frameRegion;
127 CompRegion shadowRegion;
128+ CompRegion tmpRegion;
129
130 Window inputFrame;
131 Window outputFrame;
132
133=== modified file 'plugins/opengl/src/paint.cpp'
134--- plugins/opengl/src/paint.cpp 2012-01-30 05:12:24 +0000
135+++ plugins/opengl/src/paint.cpp 2012-02-27 08:13:20 +0000
136@@ -1179,8 +1179,8 @@
137 WRAPABLE_HND_FUNCTN_RETURN (bool, glDraw, transform,
138 fragment, region, mask)
139
140- const CompRegion reg = (mask & PAINT_WINDOW_TRANSFORMED_MASK) ?
141- infiniteRegion : region;
142+ const CompRegion &reg = (mask & PAINT_WINDOW_TRANSFORMED_MASK) ?
143+ infiniteRegion : region;
144
145 if (reg.isEmpty ())
146 return true;
147
148=== removed file 'src/privateregion.h'
149--- src/privateregion.h 2010-12-04 02:41:35 +0000
150+++ src/privateregion.h 1970-01-01 00:00:00 +0000
151@@ -1,41 +0,0 @@
152-/*
153- * Copyright © 2008 Dennis Kasprzyk
154- *
155- * Permission to use, copy, modify, distribute, and sell this software
156- * and its documentation for any purpose is hereby granted without
157- * fee, provided that the above copyright notice appear in all copies
158- * and that both that copyright notice and this permission notice
159- * appear in supporting documentation, and that the name of
160- * Dennis Kasprzyk not be used in advertising or publicity pertaining to
161- * distribution of the software without specific, written prior permission.
162- * Dennis Kasprzyk makes no representations about the suitability of this
163- * software for any purpose. It is provided "as is" without express or
164- * implied warranty.
165- *
166- * DENNIS KASPRZYK DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
167- * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
168- * NO EVENT SHALL DENNIS KASPRZYK BE LIABLE FOR ANY SPECIAL, INDIRECT OR
169- * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
170- * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
171- * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
172- * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
173- *
174- * Authors: Dennis Kasprzyk <onestone@compiz-fusion.org>
175- */
176-
177-#ifndef _PRIVATEREGION_H
178-#define _PRIVATEREGION_H
179-
180-#include <core/rect.h>
181-#include <core/region.h>
182-
183-class PrivateRegion {
184- public:
185- PrivateRegion ();
186- ~PrivateRegion ();
187-
188- public:
189- Region region;
190-};
191-
192-#endif
193
194=== modified file 'src/region.cpp'
195--- src/region.cpp 2012-02-15 17:45:25 +0000
196+++ src/region.cpp 2012-02-27 08:13:20 +0000
197@@ -23,13 +23,15 @@
198 * Authors: Dennis Kasprzyk <onestone@compiz-fusion.org>
199 */
200
201-#include "privateregion.h"
202+#include "core/rect.h"
203+#include "core/region.h"
204
205 #include <X11/Xlib-xcb.h>
206 #include <X11/Xutil.h>
207 #include <X11/Xregion.h>
208
209 #include <stdio.h>
210+#include <cassert>
211
212 #include <boost/foreach.hpp>
213 #define foreach BOOST_FOREACH
214@@ -38,20 +40,29 @@
215 MAXSHORT * 2, MAXSHORT * 2));
216 const CompRegion emptyRegion;
217
218+/* Alternate emptyRegion, mostly just required to construct infiniteRegion
219+ when emptyRegion may still be uninitialized */
220+static const CompRegion &
221+_emptyRegion ()
222+{
223+ static const CompRegion r;
224+ return r;
225+}
226+
227 CompRegion::CompRegion ()
228 {
229- priv = new PrivateRegion ();
230+ init ();
231 }
232
233 CompRegion::CompRegion (const CompRegion &c)
234 {
235- priv = new PrivateRegion ();
236- XUnionRegion (CompRegion ().handle (), c.priv->region, priv->region);
237+ init ();
238+ XUnionRegion (_emptyRegion ().handle (), c.handle (), handle ());
239 }
240
241 CompRegion::CompRegion ( int x, int y, int w, int h)
242 {
243- priv = new PrivateRegion ();
244+ init ();
245
246 XRectangle rect;
247
248@@ -60,14 +71,12 @@
249 rect.width = w;
250 rect.height = h;
251
252- Region tmp = XCreateRegion ();
253- XUnionRectWithRegion (&rect, tmp, priv->region);
254- XDestroyRegion (tmp);
255+ XUnionRectWithRegion (&rect, _emptyRegion ().handle (), handle ());
256 }
257
258 CompRegion::CompRegion (const CompRect &r)
259 {
260- priv = new PrivateRegion ();
261+ init ();
262
263 XRectangle rect;
264
265@@ -76,26 +85,49 @@
266 rect.width = r.width ();
267 rect.height = r.height ();
268
269- Region tmp = XCreateRegion ();
270- XUnionRectWithRegion (&rect, tmp, priv->region);
271- XDestroyRegion (tmp);
272+ XUnionRectWithRegion (&rect, _emptyRegion ().handle (), handle ());
273+}
274+
275+CompRegion::CompRegion (Region external)
276+{
277+ priv = external;
278+}
279+
280+CompRegionRef::CompRegionRef (Region external) :
281+ CompRegion (external)
282+{
283+}
284+
285+CompRegionRef::~CompRegionRef ()
286+{
287+ /* Ensure CompRegion::~CompRegion does not destroy the region, because
288+ it's external and we don't own it. */
289+ priv = NULL;
290 }
291
292 CompRegion::~CompRegion ()
293 {
294- delete priv;
295+ if (priv)
296+ XDestroyRegion (static_cast<Region> (priv));
297+}
298+
299+void
300+CompRegion::init ()
301+{
302+ assert (sizeof (Region) == sizeof (void*));
303+ priv = XCreateRegion ();
304 }
305
306 Region
307 CompRegion::handle () const
308 {
309- return priv->region;
310+ return static_cast<Region> (priv);
311 }
312
313 CompRegion &
314 CompRegion::operator= (const CompRegion &c)
315 {
316- XUnionRegion (CompRegion ().handle (), c.priv->region, priv->region);
317+ XUnionRegion (emptyRegion.handle (), c.handle (), handle ());
318 return *this;
319 }
320
321@@ -195,9 +227,9 @@
322 return rv;
323
324 BOX b;
325- for (int i = 0; i < priv->region->numRects; i++)
326+ for (int i = 0; i < handle ()->numRects; i++)
327 {
328- b = priv->region->rects[i];
329+ b = handle ()->rects[i];
330 rv.push_back (CompRect (b.x1, b.y1, b.x2 - b.x1, b.y2 - b.y1));
331 }
332 return rv;
333@@ -314,13 +346,16 @@
334 CompRegion &
335 CompRegion::operator&= (const CompRegion &r)
336 {
337- return *this = *this & r;
338+ XIntersectRegion (r.handle (), handle (), handle ());
339+ return *this;
340 }
341
342 CompRegion &
343 CompRegion::operator&= (const CompRect &r)
344 {
345- return *this = *this & r;
346+ CompRegionRef ref (r.region ());
347+ XIntersectRegion (ref.handle (), handle (), handle ());
348+ return *this;
349 }
350
351 const CompRegion
352@@ -338,13 +373,16 @@
353 CompRegion &
354 CompRegion::operator+= (const CompRegion &r)
355 {
356- return *this = *this + r;
357+ XUnionRegion (handle (), r.handle (), handle ());
358+ return *this;
359 }
360
361 CompRegion &
362 CompRegion::operator+= (const CompRect &r)
363 {
364- return *this = *this + r;
365+ CompRegionRef ref (r.region ());
366+ XUnionRegion (handle (), ref.handle (), handle ());
367+ return *this;
368 }
369
370 const CompRegion
371@@ -362,13 +400,16 @@
372 CompRegion &
373 CompRegion::operator-= (const CompRegion &r)
374 {
375- return *this = *this - r;
376+ XSubtractRegion (handle (), r.handle (), handle ());
377+ return *this;
378 }
379
380 CompRegion &
381 CompRegion::operator-= (const CompRect &r)
382 {
383- return *this = *this - r;
384+ CompRegionRef ref (r.region ());
385+ XSubtractRegion (handle (), ref.handle (), handle ());
386+ return *this;
387 }
388
389 const CompRegion
390@@ -380,7 +421,8 @@
391 CompRegion &
392 CompRegion::operator^= (const CompRegion &r)
393 {
394- return *this = *this ^ r;
395+ XXorRegion (handle (), r.handle (), handle ());
396+ return *this;
397 }
398
399 const CompRegion
400@@ -392,16 +434,7 @@
401 CompRegion &
402 CompRegion::operator|= (const CompRegion &r)
403 {
404- return *this = *this | r;
405-}
406-
407-
408-PrivateRegion::PrivateRegion ()
409-{
410- region = XCreateRegion ();
411-}
412-
413-PrivateRegion::~PrivateRegion ()
414-{
415- XDestroyRegion (region);
416-}
417+ XUnionRegion (handle (), r.handle (), handle ());
418+ return *this;
419+}
420+

Subscribers

People subscribed via source and target branches