Review Request 121480: use QCollator instead of KStringHandler

[Frank Reininghaus]

> On Dez. 13, 2014, 9:54 nachm., Mark Gaiser wrote:
> > Hi,
> > 
> > I invite you to read my blog post about exactly this subject [1]. The simple conclusion: QCollatorSortKey is faster in all cases even with bookkeeping.
> > 
> > As you probably know, a lot of effort has been spend by numerous people to make specifically the sorting operation faster. That didn't change KStringHandler because there was no successor for it at that time. The work for QCollator was known back then and it was remain to be seen if it would be a replacement for KStringHandler. Just using QCollator is the most horrible option when it comes to performace.
> > 
> > I would really recommend against moving to just QCollator. If you move to those classes, use QCollatorSortKey.
> > 
> > [1] http://kdeblog.mageprojects.com/2014/07/06/natural-string-sorting-an-in-depth-analysis/
> 
> Emmanuel Pescosta wrote:
>     Hi Mark, thanks for the info!
>     
>     The aim of this patch is to move Dolphin a little bit more away from KDELibs4Support, so I decided to leave the QCollatorSortKey optimization away for now. (would blow up the number of changes immensely)
>     
>     We can focus on optimizations (also includes QCollatorSortKey) after Dolphin is KDELibs4Support free and freed of porting bugs.
>     
>     > Just using QCollator is the most horrible option when it comes to performace.
>     
>     As far as I know the current KStringHandler natural compare also makes use of QCollator. (Wasn't there a RR some time ago?!)

I think that KStringHandler::naturalCompare does not use QCollator, it's still mostly the same as in kdelibs AFAIK. Surprisingly, using QCollator instead of KStringhandler actually seems to cause a crash: https://bugs.kde.org/show_bug.cgi?id=342012 - apparently, QCollator isn't thread-safe. I didn't know that at all, but actually, the docs say that it's reentrant only: https://qt-project.org/doc/qt-5-snapshot/qcollator.html. This is really surprising to me - I had thought that QCollator::compare is a suitable drop-in replacement for KStringHandler::naturalCompare :-/

That would mean that each thread needs its own QCollator object. Alternatively, one could protect all uses of the collator with a mutex, but that would defeat the purpose of multi-threaded sorting.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121480/#review71936
-----------------------------------------------------------


On Dez. 17, 2014, 6:17 nachm., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121480/
> -----------------------------------------------------------
> 
> (Updated Dez. 17, 2014, 6:17 nachm.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Port away from KStringHandler naturalCompare to QCollator compare.
> 
> Make use of the QCollator instance (m_collator) in nameRoleGroups instead of using QString::localeAwareCompare (uses QCollator internally) to realise a locale aware less than.
> 
> We don't need the m_caseSensitivity anymore, because the QCollator instance also holds this information.
> 
> Removed the KFileItem::name to lower conversion because this makes the case sensitive fallback sorting in stringCompare useless.
> 
> (This patch doesn't make use of QCollatorSortKey)
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.h d98d453 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 711b079 
> 
> Diff: https://git.reviewboard.kde.org/r/121480/diff/
> 
> 
> Testing
> -------
> 
> Natural sorting works
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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