D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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 should be closed. It was already commited, unfortunately with a bug 
associated to it, fixed in the other review.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: aacid, elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, 
LeGast00n, sbergeron, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Albert Astals Cid]

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, elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, 
LeGast00n, sbergeron, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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

[Elvis Angelaccio]

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 0x56B5628: 
QSharedDataPointer::QSharedDataPointer(QSharedDataPointer
 const&) (qshareddata.h:92)
   by 0x56AF5D6: KFileItem::KFileItem(KFileItem const&) (kfileitem.h:47)
   by 0x575FDFB: KCoreDirListerCache::renameDir(QUrl const&, QUrl const&) 
(kcoredirlister.cpp:1576)
   by 0x575B041: KCoreDirListerCache::slotFileRenamed(QString const&, 
QString const&, QString const&) (kcoredirlister.cpp:969)
   by 0x577BF39: QtPrivate::FunctorCall, 
QtPrivate::List, void, void 
(KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&)>::call(void (KCoreDirListerCache::*)(QString const&, QString const&, 
QString const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577A630: void QtPrivate::FunctionPointer::call, 
void>(void (KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777250: QtPrivate::QSlotObject, 
void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) 
(qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x5798ED3: 
OrgKdeKDirNotifyInterface::FileRenamedWithLocalPath(QString const&, QString 
const&, QString const&) (moc_kdirnotify.cpp:224)
   by 0x579889D: OrgKdeKDirNotifyInterface::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (moc_kdirnotify.cpp:103)
   by 0x5798DAB: OrgKdeKDirNotifyInterface::qt_metacall(QMetaObject::Call, 
int, void**) (moc_kdirnotify.cpp:203)
   by 0x72B19EE: ??? (in /usr/lib/libQt5DBus.so.5.12.0)
 Address 0x11350758 is 72 bytes inside a block of size 128 free'd
   at 0x4839D7B: realloc (vg_replace_malloc.c:826)
   by 0x7439992: QListData::realloc_grow(int) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x7439A3B: QListData::append(int) (in /usr/lib/libQt5Core.so.5.12.0)
   by 0x576BE43: QList::insert(QList::iterator, 
KFileItem const&) (qlist.h:521)
   by 0x576B1EC: KCoreDirListerCache::DirItem::insert(KFileItem const&) 
(kcoredirlister_p.h:422)
   by 0x576AC8A: KCoreDirListerCache::reinsert(KFileItem const&, QUrl 
const&) (kcoredirlister_p.h:310)
   by 0x5760053: KCoreDirListerCache::renameDir(QUrl const&, QUrl const&) 
(kcoredirlister.cpp:1584)
   by 0x575B041: KCoreDirListerCache::slotFileRenamed(QString const&, 
QString const&, QString const&) (kcoredirlister.cpp:969)
   by 0x577BF39: QtPrivate::FunctorCall, 
QtPrivate::List, void, void 
(KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&)>::call(void (KCoreDirListerCache::*)(QString const&, QString const&, 
QString const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577A630: void QtPrivate::FunctionPointer::call, 
void>(void (KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777250: QtPrivate::QSlotObject, 
void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) 
(qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
 Block was alloc'd at
   at 0x4839D7B: realloc (vg_replace_malloc.c:826)
   by 0x7439992: QListData::realloc_grow(int) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x7439C1A: QListData::insert(int) (in /usr/lib/libQt5Core.so.5.12.0)
   by 0x576BE43: QList::insert(QList::iterator, 
KFileItem const&) (qlist.h:521)
   by 0x576B1EC: KCoreDirListerCache::DirItem::insert(KFileItem const&) 
(kcoredirlister_p.h:422)
   by 0x575D78C: KCoreDirListerCache::slotEntries(KIO::Job*, 
QList const&) (kcoredirlister.cpp:1252)
   by 0x577C268: QtPrivate::FunctorCall, 
QtPrivate::List const&>, void, void 
(KCoreDirListerCache::*)(KIO::Job*, QList const&)>::call(void 
(KCoreDirListerCache::*)(KIO::Job*, QList const&), 
KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577AAC8: void QtPrivate::FunctionPointer 
const&)>::call const&>, 
void>(void (KCoreDirListerCache::*)(KIO::Job*, QList const&), 
KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777D4A: QtPrivate::QSlotObject const&), 
QtPrivate::List const&>, void>::impl(int, 
QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x5703C23: KIO::ListJob::entries(KIO::Job*, QList 
const&) (moc_listjob.cpp:236)
   by 0x57027CE: KIO::ListJobPrivate::slotListEntries(QList 
const&) (listjob.cpp:154)

REPOSITORY
  R241 KIO

REVISION DETAIL
  

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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 there.
  
  Next step, transform this into a unit test that always crashes.

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

[Jaime Torres Amate]

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 
../../include/QtCore/../../src/corelib/global/qlogging.h:91
#11 0x7f44d4535c7a in KCoreDirListerCache::reinsert 
(this=this@entry=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, item=..., 
oldUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister_p.h:310
#12 0x7f44d452825c in KCoreDirListerCache::renameDir 
(this=this@entry=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, oldUrl=..., 
newUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:1584
#13 0x7f44d452aa29 in KCoreDirListerCache::slotFileRenamed 
(this=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, _src=..., 
_dst=..., dstPath=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:969
  
  Let's see if this assert/crash can be avoided without reverting all the 
patch. Any help is welcome.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:963
> Where did the old code call refresh() (which you now call inside findByUrl)? 
> I don't see it, I only see more specific calls in more specific cases. So 
> this looks slower and possibly incorrect (for non-local-files).

Changed to use the reinsert semantic.

> dfaure wrote in kcoredirlister.cpp:1004
> This used to modify the fileitem in dirItem->lstItems, now it's modifying a 
> copy.
> Same for all other fileitem.setFoo calls below.
> 
> Is there a call to reinsert missing?

Thanks, reinsert has a better semantics than refresh.

> markg wrote in kcoredirlister.cpp:825-829
> Hmm, this looks weird to me.
> Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.
> 
> A more efficient way (but requires you to change the lists this is backed by 
> to a std::vector) is to:
> 
> 1. Take the element out of the vector. Something like "KFileItem item = 
> std::move(*it);
> 2. Now the vector would be in a valid state but with one invalid object (will 
> post a problem if you iterate over it later on) so you have to remove that 
> element from the vector like you did.
> 
> I'm not sure if the benefits of this justify the path of changing the list 
> (dirItem->lstItems) to a std::vector, if possible at all.
> That's up to you :)

Changed, using the reinsert semantic.

> bruns wrote in kcoredirlister_p.h:302
> This can be better implemented with std::move and std::move_backward from 
> 
> http://www.cplusplus.com/reference/algorithm/move/
> 
> 1. calculate start and end iterators from the old and new URL
> 2. move old item to tmp
> 3.
> 
>   if (old_it < new_it) {
>  std::move(old_it + 1, new_it, old_it); // move downwards
>   } else {
>  std::move_backward(new_it , old_it - 1, old_it);
>   }
>   *new_it = tmp;

I've looked at those methods, they create a new "undefined" status in the list. 
I don't want to worry about this new status.

> bruns wrote in kfileitem.cpp:180
> You can probably leave this out if you use the following for ordering:
> 
>   bool operator< (const KFileItem& other) {
> if m_url.size() != other.m_url.size() {
>   return m_url.size() < other.m_url.size()
> }
> return m_url < other.m_url;
>   }
> 
> You don't need lexicographical sorting, but just a total ordering for the 
> lookup.

I've tried something similar. As QUrl doesn't have a size method, I've tried 
with path().size(). With findByUrl it is no problem, it gives me 28 msecs vs. 
1795 msecs, but inserting in the list gives me 37 msecs vs. the original 19 
msecs. 
Only checking m_url < other.m_url (without m_hash) gives me 21 msecs inserting 
and 15 msecs in findByUrl, faster than using m_hash with the right checks for 
collisions (22-23 msecs inserting).

> bruns wrote in kfileitem.h:490
> The description does not match the implementation ...

Changed, thanks. Now they match.

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

[Elvis Angelaccio]

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) {
>  const KFileItem oldItem = *kit;
> +KFileItem newItem = oldItem;
>  

Stacktrace from bug #401552  
points here. Reverting this commit seems to fix the crash.

@jtamate Can you have a look?

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

[Jaime Torres Amate]

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=43240

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[David Faure]

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 of the patch had obvious bugs that 
were not detected by the unittests).

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

[Jaime Torres Amate]

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 code as requested.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=35305=42345

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[David Faure]

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 modifies items and now has to reinsert them (and that code is hard 
to benchmark, it's mostly called from async slots...).
  
  Thanks.

INLINE COMMENTS

> kcoredirlister.cpp:2463
>  Q_ASSERT(!item.isNull());
> -(*lstNewItems)[directoryUrl].append(item);  // items not 
> filtered
> +(lstNewItems)[directoryUrl].append(item);  // items not 
> filtered
>  } else {

The parenthesis around lstNewItems can be removed now.

> kcoredirlister.cpp:2476
> +auto kit = items.cbegin();
> +const auto kend = items.end();
>  for (; kit != kend; ++kit) {

end() inconsistent with cbegin() on the line just above.

Works because the container is const, but still, better be consistent.

> kcoredirlister.cpp:2489
>  Q_ASSERT(!item.isNull());
> -(*lstNewItems)[directoryUrl].append(item);
> +(lstNewItems)[directoryUrl].append(item);
>  } else {

The parenthesis around lstNewItems can be removed now.

> kcoredirlister.cpp:2691
> +foreach (const KFileItem , *allItems) {
> +result.append(item);
> +}

Is this loop necessary? Can't we return KFileItemList(*allItems) ? There's such 
a ctor in KFileItemList.

> kcoredirlister_p.h:308
> +auto it = std::lower_bound(dirItem->lstItems.begin(), 
> dirItem->lstItems.end(), oldUrl);
> +dirItem->lstItems.erase(it);
> +dirItem->insert(item);

This is invalid code in case lower_bound() returns end().

If you're sure it "shouldn't happen" because reinsert is always called for 
items that are already in lstItems, then add a Q_ASSERT. Otherwise if().

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

[Jaime Torres Amate]

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 again.
  Better documented the new reinsert method.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=34775=35305

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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

[David Faure]

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 (!fileitem) {
> +// refresh the found item
> +KFileItem fileitem = findByUrl(nullptr, oldurl, true);

Where did the old code call refresh() (which you now call inside findByUrl)? I 
don't see it, I only see more specific calls in more specific cases. So this 
looks slower and possibly incorrect (for non-local-files).

> kcoredirlister.cpp:1004
>  if (nameOnly) {
> -fileitem->setName(dst.fileName());
> +fileitem.setName(dst.fileName());
>  } else {

This used to modify the fileitem in dirItem->lstItems, now it's modifying a 
copy.
Same for all other fileitem.setFoo calls below.

Is there a call to reinsert missing?

> kcoredirlister.cpp:1843
> -const KFileItem oldItem = *tmp;
> -*tmp = item;
>  foreach (KCoreDirLister *kdl, listers) {

This used to modify the item in dir->lstItems, so you need to reinsert in order 
to not lose the changes.

> kcoredirlister.cpp:2042
> +KFileItem oldItem = item;
> +item.refresh();
> +

Needs to be reinserted afterwards.

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

[Jaime Torres Amate]

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
  https://phabricator.kde.org/D10742?vs=34708=34775

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/widgets/kdirmodel.cpp

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Stefan Brüns]

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 , const QUrl )
> +{

This can be better implemented with std::move and std::move_backward from 

http://www.cplusplus.com/reference/algorithm/move/

1. calculate start and end iterators from the old and new URL
2. move old item to tmp
3.

  if (old_it < new_it) {
 std::move(old_it + 1, new_it, old_it); // move downwards
  } else {
 std::move_backward(new_it , old_it - 1, old_it);
  }
  *new_it = tmp;

> kfileitem.cpp:180
> +// The hash of the url for faster std::lower_bound
> +uint m_hash;
>  };

You can probably leave this out if you use the following for ordering:

  bool operator< (const KFileItem& other) {
if m_url.size() != other.m_url.size() {
  return m_url.size() < other.m_url.size()
}
return m_url < other.m_url;
  }

You don't need lexicographical sorting, but just a total ordering for the 
lookup.

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

[Stefan Brüns]

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 
> in String comparations).
> + * @since 5.47

The description does not match the implementation ...

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

[Jaime Torres Amate]

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  and 
D11282 , the best solution is to have the 
result of qHash(url) in KFileItem to compare items in the binary search.
  In two cases, the KFileItem in the list has to be moved to the right 
position, this is still faster than before.
  Introduce two methods to insert an item into the list and to move the item to 
the right position.
  
  The crash in kdirmodeltest was due to getting twice the signal of a file 
changed into a directory.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=29572=34708

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/widgets/kdirmodel.cpp

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, mwolff, markg, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Milian Wolff]

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, #frameworks, dfaure
Cc: mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=27909=29572

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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 first step towards making anything faster? This step 
in itself can only make things slower due to more "copying" (refcount updating).
  >
  >
  > I don't know what the ultimate goal of @jtamate is here (speeding things 
up, that i do know).
  
  
  Hi,
  
To speedup findByUrl, there are strategies like using a QHash for DirItem* 
or just having it sorted and search using std::binary_search.
I guess this will be easier to do if there are less pointers in the path, 
therefore  I'm just trying to remove a TODO: introduced in 
4b498196899223692e8a7d334618b2874bb6c77e in 2014.
I haven't done any benchmarks on this code yet, first it needs to pass all 
the tests.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Mark Gaiser]

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 slower due to more "copying" (refcount updating).
  
  
  I don't know what the ultimate goal of @jtamate is here (speeding things up, 
that i do know).
  But in general, putting something on the stack (aka, no new) is measurable 
faster. For small objects and in small quantities that doesn't matter much. But 
for large objects (KFileItem is large) and in large quantities (also fitting 
for KFileItem) it could very well be a nice speedup!
  
  The other side of this optimizing story is copies where you don't intent them 
to happen but they just do. For instance, a std::vector allocates a 
bunch ahead of time and then copies it in. Or moves since my move semantics 
commits and where applicable. If this were a std::vector you'd had 
no such copy or move.
  
  Back on this patch. It might allow faster routes, but it really depends on 
@jtamate next plans here if the current patch is worth it.
  Sounds like we need more info :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[David Faure]

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 DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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 refresh.
  
  I do not know why the refreshed mimetype of a file ending in .html becomes
  application/x-extension-html instead of text/html.
  
  I couldn't resist, I've removed also the NonMovableFileItem class.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=27768=27909

BRANCH
  dirlister (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  autotests/kdirlistertest.cpp
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Mark Gaiser]

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);
> +}
> +return retKFileItem;

Hmm, this looks weird to me.
Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.

A more efficient way (but requires you to change the lists this is backed by to 
a std::vector) is to:

1. Take the element out of the vector. Something like "KFileItem item = 
std::move(*it);
2. Now the vector would be in a valid state but with one invalid object (will 
post a problem if you iterate over it later on) so you have to remove that 
element from the vector like you did.

I'm not sure if the benefits of this justify the path of changing the list 
(dirItem->lstItems) to a std::vector, if possible at all.
That's up to you :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

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 part of a TODO: get rid of the raw KFileItem pointers in 
KCoreDirListerCache
  
  I've added a way to remove the item from the lister in findByUrl, to be 
modified and, if needed, added back.
  
  Questions to be answered:
  
  - In findByUrl, the dirItem->rootItem should also be removed?
  - fileItem is get twice in slotFileRenamed. Is this patch removing the right 
fileitem?
  - Should the fileitem removed in slotFileRenamed be added back or is it added 
back in emitRefreshItem? If should be  added back, to which list?
  
  I know the patched code doesn't follow the original semantics because dolphin 
see duplicated entries while renaming takes place.

TEST PLAN
  findByUrl is slow, for example, renaming 50.000 small files, it has to go 
through a list of 50.000 items 50.000 times, so renaming that number of files 
takes more than an hour.
  This patch does not improve findByUrl performance, but allows future work on 
it.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, #frameworks, dfaure
Cc: michaelh