[Development] [fix]: Assistant (and others) burning CPU because of #f27d1ccbb24ec2fd4098f2976503478831006cc8 ?

[Thiago Macieira]

On Wednesday, 30 August 2017 14:17:05 PDT René J. V. Bertin wrote:
> Thiago Macieira wrote:
> 
> Oops, missed this one.
> 
> > Unfortunately, we can't change anymore.
> 
> Can someone explain what could possibly be broken by changing this? Can the
> current base-level QObject::timerEvent() be used for anything constructive
> other than heating the office or preventing a CPU core from going to sleep?
> I only made my suggestion because it seemed rather evident that anyone
> using the QObject timer would override the timerEvent() method.

It makes QTimer possible.

void QTimer::start()
{
    if (id != INV_TIMER)                        // stop running timer
        stop();
    nulltimer = (!inter && single);
    id = QObject::startTimer(inter, Qt::TimerType(type));
}

That sounds quite important to me :-)

QBasicTimer also used it until f821d6352e384a737c82a0db903104ec4f8611bd, when 
Brad wrote "QBasicTimer becomes even more low-level. It cannot use 
QObject::startTimer() anymore, since we do not have a way to call 
QObject::killTimer() from QBasicTimer::stop()."

On a serious note, there's a lot of code that uses startTimer() to get timers 
more efficiently than QTimer or even QBasicTimer. We can't remove this function, 
because it would break the existing code. And we can't make it a single shot 
because it would be even worse: it would silently would equally break existing 
code that expects the timer to repeat itself. This code is probably buried 
deep into codebases several years old and not that trivial to find.

And I don't want to add a flag to say it's a single shot to the event 
dispatcher, because it would add overhead for all existing timers. Not to 
mention it's not doable until Qt 6 when we can add or modify virtual methods 
to QAbstractEventDispatcher.

> That said, it does seem a bit overkill to give each and every instance
> inheriting QObject timer features. Most will presumably never use that in a
> typical application, would they?

Most won't. How do you choose which ones should get the feature? If you're 
arguing for a white list, please compile such al ist, including all uses in 
KDE code, all applications, etc.

And how would we even achieve this? The only way I can think of is to make 
those functions private (a Qt 6 change) and add the whitelisted classes as 
friends.

> Found the bug/fix? The usual way, reverting all changes and restoring them
> in order of least likely culprit. Then by generating trace output,
> evidently I thought last of putting it *before* the timer ID check, and
> then I had to consult the docs to confirm that one indeed needs to kill the
> timer when done. I'd never have guessed that a noop event handler callback
> could have pump this many resources.

Ok, so you found by git bisect (or similar) action. I was hoping you had some 
interesting way of debugging that the problem is timers constantly firing. I 
woder if GammaRay can give you a list of timers active in a thread and which 
objects it's connected to. Volker?

> > Actually, there is an overhead, albeit very small one: the timer ID is
> > stored.
> Indeed, I wondered about that, and then let it pass because the compiler
> will probably optimise it away.

it won't, because the QBasicTimer object needs to be stored somewhere. If 
you're replacing an int m_timerId with QBasicTimer, there's no overhead. But 
if you don't store the timer ID, there's an overhead.

You can't not store the QBasicTimer (pardon the double negative). On its 
destruction, it will stop the timer. So if you used it in a regular function 
scope, the tiemr would never fire.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center