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/
-----------------------------------------------------------

(Updated July 28, 2013, 9:38 p.m.)


Status
------

This change has been marked as submitted.


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/b73cb498/attachment.htm>