D10702: Always use a job to delete files to avoid freezing process waiting on IO

[Mark Gaiser]

markg added a comment.


  In D10702#212753 <https://phabricator.kde.org/D10702#212753>, @meven wrote:
  
  > In D10702#211236 <https://phabricator.kde.org/D10702#211236>, @markg wrote:
  >
  > > While this might give you the expected result, it feels like a workaround.
  > >
  > > I'm assuming the fast path is there for a reason and is really substantially faster then going through the job route.
  > >  If that is the case then the proper fix would be to make that code part async. That is obviously much more complex (otherwise it would've been done already).
  > >  Think of using std::async and a QEventLoop. Sounds difficult, right? It is :) But I've been playing with that kind of stuff lately so i'm happy to share an example that you can use as a starting point.
  > >  Here it is: https://p.sc2.nl/BygE-Oiwz
  > >
  > > I wanted to paste it inline, but that already got quite big so a link it is.
  > >  I've added a bunch of comments in the code to explains what it's doing.
  > >  Note that the example does make a "QEventLoop", you should **not** do that within the if statement, but rather outside the while loop and simply call exec() and quit() every time (not making a new QEventLoop for every delete)
  > >
  > > Lastly, please benchmark this fast pats (as it currently is) compared to your KIO version and my async version to see if the fast path really is the fast path. As we just don't know and that kinda influences which route to choose here.
  >
  >
  > Please correct if I am wrong but kio::file_delete will, in the end, calls FileProtocol::deleteRecursive(const QString &path) which works the same way using QFile::remove as before.
  >  So I have the hypothesis that they should not be much difference in performance if any.
  >
  > It could be impactful because of the KIO::file_delete call and I don't know precisely how expensive such a kio call is.
  >  Or because they are more jobs class instanciated.
  >  But those KIO::file_* calls are already used in a lot of cases in CopyJobPrivate::copyNextFile for instance to recursively copy files, so I know that at least in some cases it is ok.
  >
  > Please correct me wherever I am wrong I am new in the KIO codebase. I am learning as I go.
  >
  > The main drawback in this version of the patch currently, is that we loose progressive status update when a lot of files are being removed (slotReport was called each 300 deletion before).
  >  And since needs to be fixed this at least.
  
  
  This is spot on: ..."//in the end, calls FileProtocol::deleteRecursive(const QString &path) //"...
  But it takes some time to get there.
  KIO is process based. It opens a file slave and feeds it instructions via a socket connection. That architecture is why it doesn't block.
  This is heavily oversimplified, but that's roughly how it works.
  
  This communication (and subsequent parsing of the messages) just takes "time". Not a whole lot and it's quite efficient, but it starts to count with loads of files.
  I don't know how much this time is, you'd have to benchmark it to know. I'm guessing it's quite substantial otherwise the fast path wouldn't be there.
  
  As for losing progression status... I think that's a no-go. Users kinda lake to know how far something is.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, ngraham, #dolphin, jtamate
Cc: jtamate, markg, ngraham, #frameworks, michaelh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180224/9dd03793/attachment-0001.html>