Bug 109765

Summary: Reschedule shared CFRunLoopTimer instead of reconstructing it
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: darin, kling, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109856, 110128    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch2 kling: review+

Description Antti Koivisto 2013-02-13 16:13:05 PST
It is much faster to reschedule with CFRunLoopTimerSetNextFireDate than to repeatedly delete and create timers.
Comment 1 Antti Koivisto 2013-02-13 16:20:08 PST
Created attachment 188213 [details]
patch
Comment 2 Andreas Kling 2013-02-13 16:38:21 PST
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.
Comment 3 Antti Koivisto 2013-02-13 17:05:52 PST
http://trac.webkit.org/changeset/142825
Comment 4 Darin Adler 2013-02-14 09:45:33 PST
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.
Comment 5 Antti Koivisto 2013-02-14 12:48:40 PST
(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.
Comment 6 WebKit Review Bot 2013-02-14 13:29:03 PST
Re-opened since this is blocked by bug 109856
Comment 7 Antti Koivisto 2013-02-14 13:31:33 PST
This caused failures in some inspector debugger tests

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r142826%20(5930)/results.html:
Comment 8 Antti Koivisto 2013-02-14 13:33:00 PST
Maybe nested event loops are causing problems or something?
Comment 9 Antti Koivisto 2013-02-16 09:54:18 PST
Created attachment 188716 [details]
patch2

try to fix the inspector debugger
Comment 10 Andreas Kling 2013-02-16 21:28:56 PST
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?
Comment 11 Antti Koivisto 2013-02-17 10:39:33 PST
(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.
Comment 12 Andreas Kling 2013-02-17 14:25:52 PST
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.
Comment 13 Antti Koivisto 2013-02-18 06:30:37 PST
http://trac.webkit.org/changeset/143210
Comment 14 WebKit Review Bot 2013-02-18 09:00:53 PST
Re-opened since this is blocked by bug 110128
Comment 15 Antti Koivisto 2013-02-18 09:05:04 PST
The bots still see some nested runloop related failures.
Comment 16 Eric Seidel (no email) 2013-03-01 10:39:55 PST
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.