Review Request 111700: Do not waste resources by converting a KUrl to a QString and back again in KFileItemModel::resortAllItems()

[Commit Hook]

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


This review has been submitted with commit bf2618d7cfb0bdadf28658a75654d4b7976c9924 by Frank Reininghaus to branch KDE/4.11.

- Commit Hook


On July 25, 2013, 9:07 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111700/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 9:07 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Actually, I think that the master branch is the right target for performance improvements at this point. However, I found a rather trivial error that wastes a lot of CPU cycles in some situations, and I think it's worth considering for 4.11.
> 
> The problem is in KFileItemModel::resortAllItems(). The function first creates a list of the KUrls of all items in the old order. After the sorting, it looks up the new indexes of these items to be able to tell the view and the selection manager about which item moved where. The problem is in the line
> 
> const int newIndex = m_items.value(oldUrls.at(i).url())
> 
> Note that m_items is a QHash<KUrl, int>, and oldUrls is a QList<KUrl>. Calling oldUrls.at(i).url() converts the KUrl to a QString (see KUrl::url()). Nonetheless, looking up this QString in m_items works because KUrl has a constructor which takes a QString. So the code works as expected, but the conversion from KUrl to QString and back is quite expensive. Therefore, I think that we should remove it.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp d174cf6 
> 
> Diff: http://git.reviewboard.kde.org/r/111700/diff/
> 
> 
> Testing
> -------
> 
> I opened a directory with 100,000 files in Details View, "Sort by Name", natural sorting disabled. I enabled the benchmarking output in KFileItemModel::resortAllItems() and changed the sort order a couple of times by clicking the header of the "Name" column.
> 
> This is the output (all times in milliseconds) without the patch:
> 
> 
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 1705
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 1485
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 1496
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 1430
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 1447
> 
> 
> With the patch:
> 
> 
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 753
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 492
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 498
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 535
> ===========================================================
> Resorting 100000 items
> [TIME] Resorting of 100000 items: 533
> 
> 
> Unit tests still pass, and I haven't found any regressions so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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