Make KFileItem a Q_MOVABLE type

Review Request #118775 - Created June 16, 2014 and submitted

Information
Frank Reininghaus
kio
master
Reviewers
kdeframeworks
dfaure
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.
Unit tests still pass. I verified that the memory usage of a KFileItemList with many items decreases as expected.

Issues

  • 3
  • 0
  • 0
  • 3
Description From Last Updated
{} Emmanuel Pescosta Emmanuel Pescosta
This is worth at least a comment in the code.... David Faure David Faure
Should be const. Looks slow, too, so maybe this conversion should be explicit, so we can see where it happens? David Faure David Faure
Emmanuel Pescosta
Milian Wolff
Kevin Ottens
David Faure
Frank Reininghaus
Milian Wolff
David Faure
Commit Hook
This review has been submitted with commit 4b498196899223692e8a7d334618b2874bb6c77e by Frank Reininghaus to branch master.
Frank Reininghaus
Review request changed

Status: Closed (submitted)

Loading...