Differential D8081

Fix KisSignalCompressor delays
ClosedPublic
Actions

Authored by poke1024 on Oct 1 2017, 6:10 AM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

If there are many (input) events, the timer in KisSignalCompressor might actually take a long time (>500 ms) to fire. This means that, among others, update events can take a long time to happen, giving the program a laggy appearance.

Using heavy scrubbing on the zoom slider in the overview docker panel, you can reproduce such a situation: the zooming canvas gets jerky (see test plan below for details).

The implications of this change, though short, might not be trivial:

  • if KisSignalCompressor gets called from a non-main thread, slotTimerExpired might now also be called from that thread, whereas before it was guaranteed to be called from QTimer's callback, which is probably always the main thread
  • In very bad circumstances, we might now trigger endless recursions, which was impossible before. We could add additional protection against that in the new code in KisSignalCompressor by basically checking if we are in a call we ourselves started.
Test Plan

You can see the delay of the original code in numbers by installing an instrumentation as follows:

In KisCanvas2 add an QElapsedTimer timer to KisCanvas2Private, then add:

void KisCanvas2::slotDoCanvasUpdate()
{
	printf("slotDoCanvasUpdate %ld\n", (long)m_d->timer.elapsed());
	m_d->timer.start();

and

void KisCanvas2::updateCanvasWidgetImpl(const QRect &rc)
{
	printf("triggering update %ld\n", (long)m_d->timer.elapsed());

Under heavy zoom scrubbing, you will see something like the following in the console:

triggering update 17
slotDoCanvasUpdate 17
triggering update 299
slotDoCanvasUpdate 299
triggering update 0
triggering update 0
slotDoCanvasUpdate 11
triggering update 69
slotDoCanvasUpdate 69
triggering update 8
triggering update 19
triggering update 23
triggering update 59
triggering update 62
triggering update 80
triggering update 89
triggering update 97
triggering update 117
triggering update 136
triggering update 155
triggering update 162
triggering update 169
triggering update 176
triggering update 205
triggering update 216
triggering update 230
triggering update 244
triggering update 267
triggering update 274
triggering update 278
triggering update 299
triggering update 304
triggering update 309
triggering update 318
triggering update 326
triggering update 330
triggering update 352
slotDoCanvasUpdate 386
triggering update 550
slotDoCanvasUpdate 550
triggering update 11
slotDoCanvasUpdate 11
triggering update 5
slotDoCanvasUpdate 5
triggering update 11
slotDoCanvasUpdate 11
triggering update 5
slotDoCanvasUpdate 5

I.e. updates are triggered and reported to KisSignalCompressor, but KisSignalCompressor's timer does not get called and thus the actual update (slotDoCanvasUpdate) does not run for 386 ms.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
poke1024 created this revision.Oct 1 2017, 6:10 AM
alvinhochun added a subscriber: alvinhochun.Oct 1 2017, 7:14 AM

On Windows, if a timer has an interval below 20ms Qt will automatically use a multimedia timer, in which the callback is actually fired on another thread and Qt needs to trigger the event back on the original thread. Curiously, without any modifications I don't see any lag when scrubbing the slider on Windows, perhaps because of the timer implementation difference?

if KisSignalCompressor gets called from a non-main thread, slotTimerExpired might now also be called from that thread, whereas before it was guaranteed to be called from QTimer's callback, which is probably always the main thread

Might be solved by using a signal and slot connection, since Qt will automatically ensure that the firing thread is correct.

In very bad circumstances, we might now trigger endless recursions, which was impossible before. We could add additional protection against that in the new code in KisSignalCompressor by basically checking if we are in a call we ourselves started.

Maybe just use Qt::QueuedConnection...

Well, I haven't actually done any profiling or testing here so I could be wrong.

alvinhochun added a comment.Oct 1 2017, 7:15 AM

(Also, can I just ask you once again to join the irc?)

poke1024 added a comment.Oct 1 2017, 11:37 AM

@alvinhochun This is indeed interesting, so the problem won't occur on Windows. On OS X the timer seems to get swamped if there's a lot of input events, I originally detected this by playing with touch events and seeing that if I used my MacBook touchpad Krita did not respond at all (due to this effect: a lot of touch events, no timers).

Yes, I'm just not an always online guy, but I'll see to joining IRC.

dkazakov accepted this revision.Oct 3 2017, 2:10 PM

if KisSignalCompressor gets called from a non-main thread, slotTimerExpired might now also be called from that thread

That is not an issue to worry about: one is not allowed to call signal compressor from non-gui thread anyway. QTimer can not be started from non-gui thread :) There is a special KisThreadSafeSignalCompressor that overcomes this issue by proxying all the calls via Qt's signals.

There is one minor codestyle problem, otherwise the patch is perfectly fine to be pushed.

I have tested it on Linux and it makes the numbers a bit better (though on my computer the delay is really rarely higher than 80ms). After the patch the delay is never higher than 20ms

libs/global/kis_signal_compressor.cpp
68

Please fix indentation to 'spaces' and merge two 'ifs' into one. It took me a bit of time searching for an else branch that could cause such a split :)

This revision is now accepted and ready to land.Oct 3 2017, 2:10 PM
rempt added a subscriber: rempt.Oct 3 2017, 2:13 PM

Actually, the Qt documentation says:

In multithreaded applications, you can use QTimer in any thread that has an event loop. To start an event loop from a non-GUI thread, use QThread::exec(). Qt uses the timer's thread affinity to determine which thread will emit the timeout() signal. Because of this, you must start and stop the timer in its thread; it is not possible to start a timer from another thread.

dkazakov added a comment.Oct 3 2017, 2:15 PM
In D8081#151948, @rempt wrote:

Actually, the Qt documentation says:

In multithreaded applications, you can use QTimer in any thread that has an event loop. To start an event loop from a non-GUI thread, use QThread::exec(). Qt uses the timer's thread affinity to determine which thread will emit the timeout() signal. Because of this, you must start and stop the timer in its thread; it is not possible to start a timer from another thread.

Yes, the timer works in the thread to which it belongs and which runs the event loop. But in Krita we have only one event loop and it runs in the main gui thread :)

rempt closed this revision.Oct 4 2017, 7:19 AM

Revision Contents
Changeset List

Diff 20176

View Options

libs/global/kis_signal_compressor.cpp

Loading...