Review Request 121480: use QCollator instead of KStringHandler

[Emmanuel Pescosta]

> On Dec. 13, 2014, 10:54 p.m., 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?!)
> 
> Frank Reininghaus wrote:
>     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.
> 
> Mark Gaiser wrote:
>     That's some interesting information, Frank.
>     
>     Then i see a few options since QCollator::compare is apparently not a suitable solution for dolphin because of it's multithreading.
>     
>     1. QCollator per thread. Not ideal for performance reasons.
>     2. Pre generate QCollatorSortKey and use that instead. Should be thread safe since i've used that quite a lot in a threaded environment.
>     3. Copy KStringHandler::naturalCompare - which is still the same as the kdelibs4 version - to dolphin and maintain it internally. Then revert back to KStringHandler::naturalCompare.

> Wasn't there a RR some time ago?! 

RR 112717

Hmm the problem is, that I can't reproduce the crash here?!

So how should we proceed?
* Store a copy of the collator object in KFileItemModelLessThan, add a clone method to KFileItemModelLessThan which should be called whenever a new sorting thread gets created and pass the thread local collator object on to lessThan, sortRoleCompare and stringCompare?
* Disable the parallelMergeSort until we use sorting keys?


- Emmanuel


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


On Dec. 17, 2014, 7:17 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121480/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2014, 7:17 p.m.)
> 
> 
> 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/20141219/07b382b4/attachment.htm>