Bug 109630 – Avoid updating timer heap when nothing changes

• Component: Platform
• Status: RESOLVED FIXED

[Antti Koivisto]

When the fire time of a Timer is changed we remove it from the timer heap and reinsert it. This is pretty slow. Turns out that in over 80% of cases the insertion position is the same as the original position. We can check if anything is actually going to change before doing this work.

[Antti Koivisto]

Created attachment 187963 [details]
patch

[WebKit Review Bot]

Attachment 187963 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/Timer.cpp', u'Source/WebCore/platform/Timer.h']" exit_code: 1
Source/WebCore/platform/Timer.cpp:354:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/Timer.cpp:356:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 188085 [details]
patch2

With stronger validity assertions, some cleanups.

[Antti Koivisto]

http://trac.webkit.org/changeset/142764

[Darin Adler]

Comment on attachment 188085 [details]
patch2

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

> Source/WebCore/platform/Timer.h:87
> -    const Vector<TimerBase*>& timerHeap() const { ASSERT(m_cachedThreadGlobalTimerHeap); return *m_cachedThreadGlobalTimerHeap; }
> -    Vector<TimerBase*>& timerHeap() { ASSERT(m_cachedThreadGlobalTimerHeap); return *m_cachedThreadGlobalTimerHeap; }
> +    Vector<TimerBase*>& timerHeap() const { ASSERT(m_cachedThreadGlobalTimerHeap); return *m_cachedThreadGlobalTimerHeap; }

Why this change?

[Antti Koivisto]

(In reply to comment #5)
> Why this change?

Alexey pointed out in Bug 109597 that it didn't make too much sense to have both as the vector items are mutable anyway.