An experiment with KisSignalCompressor accuracy

Authored by poke1024 on Oct 28 2017, 2:20 PM.



One pet peeve I have with KisSignalCompressor, esp. with FIRST_ACTIVE, is that it produces quite some overhead as compared to using a repeating QTimer; yes, this is related to moving D8418 to KisSignalCompressor :-)

Currently, during drawing and thus in FIRST_ACTIVE mode for updating the canvas, Krita spends a couple of percents (usually 1% to 3%) with starting and stopping timers (multiply by two if you only look at the main thread, i.e. 2% to 6%). Here are two sample profiler occurences, but it's called in more places and it varies:

This patch is an experiment to eliminate this overhead. It relaxes the guarantees when a timeout() happens to up to 2 * m_delay. That makes it possible to use one steady timer (called monitor) that runs over a whole signal burst period (i.e. the period during which signals come in without pauses longer than m_delay). As such, it completely eliminates any timer starts and stops during the signal burst phase. The compressor state is quite complex now though (I don't like that).

Diff Detail

R37 Krita
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
poke1024 created this revision.Oct 28 2017, 2:20 PM

Hi, @poke1024!

Indeed, the state became really complex... I guess it is possible to simplify it a bit with two counters:

int m_nextTimerTickSeqNo;
int m_emitOnTimeTick;

As far as I can guess, we can control all the modes with just these two variables. Here is pseudocode:

    • first: m_emitOnTimeTick = m_nextTimerTickSeqNo + 1;
    • next: m_emitOnTimeTick = qMax(m_nextTimerTickSeqNo, m_emitOnTimeTick + 1)
    • first: emitSignals(); m_emitOnTimeTick = m_nextTimerTickSeqNo - 1; (basically forbid emission on the next tick)
    • others: m_emitOnTimeTick = m_nextTimerTickSeqNo;
    • first: emitSignals(); m_emitOnTimeTick = m_nextTimerTickSeqNo - 1; (basically forbid emission on the next tick)
    • others: m_emitOnTimeTick = qMax(m_nextTimerTickSeqNo, m_emitOnTimeTick + 1)
    • first: m_emitOnTimeTick = m_nextTimerTickSeqNo; (schedule emission on the next tick)
    • others: m_emitOnTimeTick = m_nextTimerTickSeqNo;
  • on every tick:
    • check if the timer should be stopped: if (m_nextTimerTickSeqNo - m_emitOnTimeTick > ticksStopThreshold) stopTimer();
    • emit the signals if m_nextTimerTickSeqNo == m_emitOnTimeTick
    • m_nextTimerTickSeqNo++

Such system would also remove the most of the old ad-hoc state variables, like m_gotSignals.

What do you think about this idea?

Actually, we use the logic like that in the "drop frames" functionality of KisAnimationPlayer. Though the guarantees of the player are a bit more strict, therefore it has a negative feedback loop.

Hi @dkazakov, this looks like a clean design, I'll change the patch accordingly.

dkazakov requested changes to this revision.Oct 30 2017, 1:16 PM

Hi, @poke1024!

Please ping me on IRC when you are done, so I would review your updated patch before next Monday, when I usually do that :)

This revision now requires changes to proceed.Oct 30 2017, 1:16 PM
poke1024 updated this revision to Diff 21615.Oct 31 2017, 10:16 AM

I tried to incorporate @dkazakov's suggestions into a simplified state KisSignalCompressor, but it didn't work out as I expected. We still need more state vars, as a compressor can get active and only later on decide if it will emit, but it needs to track its active state independently of its emission state.

I ended up with very complex code again, so I decided the best and cleanest approach is actually to take Dmitry's idea of sequential ticks and build a new KisRelaxedTimer that behaves like a QTimer to its users.

rempt accepted this revision.Nov 1 2017, 9:37 AM
rempt added a subscriber: rempt.

I am building this now, but I like the approach, it looks very clean to me.

dkazakov accepted this revision.Nov 1 2017, 9:46 PM

Hi, @poke1024!

The patch looks very clean and understandable. The idea about relaxed timer is just awesome!

I am a bit of afraid it may cause some subtle regressions: it touches core functionality that we use in quite a lot of places. But let's just push it and let people test :)
(I have tested it a little bit, and couldn't see any obvious regressions)

This revision is now accepted and ready to land.Nov 1 2017, 9:46 PM
This revision was automatically updated to reflect the committed changes.