Bug 23488 - Make TimerBase thread-aware. For use in Workers.
Summary: Make TimerBase thread-aware. For use in Workers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 22718 23560
  Show dependency treegraph
 
Reported: 2009-01-22 17:51 PST by Dmitry Titov
Modified: 2009-01-29 18:02 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch. (24.16 KB, patch)
2009-01-22 21:51 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
Proposed patch, v2 (24.18 KB, patch)
2009-01-23 01:55 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
Updated patch (24.50 KB, patch)
2009-01-23 14:48 PST, Dmitry Titov
darin: review-
Details | Formatted Diff | Diff
Review comments addressed. (31.49 KB, patch)
2009-01-24 23:26 PST, Dmitry Titov
darin: review+
Details | Formatted Diff | Diff
Final version (31.60 KB, patch)
2009-01-29 16:20 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2009-01-22 17:51:27 PST
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.
Comment 1 Dmitry Titov 2009-01-22 21:51:31 PST
Created attachment 26957 [details]
Proposed patch.
Comment 2 Dmitry Titov 2009-01-23 01:55:54 PST
Created attachment 26964 [details]
Proposed patch, v2

Thanks to David Levin who noticed a few coding style issues!
Updated patch.
Comment 3 Dmitry Titov 2009-01-23 14:48:52 PST
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)
Comment 4 Darin Adler 2009-01-24 13:35:05 PST
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+.
Comment 5 Darin Adler 2009-01-24 13:36:14 PST
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.
Comment 6 Dmitry Titov 2009-01-24 23:26:27 PST
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
Comment 7 Darin Adler 2009-01-29 15:06:39 PST
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
Comment 8 Dmitry Titov 2009-01-29 16:20:41 PST
Created attachment 27170 [details]
Final version

Thanks!!!

Addressed latest comments (removed 'if' and fixed comment) and resolved against current tree.
Can be landed.
Comment 9 Eric Seidel (no email) 2009-01-29 18:02:16 PST
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.