[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: [RFC] Move decoration painting out of
From: Martin_Gräßlin <kde () martin-graesslin ! com>
Date: 2011-09-18 6:22:10
Message-ID: 20110918062210.16225.41010 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Sept. 17, 2011, 12:32 p.m., Thomas Lübking wrote:
> > kwin/paintredirector.h, line 37
> > <http://git.reviewboard.kde.org/r/102614/diff/3/?file=36318#file36318line37>
> >
> > ": public QTimer" ?
>
> Martin Gräßlin wrote:
> Do you see any advantages of inheriting QTimer instead of encapsulating it? I \
> decided to not inherit, because I would have had to overwrite the start() method \
> which is not virtual. Also QTimer's dtor is not virtual which seems like it is not \
> meant to be inherited.
> Thomas Lübking wrote:
> Spare one QObject and the Signal forwarding, or just because it's more "OOP" ;-)
>
> The start() virtuality (that's probably not a word) is no valid argument since it \
> would only apply if you intended to access this as QTimer, what's not possible with \
> the current solution at all.
> -----------
> I'm not sure whether your, mine or some other ppl. idea about virtual destructors \
> is flawed, but QTimer /has/ a virtual destructor because it inherits QObject, that \
> destructor is virtual and that attribute cannot be "taken away".
> Otherwise we'd all be pretty f**** 'cause eg. QWidget doesn't have an explicitly \
> virtual destructor either and calling a base class destructor on a derived class in \
> undefined ("hello crash"), ie. "QWidget *w = new QPushButton; delete w;" would be \
> illegal and i'm pretty sure such can be found bazillion times around KDE ;-)
> Also it would lead to uncertain behavior, since
> QWidget *w = new QPushButton;
> // delete w; // fail? no.
> delete (QObject*)w; // but that succeeds, calling the QPushButton d'ctor now?! - \
> yes, but the "but".
> Here's fun with (virtual) destructors - you should give it a try (or blindly trust \
> the comments) You can also try to remove the "virtual" - gcc has always been pretty \
> gracious but iirc that is in fact not legal. (since this code has no members and \
> exits pretty fast and you likely don't compile O3, there's however little chance to \
> segfault anyway =)
> ====================== snip virtuality.cpp ==================
>
> // g++ -o virtuality virtuality.cpp
> #include <stdio.h>
> class A {
> public:
> A() {};
> virtual ~A() {printf("A");}
> };
>
> class B : public A {
> public:
> B() {};
> ~B() {printf("B");}
> };
>
> class C : public B {
> public:
> C() {};
> ~C() {printf("C");}
> };
>
> int main(int argc, char **argv)
> {
> A* x = new A;
> delete x; // "A"
> printf("\n"); // just to make things more readable
> x = new B;
> delete x; // "A"? "B"? NO! "BA"
> printf("\n");
> x = new C;
> delete x; // "CBA"
> printf("\n");
> B *y = new B;
> delete y; // "BA"
> printf("\n");
> y = new C;
> delete y; // "CBA"
> printf("\n");
> C *z = new C;
> delete z; // CBA"
> printf("\n-> Impressed?\n");
> }
>
> ==================== /snip ==============================
> Spare one QObject and the Signal forwarding, or just because it's more "OOP" ;-)
true for the first part, maybe true for the OOP part ;-) But actually I doubt that \
this code will ever end in master as I have an idea how to change it further \
(rendering the deco in a worker thread and by that removing the need for the timer at \
all).
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102614/#review6590
-----------------------------------------------------------
On Sept. 17, 2011, 7:15 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102614/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2011, 7:15 a.m.)
>
>
> Review request for kwin.
>
>
> Summary
> -------
>
> When doing the callgrind analysis I noticed that the call to paint the decorations \
> is really expensive (which we already knew due to lag in present windows when \
> switching and so on). My idea to tackle this problem is to move the ensurePainted \
> out of performPaint using Arthur's DecorationPaintRedirector.
> This first diff is just a request for comments as I want to know whether this could \
> cause some problems. When the decoration is damaged it uses a 0 msec singleshot \
> timer to move the painting into the next event loop (to prevent the false detection \
> of recursive painting) and on timeout the ensurePainted is triggered.
> The result is: before the inclusive costs of Window::performPaint() was 25 % (1400 \
> calls) with 18 % on ensurePainted with just 69 (!) calls. After the inclusive costs \
> of Window::performPaint() is only ~ 12 % (2100 calls). EnsurePainted has still a \
> high cost but is moved out of the hot path.
>
> Diffs
> -----
>
> kwin/decorationpaintredirector.h PRE-CREATION
> kwin/decorationpaintredirector.cpp PRE-CREATION
> kwin/paintredirector.h bb5d67c
> kwin/paintredirector.cpp 7ed4d6f
> kwin/scene_opengl.cpp 47015b3
> kwin/scene_xrender.cpp fc08289
>
> Diff: http://git.reviewboard.kde.org/r/102614/diff
>
>
> Testing
> -------
>
> With NVIDIA I had seen some rendering artefacts when switching windows. I will \
> watch how it behaves with radeon (could be our well known NVIDIA sucks with pixmap \
> problem).
> Ok there is also a problem with radeon and decoration not being updated correctly - \
> need to investigate.
>
> Thanks,
>
> Martin
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102614/">http://git.reviewboard.kde.org/r/102614/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On September 17th, 2011, 12:32 p.m., <b>Thomas \
Lübking</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/102614/diff/3/?file=36318#file36318line37" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/paintredirector.h</a> <span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">class PendingPaintTimer</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">37</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="o">:</span> <span class="n">public</span> <span \
class="n">QObject</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">": public \
QTimer" ?</pre> </blockquote>
<p>On September 17th, 2011, 6:08 p.m., <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do you see any \
advantages of inheriting QTimer instead of encapsulating it? I decided to not \
inherit, because I would have had to overwrite the start() method which is not \
virtual. Also QTimer's dtor is not virtual which seems like it is not meant to be \
inherited.</pre> </blockquote>
<p>On September 17th, 2011, 11:24 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Spare one QObject and \
the Signal forwarding, or just because it's more "OOP" ;-)
The start() virtuality (that's probably not a word) is no valid argument since it \
would only apply if you intended to access this as QTimer, what's not possible \
with the current solution at all.
-----------
I'm not sure whether your, mine or some other ppl. idea about virtual destructors \
is flawed, but QTimer /has/ a virtual destructor because it inherits QObject, that \
destructor is virtual and that attribute cannot be "taken away".
Otherwise we'd all be pretty f**** 'cause eg. QWidget doesn't have an \
explicitly virtual destructor either and calling a base class destructor on a derived \
class in undefined ("hello crash"), ie. "QWidget *w = new QPushButton; \
delete w;" would be illegal and i'm pretty sure such can be found bazillion \
times around KDE ;-)
Also it would lead to uncertain behavior, since
QWidget *w = new QPushButton;
// delete w; // fail? no.
delete (QObject*)w; // but that succeeds, calling the QPushButton d'ctor now?! - \
yes, but the "but".
Here's fun with (virtual) destructors - you should give it a try (or blindly \
trust the comments) You can also try to remove the "virtual" - gcc has \
always been pretty gracious but iirc that is in fact not legal. (since this code has \
no members and exits pretty fast and you likely don't compile O3, there's \
however little chance to segfault anyway =)
====================== snip virtuality.cpp ==================
// g++ -o virtuality virtuality.cpp
#include <stdio.h>
class A {
public:
A() {};
virtual ~A() {printf("A");}
};
class B : public A {
public:
B() {};
~B() {printf("B");}
};
class C : public B {
public:
C() {};
~C() {printf("C");}
};
int main(int argc, char **argv)
{
A* x = new A;
delete x; // "A"
printf("\n"); // just to make things more readable
x = new B;
delete x; // "A"? "B"? NO! "BA"
printf("\n");
x = new C;
delete x; // "CBA"
printf("\n");
B *y = new B;
delete y; // "BA"
printf("\n");
y = new C;
delete y; // "CBA"
printf("\n");
C *z = new C;
delete z; // CBA"
printf("\n-> Impressed?\n");
}
==================== /snip ==============================</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Spare \
one QObject and the Signal forwarding, or just because it's more "OOP" \
;-) true for the first part, maybe true for the OOP part ;-) But actually I doubt \
that this code will ever end in master as I have an idea how to change it further \
(rendering the deco in a worker thread and by that removing the need for the timer at \
all).</pre> <br />
<p>- Martin</p>
<br />
<p>On September 17th, 2011, 7:15 a.m., Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Sept. 17, 2011, 7:15 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">When doing the callgrind analysis I noticed that the call to paint the \
decorations is really expensive (which we already knew due to lag in present windows \
when switching and so on). My idea to tackle this problem is to move the \
ensurePainted out of performPaint using Arthur's DecorationPaintRedirector.
This first diff is just a request for comments as I want to know whether this could \
cause some problems. When the decoration is damaged it uses a 0 msec singleshot timer \
to move the painting into the next event loop (to prevent the false detection of \
recursive painting) and on timeout the ensurePainted is triggered.
The result is: before the inclusive costs of Window::performPaint() was 25 % (1400 \
calls) with 18 % on ensurePainted with just 69 (!) calls. After the inclusive costs \
of Window::performPaint() is only ~ 12 % (2100 calls). EnsurePainted has still a high \
cost but is moved out of the hot path.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">With NVIDIA I had seen some rendering artefacts when switching windows. \
I will watch how it behaves with radeon (could be our well known NVIDIA sucks with \
pixmap problem).
Ok there is also a problem with radeon and decoration not being updated correctly - \
need to investigate.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/decorationpaintredirector.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>kwin/decorationpaintredirector.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>kwin/paintredirector.h <span style="color: grey">(bb5d67c)</span></li>
<li>kwin/paintredirector.cpp <span style="color: grey">(7ed4d6f)</span></li>
<li>kwin/scene_opengl.cpp <span style="color: grey">(47015b3)</span></li>
<li>kwin/scene_xrender.cpp <span style="color: grey">(fc08289)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102614/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic