> On June 16, 2014, 10:09 a.m., Emmanuel Pescosta wrote: > > src/core/kcoredirlister_p.h, lines 40-42 > > <https://git.reviewboard.kde.org/r/118775/diff/1/?file=281658#file281658line40> > > > > "using KFileItem::KFileItem;" maybe?
Maybe - I'm always a bit unsure which C++11 features are allowed. - Frank ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118775/#review60179 ----------------------------------------------------------- On June 16, 2014, 7:58 a.m., Frank Reininghaus wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118775/ > ----------------------------------------------------------- > > (Updated June 16, 2014, 7:58 a.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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
