Support throttling of DOMTimers using nested setTimeout() calls

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 240762 [details]
Patch

[WebKit Commit Bot]

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 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 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]

Created attachment 240768 [details]
Patch

[WebKit Commit Bot]

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 on attachment 240768 [details]
Patch

Clearing flags on attachment: 240768

Committed r175441: <http://trac.webkit.org/changeset/175441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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]

(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]

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]

(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]

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)]

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]

(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]

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]

(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]

Reopening as the patch was reverted for causing intermittent crashes.

[Chris Dumez]

Created attachment 241120 [details]
Patch

[Chris Dumez]

Created attachment 241299 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241299 [details]
Patch

Clearing flags on attachment: 241299

Committed r175830: <http://trac.webkit.org/changeset/175830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.