<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://reviewboard.kde.org/r/4444/">http://reviewboard.kde.org/r/4444/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 25th, 2010, 1:05 p.m., <b>jmho</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="/r/4444/diff/2/?file=29754#file29754line77" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdeedu/marble/src/lib/TextureTile.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">class TextureTile</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">77</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">QImage</span> <span class="o"><span class="hl">*</span></span><span class="hl"> </span><span class="n">m_image</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">74</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">QImage</span> <span class="n">m_image</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre>Could not this stay a pointer also? It would prevent a QImage copy.</pre>
</blockquote>
<p>On June 25th, 2010, 1:17 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Since QImage is implicitly shared, copying is really fast. Here is a quote from the Qt documentation:
"Implicitly shared classes are both safe and efficient when passed as arguments, because only a pointer to the data is passed around, and the data is copied only if and when a function writes to it, i.e., copy-on-write."
This also means that if m_image is a value, QImage will do the memory management for us.</pre>
</blockquote>
<p>On June 25th, 2010, 1:32 p.m., <b>jmho</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Sure, but
1) with pointers the code is easier to understand in this regard, that is one can easier see if a copy is a deep copy as there are much less copy operations
2) with a copy there is still the copy ctor executed, refernce counting done, etc
</pre>
</blockquote>
<p>On June 25th, 2010, 1:56 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>1) Why are there much less copy operations using a pointer? I'd say the number of deep copy operations are the same.
2) The copy constructor copies a pointer and increments the reference count. Given that this only happens after much more expensive operations like loading a tile from disk, do you really think that the additional increment of the reference count will impact performance?</pre>
</blockquote>
<p>On June 25th, 2010, 2:06 p.m., <b>jmho</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>re 1) I was referring to _all_ copy operations. I think it is clear that there are currently less than after applying the patch. What I meant is that counting the number of deep copy opterations is easier when there are not so much copy operations at all. I'm not saying we would have more deep copy operations with this patch applied. But understanding that would become harder as there were more places to review and think about.
re 2) no
I might add that I changed a little bit in this area exactly with the goal of not invoking QImage's copy ctor, that is using QImage* where possible. So I'm not convinced that making the api a little bit more convient is worth it.</pre>
</blockquote>
<p>On June 25th, 2010, 2:13 p.m., <b>Torsten Rahn</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Bernhard:
1) When you pass a pointer around then you ensure that unintended copies are not created (since those copies would have a different pointer address).</pre>
</blockquote>
<p>On June 25th, 2010, 3:14 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Well, the rule is this: *Every* assignment creates a (virtual) copy. The gag is that a shallow copy is very cheap (comparable to copying a pointer) since a deep copy is delayed until somebody writes to it. In that case, only that somebody pays for the performance panelty. However, changing images behind TextureTile's back is very unclean and shall be made impossible with this patch.
To sum it up, TextureTile will hold the data of an image which was present when it was assigned. If the image was modified afterwards, the TextureTile will hold the old data. This behavior is the same as for QString: If you modify a QString after you passed it to some label, the label will - of course - display the old text. This behavior is also called "value semantics".</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">I disagree with this part of the patch for the reason I explained above.</pre>
<br />
<p>- jmho</p>
<br />
<p>On June 25th, 2010, 12:49 p.m., Bernhard Beschow wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/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 marble.</div>
<div>By Bernhard Beschow.</div>
<p style="color: grey;"><i>Updated 2010-06-25 12:49:24</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;">Motivation: It may happen that Marble receives invalid image data over the network or loads invalid image data from disk, making Marble really slow. This is caused by heavy debug output from QImage while Marble tries to access invalid image data. This will be qFatal in the upcoming Qt 4.7.
Solution: Check if the image is valid ater loading data into a QImage instead of blindly loading data into it. Also Q_ASSERT every image assignment in TextureTile.
Note that by this patch, Marble will abort when TileLoader::scaledLowerLevelTile() is not able to produce a scaled image. This could happen when the level-zero tile is missing.</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;">Works for me. Please test if it works for you as well!</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>trunk/KDE/kdeedu/marble/src/lib/TextureTile.h <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TextureTile.cpp <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TileLoader.h <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TileLoader.cpp <span style="color: grey">(1142673)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4444/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>