[DeleteJob] Use a separate worker thread to run actual IO operation
ClosedPublic

Authored by meven on Oct 26 2019, 11:50 AM.

Details

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven retitled this revision from WIP Ues a separate worker thread to run actual IO operation to WIP [DeleteJob] Use a separate worker thread to run actual IO operation.Oct 26 2019, 11:50 AM
meven updated this revision to Diff 68785.Oct 26 2019, 11:51 AM

Formatting

dfaure requested changes to this revision.Oct 27 2019, 8:28 AM

I like the approach.
Just a few minor things.

(and remember to remove the WIP from the commit log and the phab request)

The passing of iterators to another thread *looks* scary, but of course it's safe here since we only run one subjob at a time, so the list can't change while the worker does its work.

src/core/deletejob.cpp
66

can be removed

191

I would remove the "Private" from the name, it makes one think that there is a corresponding public class somewhere.

193

You're using the 4-args connect, which is static, so m_thread.connect is confusing/wrong. This should be QObject::connect instead.

194

Prefer QObject::connect(4 args), the receiver is missing and could be q_func() for extra safety (so that the connect is broken if the DeleteJob is deleted, even if m_ioworker is leaked for some reason).

366

(With the change I suggest below, this would just move to after m_processedFiles++; above. This would simplify the control flow in this method)

370

No space after the method name.

This method could take a QUrl instead of an iterator, all it does is *it to get to the QUrl.

Even better, it could keep using an iterator, but also take care of of the two erase(it) calls and addSubjob. This is currently duplicated between the two callers. The only difference is bool isLink, you could pass that as parameter.

This would even be more consistent with DeleteJobPrivate::deleteDirUsingJob which takes care of erase(it) and addSubjob.

394

You can make it const then.

403

I suggest to remove the erroneous comment

460

Move both return to after the if/else?

This revision now requires changes to proceed.Oct 27 2019, 8:28 AM
meven updated this revision to Diff 68816.Oct 27 2019, 9:08 AM
meven marked 9 inline comments as done.

Review feedback, cleanup, simplify parts, formatting, comments

meven updated this revision to Diff 68819.Oct 27 2019, 9:35 AM

Review feedback, cleanup, simplify parts, use removeFist instead of erase, formatting, comments

meven added a comment.Oct 27 2019, 9:36 AM

I like the approach.

Thanks for the suggestion in the last diff. I feared the callback hell, but this was not too difficult in fact.

Just a few minor things.

(and remember to remove the WIP from the commit log and the phab request)

Great review, thanks.

The passing of iterators to another thread *looks* scary, but of course it's safe here since we only run one subjob at a time, so the list can't change while the worker does its work.

I have update the code to instead pass the bool isLink and use removeFirst. It should be more efficient than begin == it + erase.

I feel the code looks simpler, although you need to understand the looping spreading over 4 functions.

meven retitled this revision from WIP [DeleteJob] Use a separate worker thread to run actual IO operation to [DeleteJob] Use a separate worker thread to run actual IO operation.Oct 27 2019, 9:41 AM
meven edited the summary of this revision. (Show Details)
meven updated this revision to Diff 68821.Oct 27 2019, 9:41 AM

amend commit

meven updated this revision to Diff 68822.Oct 27 2019, 9:58 AM

Fix argument passing to invokeMethod

meven updated this revision to Diff 68825.Oct 27 2019, 10:26 AM

Fix rmdir, we must use removeLast since we remove in reverse order

dfaure added inline comments.Oct 27 2019, 10:32 AM
src/core/deletejob.cpp
67

"const bool" doesn't do anything in a signal, I suggest removing the const.

76

(here it technically does something (guarantees that it's not modified in the implementation), but that's quite unusual in Qt/KDE code; your choice)

423

removeLast here too?

456

Use m_currentURL here, or use (*it) in the Q_ARG. It's just inconsistent right now, even though of course it's all technically the same.

(I think the code wasn't using m_currentURL because that's just something used for reporting, added after the fact, and which could technically be changed again one day -- although that's unlikely, I guess).
So I don't feel strongly about which one to use, but it should be consistent.

meven updated this revision to Diff 68826.Oct 27 2019, 10:41 AM
meven marked 4 inline comments as done.

Remove some const bool in function signature, use m_currentURL for consistency

dfaure requested changes to this revision.Oct 27 2019, 5:41 PM

Almost there ;)

src/core/deletejob.cpp
73

Missing "url" after @p (which introduces the name of an argument). Not that we're going to run doxygen here, but well ;)

132

no space after method name (same on next line)

191

One problem left: creating the worker and its thread is done even if we're deleting only remote URLs. This is a bit wasteful. Suggestion: create a worker() method which does all this (everything that you added to this method) on demand, if m_ioworker is null.

351

That is a really weird comment, for a bool called isLink :-)

359

(pre-existing) s/unlink/QFile::remove/ in this comment would be less confusing

364

please remove spaces after method names everywhere

423

marked as done but I still see removeFirst, I'm confused.

This revision now requires changes to proceed.Oct 27 2019, 5:41 PM
meven updated this revision to Diff 68854.Oct 27 2019, 9:05 PM
meven marked 7 inline comments as done.

Create worker thread only once needed, fix a removeLast missing, formatting

Any reason why you didn't implement my suggestion of

 DeleteJobIOWorker *ioworker() {
     if (!m_ioworker) {
           ...
     }
     return m_ioworker;
 }
[...] 
QMetaObject::invokeMethod(ioworker(), "rmfile", [...]);

?
A call to an initSomething() method can easily be forgotten, while an on-demand getter ensure that the worker is created when it's needed (for the first time).
Sorry for the nitpicking :-)

meven updated this revision to Diff 68872.Oct 28 2019, 9:18 AM

Refactor initWorkerThread() to *worker() accessor

meven added a comment.Oct 28 2019, 9:19 AM

Any reason why you didn't implement my suggestion of

 DeleteJobIOWorker *ioworker() {
     if (!m_ioworker) {
           ...
     }
     return m_ioworker;
 }
[...] 
QMetaObject::invokeMethod(ioworker(), "rmfile", [...]);

?
A call to an initSomething() method can easily be forgotten, while an on-demand getter ensure that the worker is created when it's needed (for the first time).
Sorry for the nitpicking :-)

Since m_ioworker is accessible where worker() would be needed, nothing keeps the user to use m_ioworker instead of worker() which is in the end is equivalent to forget to call initSomething.
It was my own habit to use a init or ensureInit function in such cases, and is the main reason I was using one.
But it is more explicit to have an accessor and for code coherence I have changed the code to have a *worker() function.

src/core/deletejob.cpp
423

I missed this line, I did it line 430.

meven marked an inline comment as done.Oct 28 2019, 9:19 AM
dfaure requested changes to this revision.Oct 28 2019, 8:31 PM

Can't wait to get clang-format fully automated into the review workflow....

src/core/deletejob.cpp
76

missing space before '{', sorry I missed it until now.

100

funny that you use the ctor init list for this one, and not a default value at declaration time, like you did for m_thread :-)

196

nitpick: it would be more expected to do if (!m_ioworker) { here, since m_ioworker is what we're creating on demand. m_thread is just an implementation detail.

403

space before { here as well

431

I still see a space between method name and ( here :-)

This revision now requires changes to proceed.Oct 28 2019, 8:31 PM
meven updated this revision to Diff 68946.Oct 29 2019, 6:56 AM
meven marked 5 inline comments as done.

Mostly formatting, change style to init m_ioworker and check it in worker()

meven added a comment.Oct 29 2019, 6:58 AM

Can't wait to get clang-format fully automated into the review workflow....

Such a time saver it would be.

dfaure accepted this revision.Oct 29 2019, 6:42 PM

OK, let's get this in :-)

This revision is now accepted and ready to land.Oct 29 2019, 6:42 PM
meven added inline comments.Oct 30 2019, 5:10 AM
src/core/deletejob.cpp
67

I am hesitant to remove const QUrl& url from this signal, it could save some space in message passing.

meven edited the summary of this revision. (Show Details)
dfaure added inline comments.Nov 2 2019, 11:30 AM
src/core/deletejob.cpp
67

Seems negligible to me. If you remove the argument, you'll have to "reconstruct" the URL from files.first()/symlinks.first()? Or [ab]use m_currentURL?
The "no premature optimization" rule says: don't change it if the resulting code is going to be more complicated.

meven added inline comments.Nov 2 2019, 12:36 PM
src/core/deletejob.cpp
67

With my code change m_currentUrl is always equal to this url just before calling rmfile/rmdir so yes it would be about [ab]using m_currentURL.
So it adds an implicit invariant around m_currentURL that should be obvious to future maintainers IMO.
Is this reducing in message passing worth this added invariant ?

dfaure added inline comments.Nov 3 2019, 12:35 PM
src/core/deletejob.cpp
406

Since you're already using m_currentURL here, I'd say this is acceptable.

meven edited the summary of this revision. (Show Details)Nov 6 2019, 8:50 AM
meven updated this revision to Diff 69332.Nov 6 2019, 9:00 AM
meven marked 4 inline comments as done.

Use m_current instead of passing const QUrl &url around

meven added inline comments.Nov 6 2019, 9:04 AM
src/core/deletejob.cpp
130

Now we are at it, isLink could become m_curentURLIsLink, but isLink being a bool, it is much cheaper to move around threads, so we don't have perf incentive to do it as we had with a QString.

This revision was automatically updated to reflect the committed changes.