Bug 82176 – [Refactoring] Introduce TimerScheduler to encapsulate heap structure.

• Component: Platform
• Status: RESOLVED WONTFIX

[Hajime Morrita]

This is series of refactoring for Bug 82166.

[Hajime Morrita]

Created attachment 133760 [details]
Patch

[Hajime Morrita]

Eric, Dimitri, could someone take a look?
This is a part of long preparation for investigating my crash bug...

[WebKit Review Bot]

Comment on attachment 133760 [details]
Patch

Attachment 133760 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12128798

New failing tests:
inspector/console/console-long-eval-crash.html
fast/animation/request-animation-frame-during-modal.html
fast/events/scroll-event-during-modal-dialog.html

[WebKit Review Bot]

Created attachment 133773 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 133760 [details]
Patch

Attachment 133760 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12128816

New failing tests:
inspector/console/console-long-eval-crash.html
fast/animation/request-animation-frame-during-modal.html
fast/events/scroll-event-during-modal-dialog.html

[WebKit Review Bot]

Created attachment 133778 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Woh.  When did the bots start uploading layout test results again?

[Eric Seidel (no email)]

I think Darin used to work on Timers.  I certainly haven't much, but I can brush up if necessary.

[Hajime Morrita]

Created attachment 133960 [details]
Patch

[Hajime Morrita]

> I think Darin used to work on Timers.  I certainly haven't much, but I can brush up if necessary.
Right.
Darin, could you take a look?
My goal of this series of changes is to introduce debug information which tracks the caller location
like line number and file names. I'd like to make some cleanup before that though.

[WebKit Review Bot]

Comment on attachment 133960 [details]
Patch

Attachment 133960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12141361

New failing tests:
inspector/console/console-long-eval-crash.html
fast/animation/request-animation-frame-during-modal.html
fast/events/scroll-event-during-modal-dialog.html

[WebKit Review Bot]

Created attachment 133982 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 134190 [details]
Patch

[Hajime Morrita]

It's green! Mr. Adler, could you please take a look?

[Hajime Morrita]

Created attachment 136843 [details]
Patch

[Hajime Morrita]

Created attachment 136846 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136846 [details]
Patch

Attachment 136846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12393270

New failing tests:
inspector/console/console-long-eval-crash.html
fast/animation/request-animation-frame-during-modal.html
fast/events/scroll-event-during-modal-dialog.html

[WebKit Review Bot]

Created attachment 136860 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 136846 [details]
Patch

Attachment 136846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12391357

New failing tests:
inspector/console/console-long-eval-crash.html
fast/animation/request-animation-frame-during-modal.html
fast/events/scroll-event-during-modal-dialog.html

[WebKit Review Bot]

Created attachment 136873 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 137271 [details]
Updated to ToT

[Hajime Morrita]

Darin, could you take a look? I'm planning to add a debug facility to the Timer. 
This is a preparation for that.

[Hajime Morrita]

Created attachment 141318 [details]
Updated to ToT

[Hajime Morrita]

Created attachment 174094 [details]
Patch

[Darin Adler]

Comment on attachment 174094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174094&action=review

I’m not entirely convinced this refactoring is a significant improvement. However, you say “more simplification to come” so I am intrigued.

> Source/WebCore/ChangeLog:33
> +        (WebCore):

Please take the time to remove bogus lines like this from the change log.

> Source/WebCore/platform/ThreadTimers.cpp:59
> +ThreadTimers::~ThreadTimers()
> +{
> +}

Why?

> Source/WebCore/platform/ThreadTimers.h:44
> +        ~ThreadTimers();

Why?

> Source/WebCore/platform/ThreadTimers.h:59
> +        OwnPtr<TimerScheduler> m_scheduler;

Lets just do:

    TimerScheduler m_scheduler;

Instead of using an OwnPtr. It’s true that using an OwnPtr means we don’t have to include the TimerScheduler.h header, but in my opinion that’s not a big enough benefit to justify an extra memory block per thread. Not that many other headers have to include ThreadTimers.h, so it’s no big deal to have ThreadTimers.h include TimerScheduler.h.

> Source/WebCore/platform/Timer.cpp:96
>      // Timers should be in the heap if and only if they have a non-zero next fire time.
>      ASSERT(inHeap() == (m_nextFireTime != 0));
>      if (inHeap())

These lines of code should move into the timer scheduler/heap class and not be here.

> Source/WebCore/platform/TimerScheduler.cpp:52
> +class TimerHeapReference;

This should go earlier in the file. Forward declarations should come before other kinds of definitions.

> Source/WebCore/platform/TimerScheduler.cpp:62
> +class TimerHeapPointer {

All of this code is showing up as new in the diff, so that means this patch was not prepared in the way that lets me review it properly. I want to review any changes to this code, but instead it’s all showing up as if it was newly written code.

> Source/WebCore/platform/TimerScheduler.h:38
> +class TimerScheduler {

This class should be named TimerHeap. Its primary purpose is to encapsulate the heap’s job of keeping determining which timer is next to fire and how soon it is.

> Source/WebCore/platform/TimerScheduler.h:55
> +    void fire();
> +    void allowReentrancy() { m_firing = false; }
> +    bool isFiring() const { return m_firing; }
> +    bool isEmpty() const { return m_heap.isEmpty(); }
> +    bool canStopSharedTimer() const { return isFiring() || isEmpty(); }
> +    double nextFireTime() const;
> +    Vector<TimerBase*>& heap() { return m_heap; }

This set of functions seems disorganized. They should be structured so it’s easier to understand how this class is used.

Why does heap() need to be exposed? That seems to defeat the purpose of the class a bit.

> Source/WebCore/platform/TimerScheduler.h:57
> +    void checkIndexOf(const TimerBase*);

I’m not fond of the “of” suffixes here. I think checkIndex that takes a TimerBase is clear enough, and the word “of” doesn’t help that much.

> Source/WebCore/platform/TimerScheduler.h:58
> +    TimerActivationResult setNextFireTimeOf(TimerBase*, double newTime);

Returning this enum seems like an awkward way for this function to tell whether the shared timer needs to be updated. Lets see if we can find a simpler way to indicate that. In particular “needs update” and “done” do not seem clear concepts here. The real issue is that this function knows whether it changed nextFireTime for the entire scheduler or not. That’s the concept of the return value. So it should be named something more like “heap next fire time changed”.

It also seems quite strange to set the next fire time of a timer by calling the scheduler. Instead, I would have a function called nextFireTimeChanged that takes a Timer and the old next fire time. The Timer class would take care of modifying its own data members, and the scheduler would just deal with the heap aspect.

It seems to me that the TimerScheduler, which I would call TimerHeap, owns the m_heapIndex data member, but the rest of the data members are owned by Timer itself and should not be directly manipulated by this scheduler.

> Source/WebCore/platform/TimerScheduler.h:64
> +    void decreaseKeyOf(TimerBase*);
> +    void increaseKeyOf(TimerBase*);

I would name these keyDecreased and keyIncreased, since they are not called to decrease or increase a key, but rather to respond to a decrease or increase to the key that has already happened.

[Darin Adler]

(In reply to comment #22)
> Darin, could you take a look? I'm planning to add a debug facility to the Timer. 
> This is a preparation for that.

Sorry it took me so long to get to this. I finally had enough free time to read it over.

[Hajime Morrita]

(In reply to comment #26)
> 
> Sorry it took me so long to get to this. I finally had enough free time to read it over.

Thanks much for taking time to review this and bunch of other patches! 
I greatly appreciate it. Will revise them one by one.

[Hajime Morrita]

Created attachment 183327 [details]
Patch

[Hajime Morrita]

Comment on attachment 174094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174094&action=review

>> Source/WebCore/ChangeLog:33
>> +        (WebCore):
> 
> Please take the time to remove bogus lines like this from the change log.

Done.

>> Source/WebCore/platform/ThreadTimers.h:59
>> +        OwnPtr<TimerScheduler> m_scheduler;
> 
> Lets just do:
> 
>     TimerScheduler m_scheduler;
> 
> Instead of using an OwnPtr. It’s true that using an OwnPtr means we don’t have to include the TimerScheduler.h header, but in my opinion that’s not a big enough benefit to justify an extra memory block per thread. Not that many other headers have to include ThreadTimers.h, so it’s no big deal to have ThreadTimers.h include TimerScheduler.h.

OK, did it.

>> Source/WebCore/platform/Timer.cpp:96
>>      if (inHeap())
> 
> These lines of code should move into the timer scheduler/heap class and not be here.

Right. Moved.

>> Source/WebCore/platform/TimerScheduler.cpp:62
>> +class TimerHeapPointer {
> 
> All of this code is showing up as new in the diff, so that means this patch was not prepared in the way that lets me review it properly. I want to review any changes to this code, but instead it’s all showing up as if it was newly written code.

Sorry for that. This is my fault :-/
The revised patch does better job for this.

>> Source/WebCore/platform/TimerScheduler.h:38
>> +class TimerScheduler {
> 
> This class should be named TimerHeap. Its primary purpose is to encapsulate the heap’s job of keeping determining which timer is next to fire and how soon it is.

I realized that "scheduler" was exaggerated and misleading.
So I renamed it TimerSet since it supports deletion of item and which isn't typical heap does.

>> Source/WebCore/platform/TimerScheduler.h:55
>> +    Vector<TimerBase*>& heap() { return m_heap; }
> 
> This set of functions seems disorganized. They should be structured so it’s easier to understand how this class is used.
> 
> Why does heap() need to be exposed? That seems to defeat the purpose of the class a bit.

Done. Now heap() access is hidden behind functions for specific purposes.

>> Source/WebCore/platform/TimerScheduler.h:57
>> +    void checkIndexOf(const TimerBase*);
> 
> I’m not fond of the “of” suffixes here. I think checkIndex that takes a TimerBase is clear enough, and the word “of” doesn’t help that much.

Killed of-s.

>> Source/WebCore/platform/TimerScheduler.h:58
>> +    TimerActivationResult setNextFireTimeOf(TimerBase*, double newTime);
> 
> Returning this enum seems like an awkward way for this function to tell whether the shared timer needs to be updated. Lets see if we can find a simpler way to indicate that. In particular “needs update” and “done” do not seem clear concepts here. The real issue is that this function knows whether it changed nextFireTime for the entire scheduler or not. That’s the concept of the return value. So it should be named something more like “heap next fire time changed”.
> 
> It also seems quite strange to set the next fire time of a timer by calling the scheduler. Instead, I would have a function called nextFireTimeChanged that takes a Timer and the old next fire time. The Timer class would take care of modifying its own data members, and the scheduler would just deal with the heap aspect.
> 
> It seems to me that the TimerScheduler, which I would call TimerHeap, owns the m_heapIndex data member, but the rest of the data members are owned by Timer itself and should not be directly manipulated by this scheduler.

You are totally right. This was one which was mislead by the class name.
Did rewrite these for clarify the responsibility of each class.

>> Source/WebCore/platform/TimerScheduler.h:64
>> +    void increaseKeyOf(TimerBase*);
> 
> I would name these keyDecreased and keyIncreased, since they are not called to decrease or increase a key, but rather to respond to a decrease or increase to the key that has already happened.

Right. renamed.

[Hajime Morrita]

(In reply to comment #25)
> 
> I’m not entirely convinced this refactoring is a significant improvement. However, you say “more simplification to come” so I am intrigued.
>

I'd note that one of my goal here is to kill Timer::m_heapIndex. 
It makes whole Timer code fairly complex, and in my experience, it looks kinda premature optimization.
I've never seen these timer update function during performant hunting.
Updating m_heapIndex during the heap operation also pollutes the cache since the timer instance 
is embedded in each Timer's owner. Getting rid of it also helps in this point.

Another goal was to add some debug facility to Timer for hunting a crash bug.
But I no longer have immediate plan to do that. (Another crash bug might force me to do that...)

[Build Bot]

Comment on attachment 183327 [details]
Patch

Attachment 183327 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15938438

[WebKit Review Bot]

Comment on attachment 183327 [details]
Patch

Attachment 183327 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16187591

[Anders Carlsson]

Comment on attachment 183327 [details]
Patch

I don't think this is something we want to pursuit.