<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/111050/">http://git.reviewboard.kde.org/r/111050/</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 23rd, 2013, 6:29 p.m. UTC, <b>Milian Wolff</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/111050/diff/2/?file=165321#file165321line345" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">345</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">suffix</span> <span class="o">=</span> <span class="n">fileName</span><span class="p">.</span><span class="n">mid</span><span class="p">(</span><span class="n">dotPos</span> <span class="o">+</span> <span class="mi">1</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;">try using QStringRef here to safe allocations if you care about speed.</pre>
</blockquote>
<p>On June 23rd, 2013, 9:15 p.m. UTC, <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;">QStringRef misses the mid function which i need. Or left or right, but it misses those as well. So i don't really see how i can use QStringRef here without adding another QString...</pre>
</blockquote>
<p>On June 23rd, 2013, 9:45 p.m. UTC, <b>Milian Wolff</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;">You can still use midRef on the base QString, you just need to take the offsets into account. But anyways, this is probably not worth it if you never saw this in a profile as a hotspot. Just saying, that in general repeated .mid() is quite often a performance bottleneck.</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;">Not worth it indeed. "Usually" files have only one "." in there name so it won't matter much. It only "might" matter if files have a lot of dots and if you have a lot of those filenames. But even then, other things will likely end up as higher hot spots.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 23rd, 2013, 6:29 p.m. UTC, <b>Milian Wolff</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/111050/diff/2/?file=165321#file165321line376" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">320</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="o">!</span><span class="n">is_local_file</span> <span class="o">&&</span> <span class="n">url</span><span class="p">.</span><span class="n">isLocalFile</span><span class="p">()</span><span class="hl"> </span><span class="p"><span class="hl">)</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">376</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">is_local_file</span> <span class="o">&&</span> <span class="n">url</span><span class="p">.</span><span class="n">isLocalFile</span><span class="p">()<span class="hl">)</span></span><span class="hl"> </span><span class="p"><span class="hl">{</span></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;">unrelated change</pre>
</blockquote>
<p>On June 23rd, 2013, 9:15 p.m. UTC, <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;">Well, i changed the function so i took the liberty of updating the code style for that function as well. Do you mind if i keep this in?</pre>
</blockquote>
<p>On June 23rd, 2013, 9:45 p.m. UTC, <b>Milian Wolff</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;">In general I think this is frown upon (I do, at least). Thing is, you then uglify your patch and hide the actual interesting logic in potentially many of these "cleanup" changes. Separate the two so the git history will show something like:
patch1: improve performance
patch2: cleanup</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;">For just 2 brackets in dead code?</pre>
<br />
<p>- Mark</p>
<br />
<p>On June 23rd, 2013, 9:21 p.m. UTC, Mark Gaiser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, David Faure and Frank Reininghaus.</div>
<div>By Mark Gaiser.</div>
<p style="color: grey;"><i>Updated June 23, 2013, 9:21 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;">Hi,
I've recently seen Frank Reininghaus do his best in speeding up the rendering in dolphin with regards to the app icons. And trying to prevent icon flickering between "unknown" and the actual icon.
While reading his posts on the mailing list i was beginning to wonder: "is fast mime detection actually fast"? While it was certainly faster then "slow" mime detection, it still didn't really seem fast to me. A small benchmark app hat ran fast mime detection in /usr/bin took ~40ms to complete. That's for just 2656 items.
After quite a bit of profiling i managed to to bring the duration down from ~40ms to ~3ms sometimes ~4ms. That's well over 10x faster.
Mime detection by extension (like "file.tar.bz") is done as follows:
file.tar.bz
Loop - find first dot
- "tar.gz"
if that matches a mime type then it's returned if it doesn't then it proceeds on to the next dot:
- next dot: "gz"
if that matches.. return.
Otherwise it will return the default mime type.
I am getting an inconsistency. Using the unpatched fast mime detection on a file like: "test.tar.gz" gets detected as "application-x-compressed-tar" where the patched version detects it as "application-gzip". The slow and detailed mime detection detects the same file as "application-x-compressed-tar". What should it be? application-gzip or application-x-compressed-tar?
Note: This improved detection does expect folders to end with a "/". Otherwise they will be detected as application-octet-stream (the default). But i think this is common sense to let folders end with a "/". If any apps that don't do that, they should fix it i suppose.
Best thing, it's all internal and private api change. No public function is changed.
All feedback is welcome! If possible, i would like to put this in KDE 4.11.</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;">Tested this using just output comparison between the old version and the new implementation. It works just fine.
kurlmimetest output:
********* Start testing of KUrlMimeTest *********
Config: Using QTest library 4.8.4, Qt 4.8.4
PASS : KUrlMimeTest::initTestCase()
PASS : KUrlMimeTest::testURLList()
PASS : KUrlMimeTest::testOneURL()
PASS : KUrlMimeTest::testFromQUrl()
PASS : KUrlMimeTest::testMostLocalUrlList()
PASS : KUrlMimeTest::cleanupTestCase()
Totals: 6 passed, 0 failed, 0 skipped
********* Finished testing of KUrlMimeTest *********
</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/services/kmimetype.h <span style="color: grey">(bc35bcf)</span></li>
<li>kdecore/services/kmimetype.cpp <span style="color: grey">(d748523)</span></li>
<li>kdecore/services/kmimetyperepository.cpp <span style="color: grey">(f56f48e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111050/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>