D13814: Speedup sort

bruns added a comment. In D13814#291913 , @jtamate wrote: > Also amended the commit message. thanks, but unfortunately this won't change the git commit message :-( REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D

D13814: Speedup sort

jtamate added a comment. Also amended the commit message. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio, bruns Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, firef, andr

D13814: Speedup sort

jtamate edited the summary of this revision. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio, bruns Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuel

D13814: Speedup sort

bruns added a comment. In D13814#291757 , @jtamate wrote: > In D13814#291644 , @bruns wrote: > > > Any reason why you have pushed this with an obviously wrong commit message? > > > Because I th

D13814: Speedup sort

jtamate added a comment. In D13814#291644 , @bruns wrote: > Any reason why you have pushed this with an obviously wrong commit message? Because I though the commit message was right. What should it be? REPOSITORY R318 Dolphin REVI

D13814: Speedup sort

bruns added a comment. Any reason why you have pushed this with an obviously wrong commit message? REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio, bruns Cc: elvisangelaccio, apol, bruns, markg, kfm-dev

D13814: Speedup sort

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R318:765cc968c9df: Speedup sort (authored by jtamate). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D13814?vs=37559

D13814: Speedup sort

bruns added a comment. In D13814#290556 , @markg wrote: > Is there even a need to have n QCollator objects? > I'm talking about a QCollator _within_ a given view. Each view could obviously have it's own collator due to different view settings.

D13814: Speedup sort

jtamate edited the summary of this revision. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio, bruns Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuel

D13814: Speedup sort

markg added a comment. In D13814#290512 , @bruns wrote: > The code looks fine now, but the summary is incorrect. > > The savings is not from using a lambda, but caused by initializing it once. If the old code had used `m_collator(other.m_coll

D13814: Speedup sort

bruns added a comment. The code looks fine now, but the summary is incorrect. The savings is not from using a lambda, but caused by initializing it once. If the old code had used `m_collator(other.m_collator)` in the copy constructor, construction would have been just a ref count increme

D13814: Speedup sort

jtamate updated this revision to Diff 37559. jtamate edited the summary of this revision. jtamate added a comment. Apply the workaround in loadSortingSettings. It is applied in the constructor and when the type of sorting is changed by the user. REPOSITORY R318 Dolphin CHANGES SINCE LAST

D13814: Speedup sort

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfileitemmodel.cpp:60 > > loadSortingSettings(); > `loadSortingSettings()`, which sets `dirty`, is also called from `slotSortingChoiceChanged()`, so

D13814: Speedup sort

jtamate updated this revision to Diff 37543. jtamate added a comment. I've applied some of @bruns suggestions. Just use QString(), less filesize and memory. Move the comparison after loadSortingSettings(); Change the comment from Force the cleanup of to Force the clean state of. @bruns

D13814: Speedup sort

bruns added inline comments. INLINE COMMENTS > kfileitemmodel.cpp:58 > { > m_collator.setNumericMode(true); > this causes the dirty state - maybe also do the forced (re)initialization here? > kfileitemmodel.cpp:112 > +// Workaround for bug https://bugreports.qt.io/browse/QTBUG-69361

D13814: Speedup sort

elvisangelaccio accepted this revision. elvisangelaccio added a comment. This revision is now accepted and ready to land. Thanks for filing the upstream bug. Looks good to me now. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #framework

D13814: Speedup sort

jtamate updated this revision to Diff 37422. jtamate edited the summary of this revision. jtamate added a comment. Created the bug report referenced in the summary and code. Changed the strings to compare, they can be anything. > Perhaps there is a remote possibility of closing a view wh

D13814: Speedup sort

elvisangelaccio requested changes to this revision. elvisangelaccio added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfileitemmodel.cpp:111 > + > +// warm up the collator so it does not has thread safety problems later > +m_collator.compare(QStringLi

D13814: Speedup sort

elvisangelaccio reopened this revision. This revision is now accepted and ready to land. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarrom

D13814: Speedup sort

elvisangelaccio added a comment. Hmm, so the crash is in `ucol_close()` (aka in ICU) and we are working around it by forcing the cleanup in the KFileItemModel ctor (= single thread). Sounds reasonable, but we should (1) document this hack in the comment and/or in the commit message and (

D13814: Speedup sort

jtamate added a comment. Reading the source of QCollator*, I guess the hack works because I get a collator where d->dirty is true, and in compare there is this code if (d->dirty) d->init(); Once initialized, I don't see any other modification of QCollator members. Probabl

D13814: Speedup sort

jtamate added a comment. In D13814#288615 , @elvisangelaccio wrote: > @jtamate Can you show a complete gdb backtrace + valgrind log of this crash? Here you are F6051257: dolphin.crash.log REPOSITOR

D13814: Speedup sort

elvisangelaccio added a comment. @jtamate Can you show a complete gdb backtrace + valgrind log of this crash? REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio Cc: elvisangelaccio, apol, bruns, markg, kfm

D13814: Speedup sort

markg added a comment. Somehow i'm inclined to think that m_collator is wrong. But inspecting the code shows no issue as far as i can tell. It's a normal class member that lives as long as the KFileItemModel instance lives. Now here's a tricky part. KFileItemModel is new'ed for each vie

D13814: Speedup sort

jtamate updated this revision to Diff 37305. jtamate added a comment. My crashes are gone just doing a single comparison using the collator at the constructor. REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=37273&id=37305 REVISION DETAIL https:

D13814: Speedup sort

jtamate added a comment. >> Perhaps it's a local issue on your side? Have you tried cleaning your dolphin build (and perhaps KIO as well) and re-run it to see if it still happens? > > I've done a kdesrc-build from empty sources directory and empty build directory, but not empty install

D13814: Speedup sort

jtamate added a comment. > I just cloned dolphin with your changes. > It compiled just fine. > Starting it with and without arguments also worked just fine. > > Perhaps it's a local issue on your side? Have you tried cleaning your dolphin build (and perhaps KIO as well) and re

D13814: Speedup sort

markg added a comment. In D13814#287959 , @jtamate wrote: > I really, really hate these things. > I've been testing this patch more than one month without any problem. > And now, after pushing it, I got crashes everytime I start dolphin wit

D13814: Speedup sort

jtamate added a comment. I really, really hate these things. I've been testing this patch more than one month without any problem. And now, after pushing it, I got crashes everytime I start dolphin without parameters (user home). But not if I start it in / and keep changing directories,

D13814: Speedup sort

This revision was automatically updated to reflect the committed changes. Closed by commit R318:63825de82f3a: Speedup sort (authored by jtamate). REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=37182&id=37273 REVISION DETAIL https://phabricator.kde.o

D13814: Speedup sort

elvisangelaccio accepted this revision. This revision is now accepted and ready to land. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarrom

D13814: Speedup sort

jtamate updated this revision to Diff 37182. jtamate added a comment. Remove the friend non-exist class. REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=37056&id=37182 REVISION DETAIL https://phabricator.kde.org/D13814 AFFECTED FILES src/kitem

D13814: Speedup sort

elvisangelaccio requested changes to this revision. elvisangelaccio added a comment. This revision now requires changes to proceed. Impressive, I went from 18 seconds to 4 :O But please remove friend class KFileItemModelLessThan; from `kfileitemmodel.h` REPOSITORY R318 Dolph

D13814: Speedup sort

markg accepted this revision. markg added a comment. This revision is now accepted and ready to land. 2x +1 = +2 Ship it :) REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks, markg Cc: apol, bruns, markg, kfm-devel, spoorun, n

D13814: Speedup sort

jtamate updated this revision to Diff 37056. jtamate added a comment. Undo the right number of steps from a quick experiment. :-) REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=37053&id=37056 REVISION DETAIL https://phabricator.kde.org/D13814 A

D13814: Speedup sort

bruns added a comment. In D13814#285736 , @jtamate wrote: > In D13814#285691 , @bruns wrote: > > > I assume you are sorting by name with "natural sorting". There may be another possibility for speedu

D13814: Speedup sort

bruns added a comment. In D13814#286034 , @jtamate wrote: > > In other words, the fix for that can probably be removed now. > > I prefer not to do it (even I've tried without any crash), because QVariant is not even reentrant. Reentra

D13814: Speedup sort

jtamate updated this revision to Diff 37053. jtamate marked an inline comment as done. jtamate added a comment. > In other words, the fix for that can probably be removed now. I prefer not to do it (even I've tried without any crash), because QVariant is not even reentrant. Changed t

D13814: Speedup sort

apol added a comment. +1 otherwise. INLINE COMMENTS > kfileitemmodel.cpp:1715 > { > -KFileItemModelLessThan lessThan(this, m_collator); > +auto lanbdaLessThan = [&] (const KFileItemModel::ItemData* a, const > KFileItemModel::ItemData* b) > +{ it's "lambda". REPOSITORY R318 D

D13814: Speedup sort

markg added a comment. In D13814#285686 , @bruns wrote: > In D13814#285660 , @markg wrote: > > > > I don't know if that's still an issue or if your patch re-introduces whatever the problem was (race

D13814: Speedup sort

jtamate updated this revision to Diff 37019. jtamate added a comment. Can not use the name lessThan because of: error: use of ‘lessThan’ before deduction of ‘auto’ REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=37010&id=37019 REVISION DETAIL h

D13814: Speedup sort

jtamate added a comment. In D13814#285691 , @bruns wrote: > I assume you are sorting by name with "natural sorting". There may be another possibility for speedup here: > `QCollator::sortkey()`, http://doc.qt.io/qt-5/qcollator.html#sortKey >

D13814: Speedup sort

bruns added a comment. I assume you are sorting by name with "natural sorting". There may be another possibility for speedup here: `QCollator::sortkey()`, http://doc.qt.io/qt-5/qcollator.html#sortKey It might be possible to generate the sortkey in e.g. https://phabricator.kde.org/sour

D13814: Speedup sort

jtamate updated this revision to Diff 37010. jtamate added a comment. Remove the _ prefix. REPOSITORY R318 Dolphin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13814?vs=36953&id=37010 REVISION DETAIL https://phabricator.kde.org/D13814 AFFECTED FILES src/kitemviews/kfileite

D13814: Speedup sort

jtamate added a comment. In D13814#285686 , @bruns wrote: > In D13814#285660 , @markg wrote: > > > > I don't know if that's still an issue or if your patch re-introduces whatever the problem was (rac

D13814: Speedup sort

bruns added a comment. In D13814#285660 , @markg wrote: > > I don't know if that's still an issue or if your patch re-introduces whatever the problem was (race conditions?). You could look back in the commit log when that was added to figure out

D13814: Speedup sort

markg added a comment. > I don't know if that's still an issue or if your patch re-introduces whatever the problem was (race conditions?). You could look back in the commit log when that was added to figure out more about it. To answer that myself, it was done in this commit: https://cg

D13814: Speedup sort

markg added a comment. +1 Great! :) Back to the good old fast performance it once had! Please do get rid of the underscore before the name. Nothing (afaict) does that in Dolphin, lets not introduce it. Just lessThan is fine. Also, note that this was done for a reason. I think it wa

D13814: Speedup sort

jtamate added a dependency: D13813: make this test work again with new uds implementation. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D13814 To: jtamate, #dolphin, #frameworks Cc: kfm-devel, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

D13814: Speedup sort

jtamate created this revision. jtamate added reviewers: Dolphin, Frameworks. Restricted Application added a project: Dolphin. Restricted Application added a subscriber: kfm-devel. jtamate requested review of this revision. REVISION SUMMARY Use a lambda function instead of a class. This way the