<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, 7:29 p.m. UTC, <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;">> "KMimeTypePrivate::iconName" is now the one slowing everything down mostly because of the QHash.
Sure? Try replacing QHash with QMap (QHash is very fast at lookup with fix base and no variable costs - but it's slower on insertion)
I'd however blame KMimeTypePrivate::ensureXmlDataLoaded() here (callgrind will know for sure, btw ;-)</pre>
</blockquote>
<p>On June 23rd, 2013, 9:14 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;">Which i just tested to be sure :)
Just for fun:
- completely commenting out ensureXmlDataLoaded() made no difference thus leading me to thing that the hash is slowing something down
- replacing QHash by QMap also didn't change much. If any, it made it slightly slower
- completely disabling the cache (so only iconNameFromCache and all that uses it) did matter and made everything 1/3rd faster
So i think the caching is not helping much in performance in this case.</pre>
</blockquote>
<p>On June 23rd, 2013, 9:41 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;">Which will greatly depend on how you designed your benchmark. What is callgrind exactly showing as culprit? Could you upload the file somewhere?
You should also accompany this patch with a proper QBENCHMARK btw. This then ensures we can verify the speed improvements (i.e. do proper before/after assessments.) Furthermore it will enable others to look at the speed and make improvements without first writing a benchmark.</pre>
</blockquote>
<p>On June 23rd, 2013, 10:02 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 can provide the callgrind if you want to see that. I'm not going to make a QBENCHMARK because this code is dead! It will only live for as long as KDE 4.11 lives and whatever comes after that in the 4.xx series. The "next" KDE will use QMimeType. My "benchmark" right now consists of simply getting the mime types of all files in /usr/bin. Only by name. You can find the benchmark here: http://paste.kde.org/781292/
Note: this is very much real world like! Just browse to /usr/bin in dolphin or a file chooser dialog and you have the same case.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not entirely sure, but the caching mechanism seems to catch the condition of similar but different mimetypes with equal icons (bypassing superfluous singular ensureXmlDataLoaded() calls for that object, yesno?)
But esp /usr/bin has -here- exactly 5(!) filetypes: shellscript, perl script, python script, directory and of course ELF binary - 6 if you count int symlinks. For 2227 entries!
I can't really say, but it does not seem a reasonable test object to me at all - at least compared to, say, $HOME or, worse, $HOME/Downloads (which I have to admit is a real mess of a grabbag here ;-)
Also there seems a gap, as you claim ensureXmlDataLoaded() to have virtually no impact, and omitting the cache makes it "1/3rd faster" while (i assume) simply a direct return of m_iconName would make it "5x faster" and despite the imprecision of slang terms like "1/3rd faster" and "5x faster" - doesn't this imply most time is lost in the string operations there??
Have you checked the ratio of actual cache hits and misses?
And if you already got a callgrind dump: feel free to share anytime =)</pre>
<br />
<p>- Thomas</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>