[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 118775: Make KFileItem a Q_MOVABLE type
From: "Kevin Ottens" <ervin () kde ! org>
Date: 2014-06-17 16:31:41
Message-ID: 20140617163141.3733.63253 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118775/#review60290
-----------------------------------------------------------
Looks good to me for a "let's get the ABI change in, and improve internals later" \
approach. Let's wait for David's opinion though.
- Kevin Ottens
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
>
>
[Attachment #5 (text/html)]
<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good to me for a \
"let's get the ABI change in, and improve internals later" approach. \
Let's wait for David's opinion though.</pre> <br />
<p>- Kevin Ottens</p>
<br />
<p>On June 16th, 2014, 7:58 a.m. UTC, Frank Reininghaus wrote:</p>
<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>
<p style="color: grey;"><i>Updated June 16, 2014, 7:58 a.m.</i></p>
<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>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic