markg added a comment.

  In D10702#212753 <>, @meven wrote:
  > In 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 
  > >  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:
  > >
  > > I wanted to paste it inline, but that already got quite big so a link it 
  > >  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.

  R241 KIO


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

Reply via email to