<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127763/">https://git.reviewboard.kde.org/r/127763/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 28th, 2016, 9:22 a.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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">could you also have a look at it with heaptrack? it should instantly give you a total number of allocations after finish, which should already give us an indication whether this brings lots to the table. Your callgrind numbers seem to indicate so though. I just feel a bit uneasy with lazy-constructing the string without caching it. If that is happening a lot, you may want to cache the result</p></pre>
</blockquote>
<p>On April 28th, 2016, 12:37 p.m. UTC, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll upload the heaptrack files, TBH I don't see a big improvement there (using dolphin, on plasmashell it will definitely show, I guess I should have tested that one :D).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please note that while you're right, you're also wrong. The string wasn't being used as is, on ::iconPath it needed to have the file appended as well, so the construction at runtime happened as well. The only difference is that we're constructing the path from 3 sources rather than 2.</p></pre>
</blockquote>
<p>On April 28th, 2016, 12:45 p.m. UTC, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It went from ~500k allocations to ~450k.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can't download the files for some reason, I get a server error :-/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But this sounds like a good improvement already and funnily enough maps directly to the 10% you measured with valgrind :)</p></pre>
<br />
<p>- Milian</p>
<br />
<p>On April 28th, 2016, 12:39 p.m. UTC, Aleix Pol Gonzalez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Christoph Feck.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated April 28, 2016, 12:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kiconthemes
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Leverages the implicitly shared nature of QString by only constructing the paths when it's strictly necessary. It souldn't show a big performance impact since the generated paths were only intermediate in most cases after all.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is especially important now that plasma has several KIconLoader instances at the same time, but it should have an impact on every application using kiconthemes (or under the plasma-integration effects).</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tests still pass, apps still work.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Starting and closing dolphin under callgrind showed 1640M instructions, now 1466M(10% improvement).</p></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>src/kicontheme.cpp <span style="color: grey">(735ecf7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127763/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/3f9b64d7-58e0-454b-9774-6ec86aa7f0c3__heaptrack.dolphin.16572-after.gz">after</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/6229961c-bf00-4959-9fc1-9effa63b2b34__heaptrack.dolphin.16861-before.gz">before</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>