Review Request 118775: Make KFileItem a Q_MOVABLE type

[Commit Hook]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118775/#review60639
-----------------------------------------------------------


This review has been submitted with commit 4b498196899223692e8a7d334618b2874bb6c77e by Frank Reininghaus to branch master.

- Commit Hook


On June 19, 2014, 2:47 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118775/
> -----------------------------------------------------------
> 
> (Updated June 19, 2014, 2:47 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   src/core/kcoredirlister.cpp fef28db 
>   src/core/kcoredirlister_p.h 2660e99 
>   src/core/kfileitem.h bc2f90c 
> 
> Diff: https://git.reviewboard.kde.org/r/118775/diff/
> 
> 
> Testing
> -------
> 
> Unit tests still pass. I verified that the memory usage of a KFileItemList with many items decreases as expected.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140621/ed1172f2/attachment.html>