<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/104052/">http://git.reviewboard.kde.org/r/104052/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 24th, 2012, 1:16 a.m., <b>Michael Pyne</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/104052/diff/2/?file=50863#file50863line1491" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/util/kshareddatacache.cpp</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; ">bool KSharedDataCache::insert(const QString &key, const QByteArray &data)</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">1491</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">*</span><span class="n">destination</span> <span class="o">=</span> <span class="n">QByteArray</span><span class="p">(</span><span class="n">temp</span><span class="p">.</span><span class="n">constData</span><span class="p">(),</span> <span class="n">temp</span><span class="p">.</span><span class="n">size</span><span class="p">());</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;">The cache is unlocked here but temp is still pointing into its internals (as mentioned above)... it's possible to extract the lock out to wrap around this copy too but then I'm not sure how that would make anything different, you'd still be performing the copy.</pre>
</blockquote>
<p>On February 24th, 2012, 1:40 a.m., <b>Mark Gaiser</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;">This is mimicking the old behavior and i'm in fact returning a copy of the data here, just like how it used to be. This one won't cause a race condition. The temp value should be gone when the function moves out of scope right? Then i don't see the issue..</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;">The data does get copied here, but the copy must be complete *before* we get to this point. As it stands the cache becomes unlocked (which means **no guarantees** can be made about the lifetime of entries still in the cache) as soon as rawFind() completes. You'd never notice this error unless you can torture test KSharedDataCache to expose the race condition, but it's a race nonetheless.
Either the lock can be broken out and wrapped around the rawFind()/copy combination (but then why make the switch at all) or some way can be implemented to detect that the required cache entry has been changed and just return a failure value (which is perfectly acceptable, this is a cache after all).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 24th, 2012, 1:16 a.m., <b>Michael Pyne</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/104052/diff/2/?file=50867#file50867line103" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/util/kimagecache.cpp</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; ">KImageCache::~KImageCache()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">101</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">image</span><span class="p">.</span><span class="n">save</span><span class="p">(</span><span class="o">&</span><span class="n">buffer</span><span class="p">,</span> <span class="s">"PNG"</span><span class="p">);</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">Like some of the other reviewers I would prefer to maintain at least some compression of the image (since in general CPU is nowadays cheaper than memory and memory bandwidth), in addition to the concern with indexed/paletted images (from the Qt docs it seems the color table will still be left empty here).
Of course, the reason the QImage must be constructed that way is mostly related to being able to use the rawData QByteArray directly, which we can't really do without the cache locked... assuming we can find a way around that this construct might still be useful for QImages which we verify have no color table.</pre>
</blockquote>
<p>On February 24th, 2012, 1:40 a.m., <b>Mark Gaiser</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;">Ehm no. That would beat the entire meaning of this patch. The image.save function was the one causing high clock cycles and should be prevented. And, again, i really don't see why it would be needed to store an actual image (png) in a mmapped file. To me it seems way more obvious to store the bits that make up the image. Compressing the data is fine but then we need to use either qCompress or LZ4.</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;">So let's work on this first (especially since in the end we'll still be needing to copy data in/out anyways).
Storing an image in a container format is certainly not required (it was done simply because it was expedient)... it seems that we just need to check that the image doesn't actually require a color table of some sort (if it does we can just fall back to what we do now).
One problem I didn't mention on my last review is that changing the binary format of the KImageCache is binary-incompatible with existing KImageCaches... and I don't think KImageCache has a versioning field, only KSharedDataCache. I wouldn't mind bumping the KSharedDataCache version to support this (and potentially adding a sub-version field while I'm at it), so let me know if there's no other solution.</pre>
<br />
<p>- Michael</p>
<br />
<p>On February 23rd, 2012, 7:23 p.m., Mark Gaiser 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 kdelibs, David Faure and Michael Pyne.</div>
<div>By Mark Gaiser.</div>
<p style="color: grey;"><i>Updated Feb. 23, 2012, 7:23 p.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;">I was running KWin through callgrind to see where possible bottlenecks are. I wasn't expecting much since it improved greatly during the 4.8 dev cycle, however one stood out. The saving of PNG images was taking about 1/5th of the time in KWin that i could see directly. That looked like something i might be able to optimize.
What this patch is doing is storing the actual image bits to prevent saving a PNG image to the mmapped cache. That was a hot code path in time (cycles), not even in calls. I've also reduced the amount of memory copies to a bare minimum by adding a rawFind function to KSharedDataCache which fills a QByteArray::fromRawData thus preventing a expensive memory copy. The rawFind is used for looking up an image and fetching it's data without copying it. That is done because QImage seems to make a copy itself internally. I don't have any performance measurements, however, prior to this patch my kwin test was using up ~5.000.000.000 cycles. After this patch it's using up 1.370.000.000. I don't have raw performance numbers to see if the cache itself is actually faster, it certainly has become a lot cheaper to use the cache. Logic wise i would say creating a QImage from the cached data should be way faster now since there is no step involved anymore in decoding the image. Storing is certainly an order of magnitude faster.
Benchmark numbers. insert(write) and find(read)
-- Before patch --
READ : 0.019 msecs per iteration (total: 79, iterations: 4096)
WRITE: 0.010 msecs per iteration (total: 88, iterations: 8192)
-- After patch --
READ : 0.019 msecs per iteration (total: 79, iterations: 4096)
WRITE: 0.0026 msecs per iteration (total: 87, iterations: 32768)
Reading is equal in speed, writing is ~5x faster after the patch.
Special thanks go to David Faure for helping me a great deal with this.</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;">I've also written a bunch of test cases (greatly improved by David Faure) to see if i didn't break anything. According to the test (which is also comparing the actual image bits) it's all passing just fine.</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>kdecore/util/kshareddatacache.h <span style="color: grey">(339cecc)</span></li>
<li>kdecore/util/kshareddatacache.cpp <span style="color: grey">(9fe3995)</span></li>
<li>kdeui/tests/CMakeLists.txt <span style="color: grey">(63788f6)</span></li>
<li>kdeui/tests/kimagecachetests.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kdeui/tests/kimagecachetests.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kdeui/util/kimagecache.cpp <span style="color: grey">(a5bbbe1)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104052/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>