[webkit-reviews] review denied: [Bug 82176] [Refactoring] Introduce TimerScheduler to encapsulate heap structure. : [Attachment 174094] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Hajime Morrita <morrita at google.com>'s
request for review:
Bug 82176: [Refactoring] Introduce TimerScheduler to encapsulate heap
structure.
https://bugs.webkit.org/show_bug.cgi?id=82176

Attachment 174094: Patch
https://bugs.webkit.org/attachment.cgi?id=174094&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.