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>.
Created attachment 248753 [details] Patch
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
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
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
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
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.
The attached results say the test fails with a timeout, it does not say anything about a crash.
Yes, that's what it says. But if you look at the bot, there are crash logs.
Created attachment 248771 [details] Patch
I already tried to do this in bug 109765. Didn't stick.
That patch also tries (and fails) to deal with nested runloops.
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
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
(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.
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
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
Created attachment 248796 [details] Patch
Created attachment 248798 [details] Patch
Created attachment 248799 [details] Patch
Created attachment 248800 [details] Patch
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
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
Created attachment 248817 [details] Patch
Created attachment 248824 [details] Patch
Created attachment 248825 [details] Patch
Created attachment 248826 [details] Patch
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.
Created attachment 248856 [details] Patch
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
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
Created attachment 248864 [details] Patch
Created attachment 248867 [details] Patch
Created attachment 248870 [details] Patch
Comment on attachment 248870 [details] Patch r=me, nice Did you test on iOS?
(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.
Comment on attachment 248870 [details] Patch Clearing flags on attachment: 248870 Committed r181671: <http://trac.webkit.org/changeset/181671>
All reviewed patches have been landed. Closing bug.