<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/118775/">https://git.reviewboard.kde.org/r/118775/</a>
</td>
</tr>
</table>
<br />
<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>
<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;">This is my second attempt to make KFileItem a Q_MOVABLE_TYPE, after
https://git.reviewboard.kde.org/r/111789/
which I had to revert because it broke KDirLister, see
http://mail.kde.org/pipermail/kde-frameworks-devel/2013-August/004135.html
The motivation for this change is that this reduces the memory usage of a QList<KFileItem> a.k.a. KFileItemList, which is used extensively in KDirLister's API and in applications, by 32 bytes per item on a 64-bit system, and that it may also improve the performance in some situations because many memory allocations are saved (for details on why making a type "movable" saves memory allocations when putting objects of that type into a QList, see the discussion in the related request https://git.reviewboard.kde.org/r/115739/ for UDSEntry).
The problem with the first attempt was that KDirListerCache actually relies on the fact that KFileItem is NOT movable in memory - it keeps pointers to KFileItems in a QList and expects that these pointers remain valid even if the list is resized, and the location of its contiguous data storage with size ~"number of items in the list" in memory changes. This is the case right now because QList only keeps pointers to the KFileItems, and moving the pointers when the list is resized does not change the location of the actual KFileItems. For "movable" types, QList stores the objects directly, such that resizing the list may move the actual KFileItems. This conflicts with KDirListerCache's expectation that the KFileItems do not move.
David suggested to change the internal data storage of KDirListerCache to, e.g., a QLinkedList to circumvent this problem, see
http://mail.kde.org/pipermail/kde-frameworks-devel/2013-September/004845.html
I have a less intrusive proposal now: Make KFileItem movable, but replace all places where KDirListerCache expects a non-movable KFileItem with "NonMovableFileItem", which is a class that inherits KFileItem, but does not have the "movable" property.
That way, the data storage inside KDirListerCache remains exactly the same, and everything outside that class benefits from the movability of KFileItems. Most changes in this patch are straightforward subsitutions.
The only place where performance might suffer is KCoreDirLister::itemsForDir(const QUrl &dir, WhichItems which) in the "which == AllItems" case. The current code simply returns a shallow copy of the internal KFileItemList, but with this patch, the list has to be copied item by item (this happens in NonMovableFileItemList::operator KFileItemList()). However, the QLinkedList idea or any other approach which makes KFileItem movable, but keeps the KFileItems in KDirListerCache at fixed memory locations would suffer from the same problem.
I'm not sure if that function is used much in the AllItems case though. I put a "Q_ASSERT(0);" into NonMovableFileItemList::operator KFileItemList() and was unable to trigger that assert with Dolphin.
Ideally, one would do some benchmarking and memory profiling of this patch and alternatives, such as the QLinkedList idea. However, I'm running out of time because the release schedule is progressing fast, and even though this change is quite straightforward, it is binary incompatible. This is why I am creating this review request right now.</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 still pass. I verified that the memory usage of a KFileItemList with many items decreases as expected.</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/kcoredirlister.cpp <span style="color: grey">(fef28db)</span></li>
<li>src/core/kcoredirlister_p.h <span style="color: grey">(2660e99)</span></li>
<li>src/core/kfileitem.h <span style="color: grey">(bc2f90c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118775/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>