QtCurve: reduce progressbar timer overhead
ClosedPublic

Authored by rjvbb on Sep 1 2017, 11:43 AM.

Details

Summary

Debugging an unrelated timer issue with GammaRay I noticed QtCurve can have multiple progressbar timers active in more complex applications like KDevelop even when progressbar aren't being animated (or are in fact not being shown/active). Progressbar timers run at 20Hz, which seems a bit fast for idling.

This patch introduces a rather basic improvement. The progressbar timer is started at an idling frequency of 2Hz unless it's being started for a progressbar which requires animation. It will then be set to the working frequency in the timerEvent callback, if and only if there are progressbars to be animated.

Test Plan

Works as expected. There's a small hickup when a progressbar is switched to animated (or "busy") mode, but one that ought to be hardly noticeable in real life.

The current version of the patch has hysteresis: timers are never switched back to idling frequency. I'm looking how much more complex the code becomes when they're turned back down as soon as there are no more animated progressbars.

NB: I'm not entirely certain why multiple timers would be active, it seems there ought only be a single one per process.

Diff Detail

Repository
R626 QtCurve
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb created this revision.Sep 1 2017, 11:43 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 1 2017, 11:43 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rjvbb updated this revision to Diff 19044.Sep 1 2017, 12:00 PM
rjvbb edited the summary of this revision. (Show Details)

cleaned up version that also switches the timer back to idling frequency

rjvbb set the repository for this revision to R626 QtCurve.Sep 1 2017, 12:00 PM

Not wanting to run a timer at 20fps when we're not animating a progress bar makes sense.

What I don't get is why, when we know we don't need to animate, we don't just stop the timer.

rjvbb added a comment.Sep 1 2017, 4:02 PM

Actually we do when the last progressbar is hidden, destroyed (or gets a QEvent::Close, testing that now).

The reason we need to run a timer when progressbars are visible is how their busy mode is implemented. That does not have a separate state and there's no signal that's sent when the state changes. As far as I've seen the only option is polling. I'll have a look at how Breeze and/or Oxygen do this.

rjvbb updated this revision to Diff 19073.Sep 1 2017, 9:21 PM

Following Breeze's example, we can defer the creation of a timer for the busy/indeterminate state to when we encounter one, i.e. when Style::drawControl() is called with CE_ProgressBarContents.

Determining if the timer can be killed is a bit trickier of course (without changing considerably more), so I could do this:

1- start the timer if still necessary

A- when an animated PB is shown for the 1st time (= as usual), but not when a non animated PB is shown.
B- when an indeterminate PB is *drawn* for the 1st time

2- return to the idle frequency when there are no more visible animated PBs (= current proposed change)
3- kill the timer when all PBs are closed, destroyed or hidden

Slight complication: Style::drawControl() is const, so one also has to move m_timer, m_progressBarAnimateTimer and m_progressBarAnimateFps to a private class, which in addition would have to have a q pointer and a startParentTimer() "proxy" so we can call Style::startTimer() from a const member function.

For now I've only implemented this for Qt5, let me know if this is too much (complicated, or of a hack).

rjvbb set the repository for this revision to R626 QtCurve.Sep 1 2017, 9:22 PM
yuyichao edited edge metadata.Sep 2 2017, 1:42 AM

Style::drawControl() is const, so one also has to move m_timer, m_progressBarAnimateTimer and m_progressBarAnimateFps to a private class, which in addition would have to have a q pointer and a startParentTimer() "proxy" so we can call Style::startTimer() from a const member function.

This should not be necessary. You can add mutable or do const cast for certain members.

The logic seems sound so LGTM given the tests done.

qt4/style/qtcurve.cpp
2697–2705

Indent a bit strange?

yuyichao accepted this revision.Sep 2 2017, 1:42 AM
This revision is now accepted and ready to land.Sep 2 2017, 1:42 AM
rjvbb added a comment.Sep 2 2017, 7:55 AM
This should not be necessary. You can add `mutable` or do const cast for certain members.

Right. Never used that myself it wasn't my 1st reflex. But I don't see how could do either for startTimer()?

But I don't see how could do either for startTimer()?

const_cast<Style*>(this)->startTimer()

rjvbb added a comment.Sep 2 2017, 12:06 PM

Never too old to learn, thanks :)

This revision was automatically updated to reflect the committed changes.
rjvbb added a comment.Sep 2 2017, 5:04 PM

Implementing on-demand/JIT starting of the timer also allowed to kill the timer when all progressbars stopped animating, in the end. No more need for the idling frequency trick.