Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118775/ --- (Updated June 21, 2014, 7:39 a.m.) Status -- This change has been marked as submitted. 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- 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. Changes --- Implemented David's and Emmanuel's suggestions (which make a lot of sense, thanks!). 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 QListKFileItem 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 (updated) - 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118775/#review60568 --- Ship it! (again, with the agreement that the dirlister solution is temporary - it makes KCoreDirLister::itemsForDir slower) Thanks! - David Faure 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
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? Frank Reininghaus wrote: Maybe - I'm always a bit unsure which C++11 features are allowed. This one is unfortunately not allowed... - Kevin --- 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118775/#review60306 --- I agree with the others, good temporary solution for the API, I just hope it is only temporary. Two minor issues below. src/core/kcoredirlister_p.h https://git.reviewboard.kde.org/r/118775/#comment42042 This is worth at least a comment in the code src/core/kcoredirlister_p.h https://git.reviewboard.kde.org/r/118775/#comment42041 Should be const. Looks slow, too, so maybe this conversion should be explicit, so we can see where it happens? - David Faure 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118775/#review60179
---
src/core/kcoredirlister_p.h
https://git.reviewboard.kde.org/r/118775/#comment41931
using KFileItem::KFileItem; maybe?
src/core/kcoredirlister_p.h
https://git.reviewboard.kde.org/r/118775/#comment41932
{}
- Emmanuel Pescosta
On June 16, 2014, 9: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, 9: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
QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118775/#review60180 --- Pretty ugly, but I'd say this is a good first take as we'd then have it movable (the public ABI change) and could then try to improve the private code around the NonMovable... stuff later on. I find it rather strange that raw pointers are used here. I guess a more thorough refactoring of the code could help. I.e. having a _cache_ thats also used to modify something is odd. Rather, I'd expect that a edit operation would invalidate/update the cache as needed. So in the best case, one would just have lists/maps of const KFileItem - so pure value semantics without any pointers. - Milian Wolff 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 QListKFileItem 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
Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
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 QListKFileItem 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
