Bug 23488 – Make TimerBase thread-aware. For use in Workers.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

To add setTimeout and friends to workers, we need to make TimerBase thread-aware. Timers use 'timer heap' to compute the nearest firing time and then fire timers that are ready. This heap is now global and will go into a new ThreadTimers object which will live in ThreadGlobalData. 

Also, global SharedTimer interface gets wrapped into an abstract class so the WorkerRunLoop can implement it too.

Patch is coming soon.

[Dmitry Titov]

Created attachment 26957 [details]
Proposed patch.

[Dmitry Titov]

Created attachment 26964 [details]
Proposed patch, v2

Thanks to David Levin who noticed a few coding style issues!
Updated patch.

[Dmitry Titov]

Created attachment 26985 [details]
Updated patch

Looking at it a bit more, fixed following:
- in multiple threads case, the timers can be created before worker thread initializes SharedTimer, and SharedTimer pointer may be set to 0 before all the timers are removed. Add check to avoid calling to SharedTimer when it's 0. (in setSharedTimer, updateSharedTimer)

- made ThreadTimers::m_timerHeap an instance of a Vector rather then a pointer to avoid it leaking when ThreadTimers gets destroyed (it does if its thread is not a main one)

[Darin Adler]

Comment on attachment 26985 [details]
Updated patch

> +    class SharedTimer {
> +    public:
> +        virtual ~SharedTimer() {}
> +        virtual void setFiredFunction(void (*)()) = 0;
> +        // The fire time is relative to the classic POSIX epoch of January 1, 1970,
> +        // as the result of currentTime() is.
> +        virtual void setFireTime(double) = 0;
> +        virtual void stop() = 0;
> +    };

The formatting was slightly better in the old version, where there was a blank line before the comment. It's a little harder to understand what the comment is grouped with here with everything mushed together.

> +Vector<TimerBase*>* ThreadTimers::timerHeap()
> +{
> +    return &m_timerHeap;
> +}

I think this should return a reference instead of a pointer. Callers currently have code to handle 0, and I'm not sure why.

> +HashSet<const TimerBase*>* ThreadTimers::timersReadyToFire()
> +{
> +    return m_timersReadyToFire;
> +}

This too.

> -        ASSERT(m_index < (timerHeap ? static_cast<int>(timerHeap->size()) : 0));
> +        ASSERT(m_index < (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0));

Can timerHeap() be zero? If not, lets remove the unneeded code.

> -        ASSERT_UNUSED(offset, m_index + offset <= (timerHeap ? static_cast<int>(timerHeap->size()) : 0));
> +        ASSERT_UNUSED(offset, m_index + offset <= (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0));

Same question here.

> -    return m_nextFireTime || (timersReadyToFire && timersReadyToFire->contains(this));
> +    return m_nextFireTime || (timersReadyToFire() && timersReadyToFire()->contains(this));

Can timersReadToFire() be zero? If not, lets remove the unneeded code.

> +class SharedTimer;

Forward declarations should all go at the top of the file, not in the middle.

> +// A collection of timers per thread. Kept in ThreadGlobalData.
> +class ThreadTimers : Noncopyable {

We normally want one class per header file. Can this class go in its own header? Also, can it be private? Do source files using the Timer class need to see this class's definition?

Can TimerBase still be the public interface to fireTimersInNestedEventLoop -- after all, it can just call through to the ThreadTimers function.

I think the patch would be so much better if you used references instead of pointers that I'm going to say review- and wait for a new patch. But generally, this patch seems great, and I was really close to saying review+.

[Darin Adler]

It's critical to keep the often-used header as small as possible. I'd like to see the new version of this patch keep Timer.h small and put new declarations either in a new .h file or possibly in the .cpp file.

[Dmitry Titov]

Created attachment 27008 [details]
Review comments addressed.

Thanks for review!
I think this patch addresses all the comments:

- added vertical space to SharedTimer.h
- moved fireTimersInNestedEventLoop back as static to TimerBase. It redirects to ThreadTimers internally. So other source files do not need to know about ThreadTimers.
- ThreadTimers::timerHeap() now returns reference, not a pointer. Removed checks for zero in callers.
- ditto for the ThreadTimers::timersReadyToFire, but with a twist: this one had a dual purpose. It was used to point to a stack-allocated set of timers and also serve as reentrancy guard (this is why it could be 0 before and some checks for 0 were necessary). However, this wasn't very good for readability so I've split this into 2 variables - bool m_firingTimers and m_timersReadyToFire that is never 0 (but can be empty).
- split ThreadTimers off into a separate .h and .cpp files. Couldn't make it private since it has to be used in ThreadGlobalData.cpp as well as in Timer.cpp.
- updated ChangeLog

[Darin Adler]

Comment on attachment 27008 [details]
Review comments addressed.

> +static MainThreadSharedTimer* mainThreadSharedTimer()
> +{
> +    static MainThreadSharedTimer* timer = 0;
> +    if (!timer)
> +        timer = new MainThreadSharedTimer();
> +    return timer;
> +}

You can get the same behavior without an if statement.

    static MainThreadSharedTimer* mainThreadSharedTimer()
    {
        static MainThreadSharedTimer* timer = new MainThreadSharedTimer;
        return timer;
    }

> +// Also, SharedTimer can be replaced with 0 before all timers are destructed.

Should say "destroyed" here, not "destructed".

Looks fine, r=me

[Dmitry Titov]

Created attachment 27170 [details]
Final version

Thanks!!!

Addressed latest comments (removed 'if' and fixed comment) and resolved against current tree.
Can be landed.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebCore/platform/ThreadGlobalData.h => WebCore/platform/ThreadTimers.h
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.scons
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/WebCoreSources.bkl
	M	WebCore/platform/SharedTimer.h
	M	WebCore/platform/ThreadGlobalData.cpp
	M	WebCore/platform/ThreadGlobalData.h
	A	WebCore/platform/ThreadTimers.cpp
	M	WebCore/platform/Timer.cpp
	M	WebCore/platform/Timer.h
Committed r40393


I'm not sure what the C means during the commit.  Hopefully not pain, death, or suffering.