Review Request 112725: Save memory and time by lazy-loading the item data if possible

[Frank Reininghaus]

> On Sept. 14, 2013, 11 p.m., Mark Gaiser wrote:
> > Hi Frank,
> > 
> > I wonder who said that having folders with 100.000 entries is insane? ;) Either You or David said that to me when i first brought up massive folders and optimization ideas for it. It's very nice to see that you're now optimizing for those cases as well! Good job and keep hunting those resources.
> > 
> > Now for the more constructive feedback - or rather, an idea where you might find much more memory being wasted where i haven't found a good solution yet. With massive folders the UDSEntry object is stored for each object. In your case (since you're working from the higher level classes) the UDSEntry is stored within KFileItem. I think you can save a lot of memory in that class by making it smarter or different. KFileItems also has a bunch of private members to store the same info from UDSEntry thus i can imagine that quite a lot of data is being duplicated there.
> > 
> > Good luck in your resource hunting :)

I think that both David and I were a bit skeptical about the "500,000 files in one folder" thing ;-) I still think that optimizing for this case might not be the best thing to do if it causes a very significant cost in terms of code complexity. However, there are far more opportunities for small changes like the one I've proposed here than I first thought, which do not require massive code modifications, but still reduce our resource usage in all cases (also if there are far less files in one folder).

And I admit that opening large folders is indeed the best way to find out about such opportunities, and also to measure how much we gain by a change, so your input in the "massive folder" area was very useful, more useful than I first thought!

About the UDSEntry issue: Yes, I've also noticed that this is one of the major consumers of memory, but improving this is probably not quite trivial, so I've focused on things where we can gain something with less effort inside Dolphin for the time being. In the long term, a way to have most information about files (i.e., everything that is not needed for sorting) lazy-loaded probably makes sense.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112725/#review40039
-----------------------------------------------------------


On Sept. 14, 2013, 2:08 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112725/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2013, 2:08 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> This patch tries to prevent calling KFileItemModel::retrieveData() when possible in order to save memory and time when loading huge directories.
> 
> 
> There are changes in the following members of KFileItemModel:
> 
> * data() and setData() are modified such that the QHash "values" for an item is initialized with retrieveData() if that has not happened already.
> 
> * indexForKeyboardSearch() uses the "text" from the KFileItem itself, rather than looking than calling data() (which can be expensive if called for many items because data() might call retrieveData()).
> 
> * isExpandable() calls data(), rather than accessing the QHash "values" directly. If this hash was not initialized it, isExpandable() would always return the default value "false".
> 
> * createItemDataList() does not call retrieveData() for each item any more. However, if the sort role is such that sortRoleCompare() would try to access the QHash "values" when sorting, retrieveData() is called to ensure that sorting works correctly.
> 
> 
> Things that could still be improved:
> 
> * Currently, the layouting in Compact View calls KFileItemModel::data() for every item. In the case that only the "Name" is shown, this could be prevented (similar to the approach that is used in Icons View, see https://git.reviewboard.kde.org/r/112253/
> 
> * When changing the roles (e.g., by adding or removing columns in Details View) or changing the sort role, retrieveData() is currently called for all items. Moreover, a changedItems signal is issued for all items, such that KFileItemModelRolesUpdater tries to load icons/previews for all items (like in Dolphin < 4.11).
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 51ff142 
> 
> Diff: http://git.reviewboard.kde.org/r/112725/diff/
> 
> 
> Testing
> -------
> 
> I opened a directory with 100,000 files in Icons View and Details View with a release build, and measured the memory usage with KSysGuard. Moreover, I measured the time required to load the directory using the following patch for DolphinView: http://paste.kde.org/pae0a64c1/
> 
> The time required for the initial loading is not very meaningful because it varies strongly. We can get a better idea of how much time is spent inside Dolphin for loading if all items are cached, so I re-loaded the folder 8 of times by pressing Alt+Up and then Alt+Left.
> 
> In Icons View, the memory consumption is reduced from 198.2 MB to 164.6 MB (-17%), and the average time from 2898 ms to 2814 ms (-2.9%).
> 
> In Details View, the memory consumption is reduced from 225.1 MB to 166.6 MB (-26%), and the average time from 1539 ms to 1076 ms (-30%).
> 
> The savings are larger in Details View because retrieveData() stores more information about the items in the QHash "values" (like the "expanded parents count").
> 
> I could not see any significant differences in Compace View (as expected).
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130916/faedbef8/attachment.htm>