D10857: Change qSort to std::sort

This revision was not accepted when it landed; it landed in state "Changes Planned". This revision was automatically updated to reflect the committed changes. Closed by commit R241:7bb346ac2a4d: Change qSort to std::sort (authored by jtamate). CHANGED PRIOR TO COMMIT https://phabricator.kde.or

D10857: Change qSort to std::sort

jtamate retitled this revision from "Change qSort to std::sort in simplifiedUrlList" to "Change qSort to std::sort". jtamate edited the summary of this revision. jtamate edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10857 To: jtamat

D10857: Change qSort to std::sort in simplifiedUrlList

markg added a comment. In D10857#216725 , @dfaure wrote: > I cannot confirm that stable_sort is faster, on the contrary. http://www.davidfaure.fr/2018/qsort_performance.cpp updated, says (repeatedly) > "std::sort took: 5003 ms" > "std::stab

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate added a comment. In D10857#216725 , @dfaure wrote: > I cannot confirm that stable_sort is faster, on the contrary. http://www.davidfaure.fr/2018/qsort_performance.cpp updated, says (repeatedly) > "std::sort took: 5003 ms" > "std::st

D10857: Change qSort to std::sort in simplifiedUrlList

dfaure added a comment. I cannot confirm that stable_sort is faster, on the contrary. http://www.davidfaure.fr/2018/qsort_performance.cpp updated, says (repeatedly) "std::sort took: 5003 ms" "std::stable_sort took: 7490 ms" Maybe on specific data (the actual filenames you're testing t

D10857: Change qSort to std::sort in simplifiedUrlList

dfaure added a comment. That is amazing, I could have sworn that the whole point of sort vs stable_sort was that stable_sort was potentially slower, which was the reason for sort to exist (when you don't care about the "stability" of equal items)... REPOSITORY R241 KIO REVISION DETAIL h

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate added a comment. I've found the problem. The problem is qSort itself. Chaning qSort to qStableSort I've got normal times in the test case, select 50.000 files and pressing Shift+Supr. Also in the small test, the times, running under callgrind are: qSort: between 33148 and

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate planned changes to this revision. jtamate added a comment. I've run David test, it is not a CPU problem: g++ -fPIC -O2 qsort_performance.cpp -I /usr/include/qt5/QtCore -I/usr/include/qt5 -l Qt5Core ./a.out "qSort took: 3335 ms" "std::sort took: 3452 ms" "qSort took: 3285 m

D10857: Change qSort to std::sort in simplifiedUrlList

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. " it does suck a little that qSort beats std::sort." I was curious whether that was true, so I ran Mark's benchmark, with a few fixes: one more zero in the number of items for the vectors,

D10857: Change qSort to std::sort in simplifiedUrlList

markg accepted this revision. markg added a comment. +1, but wait till someone else had a look as well. INLINE COMMENTS > kdirmodel.cpp:42 > > -#include > - Don't you need to keep that one? Nothing seems to be including (for std::sort at the very least) so i'm not sure why it would work

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate updated this revision to Diff 28274. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Changed all qSort in Qlist to std::sort. At least the people with Intel(R) Core(TM) i5-4200U (model 69) will see a big improvemen

D10857: Change qSort to std::sort in simplifiedUrlList

markg added a comment. In D10857#214867 , @jtamate wrote: > In D10857#214607 , @dfaure wrote: > > > I'm not opposed to the idea, but measuring CPU usage is a rather misleading indicator. What if it t

D10857: Change qSort to std::sort in simplifiedUrlList

apol added a comment. I'd say the bottom line is qSort is deprecated in favor of std::sort. @jtamate make sure you are profiling release builds. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10857 To: jtamate, #frameworks, dfaure Cc: markg, apol, michaelh

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate added a comment. In D10857#214607 , @dfaure wrote: > I'm not opposed to the idea, but measuring CPU usage is a rather misleading indicator. What if it takes 10 times longer, because it's progressing much more slowly? ;) > > Please at

D10857: Change qSort to std::sort in simplifiedUrlList

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. I'm not opposed to the idea, but measuring CPU usage is a rather misleading indicator. What if it takes 10 times longer, because it's progressing much more slowly? ;) Please

D10857: Change qSort to std::sort in simplifiedUrlList

markg added a comment. +1, also for what @apol said. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10857 To: jtamate, #frameworks, dfaure Cc: markg, apol, michaelh

D10857: Change qSort to std::sort in simplifiedUrlList

apol added a comment. +1 If there's 11, we better change them all at once? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10857 To: jtamate, #frameworks, dfaure Cc: apol, michaelh

D10857: Change qSort to std::sort in simplifiedUrlList

jtamate created this revision. jtamate added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY qSort is depecreated in Qt5. But qSort is also quite slow compared to std::sort. There are 11 more us