BUG: 390748
FIXED-IN: 5.65
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Maniphest Tasks
- T11627: Improve KIO asynchronicity
- Commits
- R241:80d5f52b0675: [DeleteJob] Use a separate worker thread to run actual IO operation
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 18222 Build 18240: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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 | |
| 188 | I would remove the "Private" from the name, it makes one think that there is a corresponding public class somewhere. | |
| 190 | You're using the 4-args connect, which is static, so m_thread.connect is confusing/wrong. This should be QObject::connect instead. | |
| 191 | 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). | |
| 355 | (With the change I suggest below, this would just move to after m_processedFiles++; above. This would simplify the control flow in this method) | |
| 359 | 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. | |
| 383 | You can make it const then. | |
| 392 | I suggest to remove the erroneous comment | |
| 449 | Move both return to after the if/else? | |
Comment Actions
Review feedback, cleanup, simplify parts, use removeFist instead of erase, formatting, comments
Comment Actions
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.
| 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) | |
| 412 | removeLast here too? | |
| 445 | 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). | |
Comment Actions
Almost there ;)
| src/core/deletejob.cpp | ||
|---|---|---|
| 412 | marked as done but I still see removeFirst, I'm confused. | |
| 73 ↗ | (On Diff #68822) | Missing "url" after @p (which introduces the name of an argument). Not that we're going to run doxygen here, but well ;) |
| 132 ↗ | (On Diff #68822) | no space after method name (same on next line) |
| 188 ↗ | (On Diff #68822) | 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. |
| 340 ↗ | (On Diff #68822) | That is a really weird comment, for a bool called isLink :-) |
| 348 ↗ | (On Diff #68822) | (pre-existing) s/unlink/QFile::remove/ in this comment would be less confusing |
| 353 ↗ | (On Diff #68822) | please remove spaces after method names everywhere |
Comment Actions
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 :-)
Comment Actions
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 | ||
|---|---|---|
| 412 | I missed this line, I did it line 430. | |
Comment Actions
Can't wait to get clang-format fully automated into the review workflow....
| src/core/deletejob.cpp | ||
|---|---|---|
| 76 ↗ | (On Diff #68822) | missing space before '{', sorry I missed it until now. |
| 100 ↗ | (On Diff #68822) | 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 :-) |
| 207 ↗ | (On Diff #68822) | 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. |
| 392 ↗ | (On Diff #68822) | space before { here as well |
| 420 ↗ | (On Diff #68822) | I still see a space between method name and ( here :-) |
| src/core/deletejob.cpp | ||
|---|---|---|
| 67 | I am hesitant to remove const QUrl& url from this signal, it could save some space in message passing. | |
| 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? | |
| 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. | |
| src/core/deletejob.cpp | ||
|---|---|---|
| 395 ↗ | (On Diff #68822) | Since you're already using m_currentURL here, I'd say this is acceptable. |
| src/core/deletejob.cpp | ||
|---|---|---|
| 130 ↗ | (On Diff #68822) | 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. |