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
> > 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
> > 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
> 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.
To: meven, #frameworks, dfaure, ngraham, #dolphin, jtamate
Cc: jtamate, markg, ngraham, #frameworks, michaelh