Bug 28455 – ThreadTimer: avoid blocking UI when too many timers ready to fire

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Yong Li]

Current ThreadTimer fires all timers that meet the fire time. This can block UI, if some of those timers are big time-consumers.

Solution is to set a timeout when firing those timers. When time is out, those unfired timers should be postponed to next time.

[Yong Li]

Created attachment 35123 [details]
the patch

[Eric Seidel (no email)]

Darin Adler is probably your best bet for review here.

[Darin Adler]

Comment on attachment 35123 [details]
the patch

The concept of the code is here OK, but the conditionals need to be done better.

I think you should have the TimerBeingFired struct all the time, and just make the fire time member be conditional.

Normally we do not use the m_ prefix for struct members, only for classes where they are private.

> +    for (Vector<TimerBeingFired>::const_iterator i = firingTimers.begin(); i != firingTimers.end(); ++i) {
> +        TimerBase* timer = i->m_timer;

>      for (size_t i = 0; i != size; ++i) {
>          TimerBase* timer = firingTimers[i];

We do not normally use iterators to go through a Vector. Instead we just use indexing as you would with an array. That's what we did in the existing code, and there's no reason to change that just because the timers are a struct.

> +        // 30ms
> +        if (currentTime() - startTime > 0.03) {

The time interval should be a constant at the top of the file, and the comment should explain why that particular interval was chosen.

> +            for (++i; i != firingTimers.end(); ++i) {
> +                timer = i->m_timer;
> +                if (timer->m_nextFireTime == 0 && m_timersReadyToFire.contains(timer))
> +                    timer->setNextFireTime(i->m_fireTime);
> +            }

I wrote the class, and yet I do not understand this code. It may be correct. There needs to be a comment explaining why this is the right thing to do.

[Dmitry Titov]

I refactored the code Darin originally wrote to the point when I almost understand it :-) so let me add a comment: I think this will remove and then re-insert unfired timers into the heap structure, causing unneeded sort operations - which might be unwanted in a scenario when we have so many timers it freezes UI...

Bug 23865 has a patch addressing the same very issue, but instead of taking all the ready timers first into the separate list and then re-inserting the unfired timers back, it just fires them in a loop one-by-one, taking them from original heap until time interval permits. This avoids taking and then re-inserting timers into timer heap, while still keeping track of removed and rescheduled timers.

That patch was not landed because the original bug was about even bigger issue (recursive multiplication of timers) - but for avoiding UI freeze IMHO the patch would work better. Also, I think it is useful for any port and doesn't affect functionality so it doesn't need ENABLE define guards.

If you find this makes sense, please feel free to use the code in Bug 23865 as an idea or ping me to continue to work on it...

[Yong Li]

> > +    for (Vector<TimerBeingFired>::const_iterator i = firingTimers.begin(); i != firingTimers.end(); ++i) {
> > +        TimerBase* timer = i->m_timer;
> 
> >      for (size_t i = 0; i != size; ++i) {
> >          TimerBase* timer = firingTimers[i];
> 
> We do not normally use iterators to go through a Vector. Instead we just use
> indexing as you would with an array. That's what we did in the existing code,
> and there's no reason to change that just because the timers are a struct.

I agree with the rest of your suggestions. but for this one I think using iterator (or const_iterator) should be better.

First, "iterator" is kind of standard way for enumerating a C++ container. 

typedef Vector<some type> MyContainerType; 

for (MyContainerType::const_iterator i = container.begin(); i != container.end(); ++i) {
}

If someday you change the container type from Vector to List, you don't have to change the code that enumerates the container.

Second, I think the fastest way of walking through an array is using an increasing pointer instead of an index, because array[i] may require i * sizeof(T) to get the address of the element. Although some compilers can be smart enough to optimize this, I saw replacing array index with increasing pointer boosted performance in practice. Vector<T>::iterator is just a pointer T*, and so that it doesn't rely on how smart the compiler is to make the code run faster.

[Yong Li]

(In reply to comment #4)
> I refactored the code Darin originally wrote to the point when I almost
> understand it :-) so let me add a comment: I think this will remove and then
> re-insert unfired timers into the heap structure, causing unneeded sort
> operations - which might be unwanted in a scenario when we have so many timers
> it freezes UI...
> 
> Bug 23865 has a patch addressing the same very issue, but instead of taking all
> the ready timers first into the separate list and then re-inserting the unfired
> timers back, it just fires them in a loop one-by-one, taking them from original
> heap until time interval permits. This avoids taking and then re-inserting
> timers into timer heap, while still keeping track of removed and rescheduled
> timers.
> 
> That patch was not landed because the original bug was about even bigger issue
> (recursive multiplication of timers) - but for avoiding UI freeze IMHO the
> patch would work better. Also, I think it is useful for any port and doesn't
> affect functionality so it doesn't need ENABLE define guards.
> 
> If you find this makes sense, please feel free to use the code in Bug 23865 as
> an idea or ping me to continue to work on it...

If that patch doesn't 
(In reply to comment #4)
> I refactored the code Darin originally wrote to the point when I almost
> understand it :-) so let me add a comment: I think this will remove and then
> re-insert unfired timers into the heap structure, causing unneeded sort
> operations - which might be unwanted in a scenario when we have so many timers
> it freezes UI...
> 
> Bug 23865 has a patch addressing the same very issue, but instead of taking all
> the ready timers first into the separate list and then re-inserting the unfired
> timers back, it just fires them in a loop one-by-one, taking them from original
> heap until time interval permits. This avoids taking and then re-inserting
> timers into timer heap, while still keeping track of removed and rescheduled
> timers.
> 
> That patch was not landed because the original bug was about even bigger issue
> (recursive multiplication of timers) - but for avoiding UI freeze IMHO the
> patch would work better. Also, I think it is useful for any port and doesn't
> affect functionality so it doesn't need ENABLE define guards.
> 
> If you find this makes sense, please feel free to use the code in Bug 23865 as
> an idea or ping me to continue to work on it...

Hm... It seems better.

[Darin Adler]

> I agree with the rest of your suggestions. but for this one I think using
> iterator (or const_iterator) should be better.
> 
> First, "iterator" is kind of standard way for enumerating a C++ container. 
> 
> typedef Vector<some type> MyContainerType; 
> 
> for (MyContainerType::const_iterator i = container.begin(); i !=
> container.end(); ++i) {
> }
> 
> If someday you change the container type from Vector to List, you don't have to
> change the code that enumerates the container.

I don't agree. I am aware that using iterators lets you use the same code on Vector and List. I find the code using indices much easier to read and have never had an experience where I had to change code from Vector to List.

I do find it useful for generic code that works on, say, both Vector and List.

[Dmitry Titov]

Re-resolving due to Bugzilla data loss. The bug 23865 now has the patch fixing this issue.

*** This bug has been marked as a duplicate of bug 23865 ***