Bug 109765 – Reschedule shared CFRunLoopTimer instead of reconstructing it

• Component: Platform
• Status: REOPENED

[Antti Koivisto]

It is much faster to reschedule with CFRunLoopTimerSetNextFireDate than to repeatedly delete and create timers.

[Antti Koivisto]

Created attachment 188213 [details]
patch

[Andreas Kling]

Comment on attachment 188213 [details]
patch

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

Hilarious! r+ from me since this looks like correct use of the API to me, though you might wanna double-check with someone with more CF knowledge.

> Source/WebCore/platform/mac/SharedTimerMac.mm:154
> +    static CFRunLoopTimerRef timer;
> +    if (!timer) {

You could use dispatch_once() for this block.

[Antti Koivisto]

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

[Darin Adler]

When I first implemented SharedTimer I tried using CFRunLoopTimerSetNextFireDate and it didn’t work. Maybe I did it wrong. Or maybe there were CF bugs that have since been fixed. Someone should be sure to test this on the oldest OS X and iOS versions that WebKit currently supports in case that old bug is not fixed there.

[Antti Koivisto]

(In reply to comment #4)
> When I first implemented SharedTimer I tried using CFRunLoopTimerSetNextFireDate and it didn’t work. Maybe I did it wrong. Or maybe there were CF bugs that have since been fixed. Someone should be sure to test this on the oldest OS X and iOS versions that WebKit currently supports in case that old bug is not fixed there.

JSC already uses CFRunLoopTimerSetNextFireDate for HeapTimer.

[WebKit Review Bot]

Re-opened since this is blocked by bug 109856

[Antti Koivisto]

This caused failures in some inspector debugger tests

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r142826%20(5930)/results.html:

[Antti Koivisto]

Maybe nested event loops are causing problems or something?

[Antti Koivisto]

Created attachment 188716 [details]
patch2

try to fix the inspector debugger

[Andreas Kling]

Comment on attachment 188716 [details]
patch2

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

> Source/WebCore/platform/mac/SharedTimerMac.mm:177
> +void reinsertSharedTimer()
> +{
> +    // For some reason the timer won't fire in a nested runloop unless it has been freshly inserted.
> +    CFRunLoopTimerInvalidate(globalSharedTimer);
> +    CFRelease(globalSharedTimer);
> +    globalSharedTimer = 0;
> +}

This function's name seems to be at odds with its contents.
Where does the timer actually get re-inserted?

[Antti Koivisto]

(In reply to comment #10)
> This function's name seems to be at odds with its contents.
> Where does the timer actually get re-inserted?

From sharedTimer(), next time the timer is accessed. This will happen from ThreadTimers::fireTimersInNestedEventLoop() right after this call.

Suggestions welcome.

[Andreas Kling]

Comment on attachment 188716 [details]
patch2

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

r=me if you find a nicer color for the bikeshed.

> Source/WebCore/platform/mac/SharedTimerMac.mm:171
> +void reinsertSharedTimer()

I'd just prefer to call this removeFoo() instead of reinsertBar() since that's what it does.

[Antti Koivisto]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 110128

[Antti Koivisto]

The bots still see some nested runloop related failures.

[Eric Seidel (no email)]

Comment on attachment 188213 [details]
patch

Cleared Andreas Kling's review+ from obsolete attachment 188213 [details] so that this bug does not appear in http://webkit.org/pending-commit.