Bug 138262

Summary: Support throttling of DOMTimers using nested setTimeout() calls
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, kling, rniwa, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 136197, 138335, 138449    
Bug Blocks: 138292    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2014-10-31 15:18:02 PDT
We currently support throttling of DOMTimers under certain conditions, when the timers are repeating (installed using setInterval()). We should extend the support to DOMTimers using nested setTimeout() calls as this is quite common on the Web. I see for example that cnn.com is using such pattern and some of its timers would get throttled under our currently policy if we were to extend support. Radar: rdar://problem/18842049
Attachments
Patch (6.01 KB, patch)
2014-10-31 15:32 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (6.06 KB, patch)
2014-10-31 16:24 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (4.87 KB, patch)
2014-11-06 11:33 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (5.83 KB, patch)
2014-11-10 12:29 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2014-10-31 15:32:02 PDT
Created attachment 240762 [details] Patch
WebKit Commit Bot
Comment 2 2014-10-31 15:35:04 PDT
Attachment 240762 [details] did not pass style-queue: ERROR: Source/WebCore/page/DOMTimer.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gavin Barraclough
Comment 3 2014-10-31 15:57:34 PDT
Comment on attachment 240762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240762&action=review > Source/WebCore/page/DOMTimer.cpp:118 > + Vector<DOMTimer*> nestedTimers; Is the default internal capacity of vectors 0? – this seems common enough, maybe it's worth giving this some internal capacity, maybe just 1 or 2?
Chris Dumez
Comment 4 2014-10-31 16:02:00 PDT
Comment on attachment 240762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240762&action=review >> Source/WebCore/page/DOMTimer.cpp:118 >> + Vector<DOMTimer*> nestedTimers; > > Is the default internal capacity of vectors 0? – this seems common enough, maybe it's worth giving this some internal capacity, maybe just 1 or 2? Yes, it is 0 by default. I will update to use 1 as inline capacity.
Chris Dumez
Comment 5 2014-10-31 16:24:38 PDT
Created attachment 240768 [details] Patch
WebKit Commit Bot
Comment 6 2014-10-31 16:27:33 PDT
Attachment 240768 [details] did not pass style-queue: ERROR: Source/WebCore/page/DOMTimer.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 7 2014-10-31 17:12:52 PDT
Comment on attachment 240768 [details] Patch Clearing flags on attachment: 240768 Committed r175441: <http://trac.webkit.org/changeset/175441>
WebKit Commit Bot
Comment 8 2014-10-31 17:12:59 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 9 2014-11-01 08:41:27 PDT
jquery/manipulation.html started to time out around when this was landed. Coincidence? https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r175443%20(8606)/results.html
Chris Dumez
Comment 10 2014-11-01 13:33:13 PDT
(In reply to comment #9) > jquery/manipulation.html started to time out around when this was landed. > Coincidence? > > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/ > r175443%20(8606)/results.html I am going to check although it seems that on 1 of the bots at least, the test started failing later: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/8606 The failure seems consistent at least so it looks like a regression.
Chris Dumez
Comment 11 2014-11-01 19:53:22 PDT
A quick grep shows jquery/manipulation.html indeed involves plugins and the tests are likely executed by timers so there is a chance my change caused this. I'll double check to make sure timers get throttled when running this test. If this is the case, I'll see if this is easy fixable or not.
Chris Dumez
Comment 12 2014-11-01 20:07:45 PDT
(In reply to comment #11) > A quick grep shows jquery/manipulation.html indeed involves plugins and the > tests are likely executed by timers so there is a chance my change caused > this. I'll double check to make sure timers get throttled when running this > test. If this is the case, I'll see if this is easy fixable or not. Yes, I confirmed that my change is causing one timer to fire every second instead of every 13ms when running jquery/manipulation.html. The test still passes on my machine, it likely just runs slower, which is why it fails on some Debug bots. Maybe we can mark that test as slow for now?
Chris Dumez
Comment 13 2014-11-01 20:39:15 PDT
I marked the test as slow for Debug build for now (https://trac.webkit.org/r175456). I'll monitor the bots to make sure it helps. One possible solution to this problem would be to not throttle timers if we are running the layout tests, to make sure we don't slow down tests. I believe we already have a method somewhere in WebCore to detect that we are running the layout tests. Any thoughts on this?
Simon Fraser (smfr)
Comment 14 2014-11-02 09:27:32 PST
It sounds like the throttling changes are going to affect real content. Are we OK with that? 13ms -> 1000ms is a pretty serious throttle for a foreground page.
Chris Dumez
Comment 15 2014-11-02 09:30:50 PST
(In reply to comment #14) > It sounds like the throttling changes are going to affect real content. Are > we OK with that? 13ms -> 1000ms is a pretty serious throttle for a > foreground page. We've had this throttling in place for a while and so far we haven't run into trouble. The throttling occurs if the timer is interacting with a non-visible plugins only so the user should not see difference (in theory). My patch is merely extending timer throttling support to timers that keep rescheduling themselves using setTimeout() instead of simply using setInterval().
Chris Dumez
Comment 16 2014-11-03 11:00:37 PST
Interestingly, I have just noticed that the timer is only being throttled in debug builds. This difference in behavior is unexpected so this needs to be investigated.
Chris Dumez
Comment 17 2014-11-03 12:14:20 PST
(In reply to comment #16) > Interestingly, I have just noticed that the timer is only being throttled in > debug builds. This difference in behavior is unexpected so this needs to be > investigated. DOMTimers fire 16 times when running jquery/manipulation.html on a debug build, but only twice on a release build. As we start throttling when the nested level reaches 5, no throttling happens on a release build. Now, we just need to figure out why timers fire more often in debug builds when running this test.
Chris Dumez
Comment 18 2014-11-06 09:53:03 PST
Reopening as the patch was reverted for causing intermittent crashes.
Chris Dumez
Comment 19 2014-11-06 11:33:57 PST
Created attachment 241120 [details] Patch
Chris Dumez
Comment 20 2014-11-10 12:29:24 PST
Created attachment 241299 [details] Patch
WebKit Commit Bot
Comment 21 2014-11-10 14:06:00 PST
Comment on attachment 241299 [details] Patch Clearing flags on attachment: 241299 Committed r175830: <http://trac.webkit.org/changeset/175830>
WebKit Commit Bot
Comment 22 2014-11-10 14:06:07 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.