Bug 45362 – Reduce minimum DOMTimer interval

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matthew Delaney]

Reduce the DOMTimer interval from 10ms down to either 3 or 4 ms for mac platform. 
Chrome apparently has had 4ms and I can't find reports of this being an issue (with regard to run away timers, battery life, CPU waste). 
Dropping to 3ms showed considerable performance benefits for Safari on a handful of performance tests(animations, canvas, etc.). Will post perf test results soon.
Will have a patch up soon after finishing a few offline discussions.

[Matthew Delaney]

Created attachment 67137 [details]
Patch

[Alexey Proskuryakov]

Do we want to keep WebKit2 in sync?

[Matthew Delaney]

Created attachment 67276 [details]
Patch

[Early Warning System Bot]

Attachment 67276 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3942360

[WebKit Review Bot]

Attachment 67276 [details] did not build on win:
Build output: http://queues.webkit.org/results/3908379

[Matthew Delaney]

Created attachment 67491 [details]
Lower DOMTimer's min interval to 3ms

Updated to build on windows.

[James Robinson]

Is there any reason to go to 3 instead of chromium's 4?  If you really want 3 then we can change chromium's as well to be consistent, but I'm curious if you have a good reason to prefer one or the other.

[Simon Fraser (smfr)]

Comment on attachment 67491 [details]
Lower DOMTimer's min interval to 3ms

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 67395)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Matthew Delaney  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Reduce Safari DOMTimer interval
> +        https://bugs.webkit.org/show_bug.cgi?id=45362
> +
> +        * WebCore.exp.in:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * page/DOMTimer.cpp: Deleted obsolete comments.
> +        * page/DOMTimer.h: Deleted obsolte comments.
> +        * page/DOMTimerSettings.cpp: Added. Exposes function to set the DOMTimer's minInterval.
> +        (WebCore::setMinTimerIntervalSetting):
> +        * page/DOMTimerSettings.h: Added.

Why not just do this through WebCore::Settings?


> Index: WebKit2/WebKit2.xcodeproj/project.pbxproj
> ===================================================================
> --- WebKit2/WebKit2.xcodeproj/project.pbxproj	(revision 67395)
> +++ WebKit2/WebKit2.xcodeproj/project.pbxproj	(working copy)
> @@ -1680,6 +1680,7 @@
> 			isa = PBXProject;
> 			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "WebKit2" */;
> 			compatibilityVersion = "Xcode 3.1";
> +			developmentRegion = English;
> 			hasScannedForEncodings = 1;
> 			knownRegions = (
> 				English,

Please don't include this project change in any of the diffs.

r- for the 3 vs. 4ms thing.

[Simon Fraser (smfr)]

Bug 45763 says to change Chromium to 3ms.

[Matthew Delaney]

@ James No. Initially, I dropped it to 3 because I was under the impression Chrome was at 3 (from conversation about it) and it wasn't until I had been using and tested 3 quite a bit that I checked the actual Chrome code and noticed it was 4ms. Do you have any thoughts on 3 vs. 4?

My thoughts: 
-4ms seems to be a safe bet because I couldn't find any mention of specific issues that Chrome has had with 4ms in the recent past.
-3ms hasn't shown any issues so far from what I've been testing.
-3ms increases perf on certain benchmarks, such as those with very rapid short drawing cycles. However, I've yet to find a benchmark with an increase from this that merits the drop to 3ms since most of these tests are already performing faster than necessary (i.e. drawing faster than is painted).

Not trying to start another endless thread, but can you think of any concrete reasons to drop to 3ms over 4ms? Otherwise, I'll bump this patch back up to 4ms and lowering below that can be revisited in the future if there are any compelling reasons to do so.

[Mike Belshe]

I'm glad to see work to bring this down.  Bug 45763 is really about matching Safari with Chromium.  I don't think there is a significant difference between 3 or 4, and either should work okay.  We'll just try to match what you decide.

We haven't seen issues at 4ms.

I don't have any data which suggests 3ms would make much difference.

[Matthew Delaney]

Created attachment 67856 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 67856 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 67677)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> [phone]  Matthew Delaney  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Reduce minimum DOMTimer interval
> +        https://bugs.webkit.org/show_bug.cgi?id=45362

You need a description here of what the problem is, and what you changed to fix it.

> +        No new tests. (OOPS!)

Remove this, and justify why you couldn't add any LayoutTests.

> +        * WebCore.exp.in:
> +        * WebCore.xcodeproj/project.pbxproj:

The project changes are bogus.

> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore/WebCore.xcodeproj/project.pbxproj	(revision 67626)
> +++ WebCore/WebCore.xcodeproj/project.pbxproj	(working copy)
> @@ -2384,8 +2384,8 @@
>  		893C47B81238A099002B3D86 /* JSFileCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 893C47B61238A099002B3D86 /* JSFileCallback.h */; };
>  		893C47BB1238A0A9002B3D86 /* JSFileWriterCallback.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 893C47B91238A0A9002B3D86 /* JSFileWriterCallback.cpp */; };
>  		893C47BC1238A0A9002B3D86 /* JSFileWriterCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 893C47BA1238A0A9002B3D86 /* JSFileWriterCallback.h */; };
> -		893C47DF123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */; };
>  		893C47CC123EEBA2002B3D86 /* JSEntryCustom.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 893C47CA123EEBA2002B3D86 /* JSEntryCustom.cpp */; };
> +		893C47DF123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */; };
>  		89878552122CA064003AABDA /* DirectoryEntry.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 89878539122CA064003AABDA /* DirectoryEntry.cpp */; };
>  		89878553122CA064003AABDA /* DirectoryEntry.h in Headers */ = {isa = PBXBuildFile; fileRef = 8987853A122CA064003AABDA /* DirectoryEntry.h */; };
>  		89878554122CA064003AABDA /* DirectoryReader.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8987853B122CA064003AABDA /* DirectoryReader.cpp */; };
> @@ -8290,8 +8290,8 @@
>  		893C47B61238A099002B3D86 /* JSFileCallback.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSFileCallback.h; sourceTree = "<group>"; };
>  		893C47B91238A0A9002B3D86 /* JSFileWriterCallback.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSFileWriterCallback.cpp; sourceTree = "<group>"; };
>  		893C47BA1238A0A9002B3D86 /* JSFileWriterCallback.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSFileWriterCallback.h; sourceTree = "<group>"; };
> -		893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSDirectoryEntryCustom.cpp; sourceTree = "<group>"; };
>  		893C47CA123EEBA2002B3D86 /* JSEntryCustom.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSEntryCustom.cpp; sourceTree = "<group>"; };
> +		893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSDirectoryEntryCustom.cpp; sourceTree = "<group>"; };
>  		89878539122CA064003AABDA /* DirectoryEntry.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = DirectoryEntry.cpp; path = fileapi/DirectoryEntry.cpp; sourceTree = "<group>"; };
>  		8987853A122CA064003AABDA /* DirectoryEntry.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DirectoryEntry.h; path = fileapi/DirectoryEntry.h; sourceTree = "<group>"; };
>  		8987853B122CA064003AABDA /* DirectoryReader.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = DirectoryReader.cpp; path = fileapi/DirectoryReader.cpp; sourceTree = "<group>"; };

These changes look unrelated.

> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 67626)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -200,6 +200,8 @@ namespace WebCore {
>          void setDOMPasteAllowed(bool);
>          bool isDOMPasteAllowed() const { return m_isDOMPasteAllowed; }
>          
> +        static void setDOMTimerMinInterval(double);

I think setMinDOMTimerInterval() would be a better name. Also, it doesn't have to be static. You should add a comment saying what the units are for the paramter (seconds, milliseconds?).

> Index: WebKit2/ChangeLog
> ===================================================================

> +        Reduce minimum DOMTimer interval
> +        https://bugs.webkit.org/show_bug.cgi?id=45362
> +
> +        * WebProcess/WebProcess.cpp: Keeping webkit2 in sync with webkit to set the dom timer's min interval down to 4ms.

You don't need the "keeping in sync" comment. Please capitalize your sentences correctly: DOM, WebKit2.

> Index: WebKit2/WebProcess/WebProcess.cpp
> ===================================================================
> --- WebKit2/WebProcess/WebProcess.cpp	(revision 67626)
> +++ WebKit2/WebProcess/WebProcess.cpp	(working copy)
> @@ -98,6 +98,7 @@ WebProcess::WebProcess()
>      // Initialize our platform strategies.
>      WebPlatformStrategies::initialize();
>  #endif // USE(PLATFORM_STRATEGIES)
> +    Settings::setDOMTimerMinInterval(0.004);

I wonder if the 0.004 should be declared somewhere central?

> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 67677)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Matthew Delaney  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Reduce minimum DOMTimer interval
> +        https://bugs.webkit.org/show_bug.cgi?id=45362
> +
> +        * WebView.cpp: 
> +        (WebView::initWithFrame): Having init with frame set the dom timer min interval down to 4ms.

Please improve the description of what you added.

[Matthew Delaney]

Created attachment 67869 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 67869 [details]
Patch

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

r=me comments addressed.

> WebCore/ChangeLog:9
> +        who hasn't had any problem with 4ms in the past 2 years as well as increasing our

Chrome, _which_ hasn't
in the past two years comma

> WebCore/page/Settings.h:203
> +        void setMinDOMTimerInterval(double); // Interval specified in ms.

Seconds, right? You call it with "0.04"

[Matthew Delaney]

Created attachment 67873 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 67873 [details]
Patch

Clearing flags on attachment: 67873

Committed r67758: <http://trac.webkit.org/changeset/67758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This change seems testable.  I'm surprised it wasn't tested.  I should be simple to make a guess at what the minimum clamp is by spinning a timer for a few seconds.  You'd just want to make sure that clamp was within the expected range.

[Darin Fisher (:fishd, Google)]

It will still be tough to make such a test not be flaky.

[Simon Fraser (smfr)]

Reopening for minor tweaks.

Dan suggests that the Settings method should be static, so as not to imply that it's possible to have a different min interval on different web views.

[Matthew Delaney]

Created attachment 68399 [details]
Patch

[Darin Adler]

Comment on attachment 68399 [details]
Patch

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

Seems OK, but the WebKit part of this could use some additional refinement.

> WebKit/mac/WebView/WebView.mm:699
> -    _private->page->settings()->setMinDOMTimerInterval(0.004);
> +    Settings::setMinDOMTimerInterval(0.004);

It doesn’t make sense to call this over and over again, each time we initialize a WebView. Instead we could call this in a method like +[WebView initialize] along with all the other one-time initialization that is done there.

> WebKit/win/WebView.cpp:2592
> -    m_page->settings()->setMinDOMTimerInterval(0.004);
> +    Settings::setMinDOMTimerInterval(0.004);

Same comment here. This should be part of one-time initialization, not per-WebView initialization.

[Matthew Delaney]

Created attachment 68562 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 68562 [details]
Patch

Clearing flags on attachment: 68562

Committed r68188: <http://trac.webkit.org/changeset/68188>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.