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

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:80d5f52b0675: [DeleteJob] Use a separate worker thread to 
run actual IO operation (authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=69332=69728

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> deletejob.cpp:130
> +/// Callback of worker rmfile
> +void rmFileResult(bool result, bool isLink);
> +/// Callback of worker rmdir

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.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 69332.
meven marked 4 inline comments as done.
meven added a comment.


  Use m_current instead of passing const QUrl  around

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68946=69332

BRANCH
  arcpatch-D24962

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> deletejob.cpp:406
> +QMetaObject::invokeMethod(worker(), "rmfile", 
> Qt::QueuedConnection,
> +  Q_ARG(const QUrl&, m_currentURL),
> +  Q_ARG(bool, isLink));

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in 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.

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 ?

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> meven wrote in deletejob.cpp:67
> I am hesitant to remove `const QUrl& url` from this signal, it could save 
> some space in message passing.

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.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added a task: T11627: Improve KIO asynchronicity.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added a reviewer: Frameworks.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in deletejob.cpp:67
> "const bool" doesn't do anything in a signal, I suggest removing the const.

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  OK, let's get this in :-)

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24962

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added a comment.


  In D24962#555443 , @dfaure wrote:
  
  > Can't wait to get clang-format fully automated into the review workflow
  
  
  Such a time saver it would be.

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68946.
meven marked 5 inline comments as done.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68872=68946

BRANCH
  arcpatch-D24962

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> deletejob.cpp:76
> + */
> +void rmfile(const QUrl& url, bool isLink){
> +emit rmfileResult(QFile::remove(url.toLocalFile()), url, isLink);

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

> deletejob.cpp:100
>  , m_reportTimer(nullptr)
> +, m_ioworker(nullptr)
>  {

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 :-)

> deletejob.cpp:197
> +
> +if (m_thread == nullptr) {
> +m_thread = new QThread();

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.

> deletejob.cpp:404
> +// If local file, try do it directly
> +if (m_currentURL.isLocalFile()){
> +// separate thread will do the work

space before `{` here as well

> deletejob.cpp:432
> +
> +void DeleteJobPrivate::deleteDirUsingJob (const QUrl )
> +{

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

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven added a comment.


  In D24962#554961 , @dfaure wrote:
  
  > 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.

INLINE COMMENTS

> dfaure wrote in deletejob.cpp:412
> marked as done but I still see removeFirst, I'm confused.

I missed this line, I did it line 430.

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68872.
meven added a comment.


  Refactor initWorkerThread() to *worker() accessor

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68854=68872

BRANCH
  arcpatch-D24962

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  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 :-)

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68854.
meven marked 7 inline comments as done.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68826=68854

BRANCH
  arcpatch-D24962

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Almost there ;)

INLINE COMMENTS

> deletejob.cpp:73
> +/**
> + * Deletes the file @p points to
> + * The file must be a LocalFile

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

> deletejob.cpp:132
> +void rmdirResult(bool result, const QUrl& url);
> +void deleteFileUsingJob (const QUrl& url, bool isLink);
> +void deleteDirUsingJob (const QUrl& url);

no space after method name (same on next line)

> deletejob.cpp:188
> +
> +m_ioworker = new DeleteJobIOWorker;
> +m_ioworker->moveToThread(_thread);

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.

> deletejob.cpp:340
> +
> +if (isLink) { // not a link
> +symlinks.removeFirst();

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

> deletejob.cpp:348
> +} else {
> +// fallback if unlink() failed (we'll use the job's error handling 
> in that case)
> +deleteFileUsingJob(url, isLink);

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

> deletejob.cpp:353
> +
> +void DeleteJobPrivate::deleteFileUsingJob (const QUrl , bool isLink)
>  {

please remove spaces after method names everywhere

> dfaure wrote in deletejob.cpp:412
> removeLast here too?

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

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68826.
meven marked 4 inline comments as done.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68825=68826

BRANCH
  master

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> deletejob.cpp:67
> +Q_SIGNALS:
> +void rmfileResult(bool succeeded, const QUrl& url, const bool isLink);
> +void rmddirResult(bool succeeded, const QUrl& url);

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

> deletejob.cpp:76
> + */
> +void rmfile(const QUrl& url, const bool isLink){
> +emit rmfileResult(QFile::remove(url.toLocalFile()), url, isLink);

(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)

> deletejob.cpp:412
> +m_processedDirs++;
> +dirs.removeFirst();
> +deleteNextDir();

removeLast here too?

> deletejob.cpp:445
> +// If local dir, try to rmdir it directly
> +if ((*it).isLocalFile()) {
> +// delete it on separate worker thread

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.

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68825.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68822=68825

BRANCH
  master

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68822.
meven added a comment.


  Fix argument passing to invokeMethod

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68821=68822

BRANCH
  master

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

meven updated this revision to Diff 68821.
meven added a comment.


  amend commit

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24962?vs=68819=68821

BRANCH
  master

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

AFFECTED FILES
  src/core/deletejob.cpp

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Méven Car]

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".
meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns