<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="https://git.reviewboard.kde.org/r/118587/">https://git.reviewboard.kde.org/r/118587/</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 6th, 2014, 10:59 a.m. UTC, <b>David Faure</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="https://git.reviewboard.kde.org/r/118587/diff/1/?file=279294#file279294line36" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/config/bufferfragment_p.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n"><span class="hl">KConfigIniBackend</span></span><span class="o"><span class="hl">::</span></span><span class="n">BufferFragment</span><span class="ew"> </span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">BufferFragment</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;">Why? Now it could clash with an application class BufferFragment, on a system without "hidden visibility". Or it might confuse gdb, even with hidden-visibility? not sure how that works.</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;">Because I need to access it from free functions (qHash, lookup). I can also make it a public if you prefer to keep the namespace. Is that OK?</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 6th, 2014, 10:59 a.m. UTC, <b>David Faure</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="https://git.reviewboard.kde.org/r/118587/diff/1/?file=279295#file279295line109" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/config/kconfigini.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">KConfigIniBackend::parseConfig(const QByteArray& currentLocale, KEntryMap& entryMap,</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">109</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// This is often the case, especially sub groups will all have</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;">This isn't always the case though. kmail does that, yes, but e.g. kdeglobals doesn't.
Nor most desktop files, nor konquerorrc (except that some video player added per-file groups into mine!).
What do we lose if the config file has no repeated keys? Memory (the hash) and CPU (inserting and looking up into the hash), right?
I have a hard time being sure that the tradeoff is always in our favour.</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;">Yes, the tradeoff is the memory of the hash-internal structure (which is temporary!).
Looking at my konquerorrc, I still see tons of shared strings: false and true :) Then all the stuff in the toolbar configuration, esp. the keys used there.
Also: konquerorrc and other config or desktop files are small. It might be slowed down by this patch, but not by much. It will not become a hotspot suddenly. But the cases where this already is an issue right now, this patch helps a lot.</pre>
<br />
<p>- Milian</p>
<br />
<p>On June 6th, 2014, 9:31 a.m. UTC, Milian Wolff wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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 and David Faure.</div>
<div>By Milian Wolff.</div>
<p style="color: grey;"><i>Updated June 6, 2014, 9:31 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">Optimize KConfigIniBackend::parseConfig by reducing allocations.
Yet another awesome application of the Qt implicit sharing trick.
Since config files often contain only few different keys and even
value strings, we can share them. This reduces memory consumption
and also speeds up parsing, as we do not have to allocate the
duplicated strings, but can simply reuse the previous values.
The most extreme case for this of my knowledge, is KatePart:
katesyntaxhighlightingrc has more then 20k lines which triggered
about 30k allocations on startup. With this patch applied, this
value goes down dramatically. I added a simple static counter for
the cache hit/miss ratio, which resulted in 5442 cache misses compared
to 43624 cache hits across all KConfig files parsed by kwrite on startup.
</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;">Unit tests all pass. My malloc tracer shows that the allocations are all gone.
My malloc tracer showed before:
17421 allocations at:
0x7fee73692b97 QByteArray::QByteArray(char const*, int) /usr/lib/libQtCore.so.4
0x7fee73bb7cee ? /usr/lib/libkdecore.so.5
0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags<KConfig::OpenFlag>, char const*) /usr/lib/libkdecore.so.5
0x7fee64830c06 KateHlManager::KateHlManager() in /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
12699 allocations at:
0x7fee73692b97 QByteArray::QByteArray(char const*, int) /usr/lib/libQtCore.so.4
0x7fee73bb7cd7 ? /usr/lib/libkdecore.so.5
0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags<KConfig::OpenFlag>, char const*) /usr/lib/libkdecore.so.5
0x7fee64830c06 KateHlManager::KateHlManager() in /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
These where the allocation hotspots number #1 and #3 respectively. With the patch applied, the two locations are not under the top 10 anymore.</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/config/bufferfragment_p.h <span style="color: grey">(5a753ad)</span></li>
<li>kdecore/config/kconfigini.cpp <span style="color: grey">(8ec43c5)</span></li>
<li>kdecore/config/kconfigini_p.h <span style="color: grey">(d7aa43e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118587/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>