D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate closed this revision. jtamate added a comment. In D10742#499346 , @aacid wrote: > @jtamate Does that "let's continue in that other review" mean we should cancel this one? Still shows on the list of reviews to consider Yes, it shou

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

aacid added a comment. @jtamate Does that "let's continue in that other review" mean we should cancel this one? Still shows on the list of reviews to consider REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: aacid, elvisangelacc

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate added a comment. Let's continue on D17619 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

elvisangelaccio added a comment. In D10742#377356 , @jtamate wrote: > Let's see if this assert/crash can be avoided without reverting all the patch. Any help is welcome. Does this valgrind log help? Invalid read of size 8 at

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate added a comment. I'm able to reproduce the bug with the following steps: - Create a folder structure X/X1/X2/X3/X4 and add a new empty text file into each folder. - Within Dolphin, open a tab for each folder. - In the tab showing X contents, rename X1 to X1_ and the crash is t

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate added a comment. I've been able to reproduce the bug: #10 0x7f44d257ae1d in qt_assert (assertion=assertion@entry=0x7f44d456fc43 "it != dirItem->lstItems.end()", file=file@entry=0x7f44d456fc10 "/g/5kde/frameworks/kio/src/core/kcoredirlister_p.h", line=line@entry=308) at ..

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

elvisangelaccio reopened this revision. elvisangelaccio added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kcoredirlister.cpp:1576-1577 > +for (auto kit = dir->lstItems.begin(), kend = > dir->lstItems.end(); kit != kend; ++kit) { >

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

This revision was automatically updated to reflect the committed changes. Closed by commit R241:20b89972b643: get rid of the raw KFileItem pointers in KCoreDirListerCache (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10742?vs=42345&id=43240

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. I think I'm fine with it now, but please wait until next Monday (Oct 8) before pushing, so it doesn't break the upcoming KF5 release. I'm not 100% confident (given that earlier versions

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 42345. jtamate marked 5 inline comments as done. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. I've tried to do an automatic benchmark of the rename case, without success. Changed cod

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Since the goal is to speed up findByUrl, can you add a Q_BENCHMARK for findByUrl? I do expect it to be faster of course --- a little bit at the expense of the code which modifie

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 35305. jtamate marked 4 inline comments as done. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. The change in kdirmodel is not needed anymore. The methods that were const are const aga

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate added a dependency: D13211: Enable comparing KFileItems by url. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kcoredirlister.cpp:963 > QUrl oldurl = src.adjusted(QUrl::StripTrailingSlash); > -KFileItem *fileitem = findByUrl(nullptr, oldurl); > -if (!fileit

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 34775. jtamate marked 5 inline comments as done. jtamate edited the summary of this revision. jtamate added a comment. Removed m_hash, after implementing the right checks it was slower than comparing QUrls. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

bruns added inline comments. INLINE COMMENTS > kcoredirlister_p.h:302 > +// Remove the item from the sorted list (from the wrong place) and > insert it in the right place. > +void reinsert(KFileItem &item, const QUrl &oldUrl) > +{ This can be better implemented with std::move and st

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

bruns added inline comments. INLINE COMMENTS > kfileitem.cpp:1248 > +{ > +return d->m_hash < other.d->m_hash; > +} This is incomplete for two cases: 1. Same URL 2. Hash collision > kfileitem.h:490 > +/** > + * Returns -1 if other's URL is greater, 0 if == and 1 if less than (as >

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 34708. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Restricted Application added a subscriber: kde-frameworks-devel. Based on the tests done in D12945

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate planned changes to this revision. jtamate added a comment. I've noticed this creates/exposes some crashes in other code/tests, for example in kdirmodeltest the second and later times the test is run. I'll check all the failing tests (given enough time). REPOSITORY R241 KIO REVISI

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

mwolff added a comment. no real review, just want to mention that you'll still have heap allocations for every item - now once per container it is in (due to QList and QSet and sizeof the KFileItem) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #fr

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 29572. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Pass all the unittests, without modifications, finally. Use as much as possible the recently added move semantics in KFileItem. R

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate added a comment. In D10742#218222 , @markg wrote: > In D10742#218171 , @dfaure wrote: > > > Can someone explain to me how switching from pointers to values is making anything faster, or is a

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

markg added a comment. In D10742#218171 , @dfaure wrote: > Can someone explain to me how switching from pointers to values is making anything faster, or is a first step towards making anything faster? This step in itself can only make things slo

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

dfaure added a comment. Can someone explain to me how switching from pointers to values is making anything faster, or is a first step towards making anything faster? This step in itself can only make things slower due to more "copying" (refcount updating). REPOSITORY R241 KIO REVISION DET

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate updated this revision to Diff 27909. jtamate edited the test plan for this revision. jtamate added a comment. The first patch had 12 unit test failing. This one has 2 1/2 unit test failing. I've changed findByUrl, instead of removing the item from the list, tell the item to ref

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

markg added a comment. I unfortunately have no clue how to answer your questions. INLINE COMMENTS > kcoredirlister.cpp:825-829 > +// If take remove the element from the list. > +if (take) { > +dirItem->lstItems.erase(it); > +

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

jtamate created this revision. jtamate added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY This summary will change when the doubts are resolved (if they can be resolved!). Implement the first