KJob: add start(int delay) method
AbandonedPublic
Actions

Authored by rjvbb on Nov 25 2017, 5:03 PM.

Details

Reviewers

Summary

I'd like to propose a KJob method that provides a simple API to start asynchronous jobs after a delay. The default implementation would simply call KJob::start() via a single shot QTimer.

My justification (correct me if I'm wrong...):
Consider a cross-platform filemanager that provides a graphical directory representation which is to be kept in sync with the actual contents using a KDirWatch instance. Being cross-platform it has to contend with limitations on its use of KDirWatch and thus watches only directories. If it handles directory (re)loads with individual KJobs it will likely be starting multiple jobs for (re)loading the same folder(s) during disk-intensive operations in the parent directory (e.g. checking out a different git branch).
In order to limit the cost and potential side-effects of such concurrent reloads it can keep track of the current job list and cancel all jobs already loading a given folder when a new reload request comes in for that directory.

Starting the jobs with a delay should help reducing this cost even more: load requests that are superseded by a new request within the delay will not have cost a significant number of CPU cycles and will not have had the chance to cause side-effects.

Diff Detail

Lint

Lint Skipped

Unit

Unit Tests Skipped

rjvbb created this revision.Nov 25 2017, 5:03 PM

Restricted Application added a subscriber: Frameworks. · View Herald TranscriptNov 25 2017, 5:03 PM

rjvbb requested review of this revision.Nov 25 2017, 5:03 PM

anthonyfieroni added a subscriber: anthonyfieroni.Nov 25 2017, 6:43 PM

anthonyfieroni added inline comments.

src/lib/jobs/kjob.cpp
98 ↗	(On Diff #22925)	Use new syntax QTimer::singleShot(delay, this, &KJob::start);
src/lib/jobs/kjob.h
182 ↗	(On Diff #22925)	You cannot add a virtual function it will break ABI compatibility.

rjvbb marked 2 inline comments as done.Nov 25 2017, 7:54 PM

rjvbb added inline comments.

src/lib/jobs/kjob.cpp
98 ↗	(On Diff #22925)	So KJob::emitSpeed() doesn't use the "old" syntax for compatibility reasons?
src/lib/jobs/kjob.h
182 ↗	(On Diff #22925)	Duh, that s*kcs ... (I knew about making existing methods virtual). I'm not entirely certain if this has to be a virtual method but I guess there's no other choice. At least this removes any doubts as to whether I should provide a default implementation =)

Updated as requested.

rjvbb set the repository for this revision to R244 KCoreAddons.Nov 25 2017, 7:55 PM

rjvbb added inline comments.Nov 25 2017, 8:22 PM

src/lib/jobs/kjob.cpp
98 ↗	(On Diff #22925)	In fact, are you sure that KJob::start won't be ambiguous? Trying to compile this with clang++ I get src/lib/jobs/kjob.cpp:98:5: error: no matching function for call to 'singleShot' QTimer::singleShot(delay, this, &KJob::start); ^~~~~~~~~~~~~~~~~~ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:83:17: note: candidate function not viable: no overload of 'start' matching 'const char ' for 3rd argument static void singleShot(int msec, const QObject receiver, const char member); ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:187:17: note: candidate function not viable: no known conversion from 'int' to 'std::chrono::milliseconds' (aka 'duration<long long, milli>') for 1st argument static void singleShot(std::chrono::milliseconds value, const QObject receiver, const char member) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:102:24: note: candidate template ignored: couldn't infer template argument 'Func1' static inline void singleShot(Duration interval, const typename QtPrivate::FunctionPointer<Func1>::Object receiver, Func1 slot) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:130:13: note: candidate template ignored: couldn't infer template argument 'Func1' singleShot(Duration interval, Qt::TimerType timerType, Func1 slot) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:138:13: note: candidate template ignored: couldn't infer template argument 'Func1' singleShot(Duration interval, QObject context, Func1 slot) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:107:24: note: candidate function template not viable: requires 4 arguments, but 3 were provided static inline void singleShot(Duration interval, Qt::TimerType timerType, const typename QtPrivate::FunctionPointer<Func1>::Object receiver, ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:123:13: note: candidate function template not viable: requires 2 arguments, but 3 were provided singleShot(Duration interval, Func1 slot) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:145:13: note: candidate function template not viable: requires 4 arguments, but 3 were provided singleShot(Duration interval, Qt::TimerType timerType, QObject context, Func1 slot) ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:84:17: note: candidate function not viable: requires 4 arguments, but 3 were provided static void singleShot(int msec, Qt::TimerType timerType, const QObject receiver, const char member); ^ /opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:193:17: note: candidate function not viable: requires 4 arguments, but 3 were provided static void singleShot(std::chrono::milliseconds value, Qt::TimerType timerType, const QObject receiver, const char *member) ^ 1 error generated.

src/lib/jobs/kjob.cpp

98 ↗

(On Diff #22925)

In fact, are you sure that KJob::start won't be ambiguous? Trying to compile this with clang++ I get

src/lib/jobs/kjob.cpp:98:5: error: no matching function for call to 'singleShot'
    QTimer::singleShot(delay, this, &KJob::start);
    ^~~~~~~~~~~~~~~~~~
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:83:17: note: candidate function not viable: no overload of 'start' matching 'const char *' for 3rd argument
    static void singleShot(int msec, const QObject *receiver, const char *member);
                ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:187:17: note: candidate function not viable: no known conversion from 'int' to 'std::chrono::milliseconds' (aka 'duration<long long, milli>') for 1st argument
    static void singleShot(std::chrono::milliseconds value, const QObject *receiver, const char *member)
                ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:102:24: note: candidate template ignored: couldn't infer template argument 'Func1'
    static inline void singleShot(Duration interval, const typename QtPrivate::FunctionPointer<Func1>::Object *receiver, Func1 slot)
                       ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:130:13: note: candidate template ignored: couldn't infer template argument 'Func1'
            singleShot(Duration interval, Qt::TimerType timerType, Func1 slot)
            ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:138:13: note: candidate template ignored: couldn't infer template argument 'Func1'
            singleShot(Duration interval, QObject *context, Func1 slot)
            ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:107:24: note: candidate function template not viable: requires 4 arguments, but 3 were provided
    static inline void singleShot(Duration interval, Qt::TimerType timerType, const typename QtPrivate::FunctionPointer<Func1>::Object *receiver,
                       ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:123:13: note: candidate function template not viable: requires 2 arguments, but 3 were provided
            singleShot(Duration interval, Func1 slot)
            ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:145:13: note: candidate function template not viable: requires 4 arguments, but 3 were provided
            singleShot(Duration interval, Qt::TimerType timerType, QObject *context, Func1 slot)
            ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:84:17: note: candidate function not viable: requires 4 arguments, but 3 were provided
    static void singleShot(int msec, Qt::TimerType timerType, const QObject *receiver, const char *member);
                ^
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers/qtimer.h:193:17: note: candidate function not viable: requires 4 arguments, but 3 were provided
    static void singleShot(std::chrono::milliseconds value, Qt::TimerType timerType, const QObject *receiver, const char *member)
                ^
1 error generated.

int delay can be negative; prevent warnings from QTimer by calling start() directly when delay<=0.

rjvbb set the repository for this revision to R244 KCoreAddons.Nov 25 2017, 8:41 PM

I don't really see the value here. Can't the application developer also do the QTimer gimmick?

Something I find myself doing often is the singleShot with 0, maybe simply adding a startLater() (à la deleteLater()) method would work and be slightly more elegant API.

I'm not sure when is it that we'd use a delay != 0.

I don't think it's KJob's job (haha) to be responsible for compressing change notifications, which is basically the use case you're mentioning. This can be done with a QTimer and an associate container of pending changes, on the application side. Such a solution is much more flexible because you can add the data you need (for fast lookups, for more precise decision making etc.) in that container, rather than having to introspect not-started-yet jobs. Not to mention the issue with in-progress jobs.

The pipeline here is KDirWatch ---> notification compression ---> actual operations that *should* be triggered = KJob.

Misusing KJob as a holder for notification compression seems wrong to me, there are better data structures for that.

-1 from me.

This revision now requires changes to proceed.Nov 25 2017, 8:47 PM

anthonyfieroni added inline comments.Nov 25 2017, 8:49 PM

src/lib/jobs/kjob.cpp
98 ↗	(On Diff #22925)	Oh yes, it ambiguous :) It should use static_cast, if patch is accepted :)

I'm not convinced that there's much that can be done in the example I gave to make it smarter and more capable to predict random events, but that's not the point here. It's just an explicit example I had at hand. I proposed this addition because it's a tiny convenience that I think can be useful in other situations as well.

In fact it surprised me a bit that there wasn't already an API for spawning an async job after a delay.

-2

There *is* already API for this: QTimer::singleShot(delay, job, &KJob::start)

rjvbb abandoned this revision.Nov 26 2017, 12:01 AM

Revision Contents
Changeset List

			Path	Packages
M			src/lib/jobs/kjob.h (7 lines)
M			src/lib/jobs/kjob.cpp (5 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	22925		Nov 25 2017, 4:42 PM	★	★
Diff 2	22932	Updated as requested.	Nov 25 2017, 7:54 PM	★	★
Diff 3	22934	`int delay` can be negative; prevent warnings from QTimer by calling start()…	Nov 25 2017, 8:41 PM	★	★

KJob: add start(int delay) methodAbandonedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 22932

src/lib/jobs/kjob.h

src/lib/jobs/kjob.cpp

KJob: add start(int delay) method
AbandonedPublic
Actions

Revision Contents
Changeset List