Bug 142752 – [Mac][iOS] setSharedTimerFireInterval() / stopSharedTimer() are expensive

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Dumez]

setSharedTimerFireInterval() / stopSharedTimer() are expensive on Mac and iOS on pages using a lot of timers.

For example, on bing.com, ~15.4% of the CPU time is spent in setSharedTimerFireInterval() and ~14.7% of the CPU time is spent in stopSharedTimer(). The expensive calls are CFRunLoopAddTimer (11.4%), CFRunLoopTimerInvalidate (14.1%), CFRunLoopTimerCreate (3.3%).

The issue is that we keep creating, adding to run loop modes, and then destroying the sharedTimer for each firing event. This is very expensive.
In such case, the CFRunLoopTimerRef documentation advises to
"""
... create a repeating timer with an initial firing time in the distant future (or the initial firing time) and a very large repeat interval—on the order of decades or more—and add it to all the necessary run loop modes. Then, when you know when the timer should fire next, you reset the firing time with CFRunLoopTimerSetNextFireDate, perhaps from the timer’s own callback function. This technique effectively produces a reusable, asynchronous timer.
""" [1].

Doing so greatly decreases CPU using in setSharedTimerFireInterval() (15.4% -> 4.6%) and stopSharedTimer() (14.6% -> 8.6%).

[1] https://developer.apple.com/library/prerelease/ios/documentation/CoreFoundation/Reference/CFRunLoopTimerRef/index.html#//apple_ref/c/func/CFRunLoopTimerSetNextFireDate

Radar: <rdar://problem/20176731>.

[Chris Dumez]

Created attachment 248753 [details]
Patch

[Build Bot]

Comment on attachment 248753 [details]
Patch

Attachment 248753 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6626488874958848

New failing tests:
inspector-protocol/dom-debugger/node-removed.html

[Build Bot]

Created attachment 248758 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 248753 [details]
Patch

Attachment 248753 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5011494735446016

New failing tests:
inspector-protocol/dom-debugger/node-removed.html

[Build Bot]

Created attachment 248759 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alexey Proskuryakov]

Oh wait, is it this patch that actually breaks inspector-protocol/dom-debugger/node-removed.html? I'm no longer sure if it was for a good reason that I disabled it in bug 142753.

[Chris Dumez]

The attached results say the test fails with a timeout, it does not say anything about  a crash.

[Alexey Proskuryakov]

Yes, that's what it says. But if you look at the bot, there are crash logs.

[Chris Dumez]

Created attachment 248771 [details]
Patch

[Antti Koivisto]

I already tried to do this in bug 109765. Didn't stick.

[Antti Koivisto]

That patch also tries (and fails) to deal with nested runloops.

[Build Bot]

Comment on attachment 248771 [details]
Patch

Attachment 248771 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5282230683303936

New failing tests:
inspector-protocol/dom-debugger/node-removed.html

[Build Bot]

Created attachment 248773 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

(In reply to comment #11)
> That patch also tries (and fails) to deal with nested runloops.

Thanks for the pointer Antti. I wasn't aware this was previously tried. I'll still try and make it work as the performance gain seems worth it.

[Build Bot]

Comment on attachment 248771 [details]
Patch

Attachment 248771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4816377592414208

New failing tests:
inspector-protocol/dom-debugger/node-removed.html

[Build Bot]

Created attachment 248775 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 248796 [details]
Patch

[Chris Dumez]

Created attachment 248798 [details]
Patch

[Chris Dumez]

Created attachment 248799 [details]
Patch

[Chris Dumez]

Created attachment 248800 [details]
Patch

[Build Bot]

Comment on attachment 248800 [details]
Patch

Attachment 248800 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5369388454641664

New failing tests:
fast/events/scroll-event-during-modal-dialog.html

[Build Bot]

Created attachment 248806 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 248817 [details]
Patch

[Chris Dumez]

Created attachment 248824 [details]
Patch

[Chris Dumez]

Created attachment 248825 [details]
Patch

[Chris Dumez]

Created attachment 248826 [details]
Patch

[Darin Adler]

Comment on attachment 248826 [details]
Patch

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

Seems really unfortunate that the iOS and Mac versions of this don’t share more code.

I wonder if there is a way to do even better on this, cutting down on the repeated calls to CFRunLoopTimerSetNextFireDate somehow.

> Source/WebCore/platform/SharedTimer.h:79
> +#if PLATFORM(MAC) || PLATFORM(IOS)

Could use PLATFORM(COCOA) for this. We often do. Or we could just put empty removeSharedTimer functions into the other platform SharedTimer implementation files. Seems worthwhile to avoid the #if if at all possible.

[Chris Dumez]

Created attachment 248856 [details]
Patch

[Build Bot]

Comment on attachment 248856 [details]
Patch

Attachment 248856 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4674012311453696

New failing tests:
fast/events/scroll-event-during-modal-dialog.html

[Build Bot]

Created attachment 248860 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 248864 [details]
Patch

[Chris Dumez]

Created attachment 248867 [details]
Patch

[Chris Dumez]

Created attachment 248870 [details]
Patch

[Antti Koivisto]

Comment on attachment 248870 [details]
Patch

r=me, nice

Did you test on iOS?

[Chris Dumez]

(In reply to comment #34)
> Comment on attachment 248870 [details]
> Patch
> 
> r=me, nice
> 
> Did you test on iOS?

Well, I tested a few sites. I initially wrote this optimization for bing.com on iOS.

[WebKit Commit Bot]

Comment on attachment 248870 [details]
Patch

Clearing flags on attachment: 248870

Committed r181671: <http://trac.webkit.org/changeset/181671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.