<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/115739/">https://git.reviewboard.kde.org/r/115739/</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 13th, 2014, 9:31 p.m. UTC, <b>Christoph Feck</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;">Making the type movable does not make QList store it directly, how did you check this?
http://qt-project.org/doc/qt-5/qlist.html says:
"Internally, QList<T> is represented as an array of pointers to items of type T. If T is itself a pointer type or a basic type that is no larger than a pointer, or if T is one of Qt's shared classes, then QList<T> stores the items directly in the pointer array."</pre>
</blockquote>
<p>On February 13th, 2014, 9:33 p.m. UTC, <b>Christoph Feck</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;">Wait, UDSEntry is just a pointer, right?</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;">Yes, it is a pointer to a UDSEntryPrivate. Thanks for bringing this issue up!</pre>
<br />
<p>- Frank</p>
<br />
<p>On February 13th, 2014, 8:23 p.m. UTC, Frank Reininghaus 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 KDE Frameworks and David Faure.</div>
<div>By Frank Reininghaus.</div>
<p style="color: grey;"><i>Updated Feb. 13, 2014, 8:23 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">I'm continuing my efforts to make UDSEntry more efficient, which were started in https://git.reviewboard.kde.org/r/113591/. This is the second step, and I'll probably do more in the future, for which I will split https://git.reviewboard.kde.org/r/113355/ into independent parts.
This patch contains the following changes (which are separate commits):
1. Make UDSEntry a Q_MOVABLE type. This has the effect that a QList<UDSEntry> a.k.a. UDSEntryList will store the actual entries in a single allocated block of memory, and not pointers to UDSEntries which are allocated individually on the heap (this means that this change is binary incompatible). This reduces the memory usage by 32 bytes per UDSEntry in a QList because each memory allocation uses at least 32 bytes on a 64-bit system.
This idea is similar to what I proposed for KFileItem in https://git.reviewboard.kde.org/r/111789/. That one had to be reverted later though because it turned out that KDirLister does rely on QList<KFileItem> storing only pointers to the KFileItems. I'm confident that no such trouble will happen for UDSEntry - all KIO unit tests still pass.
One could argue that we could simply use a UDSEntryVector instead of a UDSEntyList to achieve the same memory savings, but considering that the list is used quite frequently ( http://lxr.kde.org/ident?i=UDSEntryList ), this might require a lot of porting work and cause other unexpected problems.
Note that I could not make the Q_DECLARE_TYPEINFO declaration work if it was inside the KIO namespace, but I still preferred to do it immediately after the class declaration, so I had to interrupt the namespace briefly.
2. Add some benchmarks to measure how long typical UDSEntry operations take: add fields to an entry, read fields from an entry, store a UDSEntryList in a QByteArray, and read it back from the QByteArray.
All measurements are done both for UDSEntries with 8 fields (this is a rather typical use case because kio_file usually creates 8 fields), and for entries with the maximum number of fields.
3. Add a simple manual test ('listjobtest') that runs a KIO::ListJob for a given URL. This can be used to easily measure the memory usage of the UDSEntryList that contains an entry for each file at that URL.
</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;">1. KIO unit tests still pass.
2. I've confirmed with the new 'listjobtest' and KSysGuard that the memory usage is really lowered by 32 bytes per item in a UDSEntryList.
3. The benchmark results do not change significantly. I only tested it with a debug build (i.e., with optimizations disabled) though, and I'm afraid I might be lacking the resources to set up an additional build of Qt5 and all of KF5 in release mode. However, since UDSEntry essentially only depends on qtbase, I might be able to just do a release build of qtbase and build a stand-alone version of UDSEntry+benchmarks on top of that. I'll look into this option for getting reliable benchmark results when I work on further improvements in the future.</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/core/udsentry.h <span style="color: grey">(9550a7e)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(dbca6a5)</span></li>
<li>tests/listjobtest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/udsentrybenchmark.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115739/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>