Bug 45362 - Reduce minimum DOMTimer interval
Summary: Reduce minimum DOMTimer interval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Matthew Delaney
URL:
Keywords:
Depends on: 46307
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-07 19:54 PDT by Matthew Delaney
Modified: 2010-09-23 13:35 PDT (History)
15 users (show)

See Also:


Attachments
Patch (14.77 KB, patch)
2010-09-09 18:53 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (15.29 KB, patch)
2010-09-10 17:56 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Lower DOMTimer's min interval to 3ms (18.41 KB, patch)
2010-09-13 17:05 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2010-09-16 16:09 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (7.95 KB, patch)
2010-09-16 18:02 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2010-09-16 18:25 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2010-09-22 11:15 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (5.74 KB, patch)
2010-09-23 12:17 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Delaney 2010-09-07 19:54:51 PDT
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.
Comment 1 Matthew Delaney 2010-09-09 18:53:28 PDT
Created attachment 67137 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-09-09 19:06:33 PDT
Do we want to keep WebKit2 in sync?
Comment 3 Matthew Delaney 2010-09-10 17:56:14 PDT
Created attachment 67276 [details]
Patch
Comment 4 Early Warning System Bot 2010-09-10 18:14:42 PDT
Attachment 67276 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3942360
Comment 5 WebKit Review Bot 2010-09-11 01:58:10 PDT
Attachment 67276 [details] did not build on win:
Build output: http://queues.webkit.org/results/3908379
Comment 6 Matthew Delaney 2010-09-13 17:05:51 PDT
Created attachment 67491 [details]
Lower DOMTimer's min interval to 3ms

Updated to build on windows.
Comment 7 James Robinson 2010-09-15 14:26:36 PDT
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.
Comment 8 Simon Fraser (smfr) 2010-09-16 10:50:19 PDT
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 @@
> +2010-09-13  Matthew Delaney  <mdelaney@apple.com>
> +
> +        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.
Comment 9 Simon Fraser (smfr) 2010-09-16 10:58:02 PDT
Bug 45763 says to change Chromium to 3ms.
Comment 10 Matthew Delaney 2010-09-16 11:01:56 PDT
@ 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.
Comment 11 Mike Belshe 2010-09-16 11:07:14 PDT
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.
Comment 12 Matthew Delaney 2010-09-16 16:09:59 PDT
Created attachment 67856 [details]
Patch
Comment 13 Simon Fraser (smfr) 2010-09-16 16:42:40 PDT
Comment on attachment 67856 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 67677)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2010-09-16  Matthew Delaney  <mdelaney@apple.com>
> +
> +        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 @@
> +2010-09-16  Matthew Delaney  <mdelaney@apple.com>
> +
> +        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.
Comment 14 Matthew Delaney 2010-09-16 18:02:13 PDT
Created attachment 67869 [details]
Patch
Comment 15 Simon Fraser (smfr) 2010-09-16 18:08:55 PDT
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"
Comment 16 Matthew Delaney 2010-09-16 18:25:42 PDT
Created attachment 67873 [details]
Patch
Comment 17 WebKit Commit Bot 2010-09-17 16:23:26 PDT
Comment on attachment 67873 [details]
Patch

Clearing flags on attachment: 67873

Committed r67758: <http://trac.webkit.org/changeset/67758>
Comment 18 WebKit Commit Bot 2010-09-17 16:23:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Eric Seidel (no email) 2010-09-17 16:26:32 PDT
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.
Comment 20 Darin Fisher (:fishd, Google) 2010-09-17 16:29:05 PDT
It will still be tough to make such a test not be flaky.
Comment 21 Simon Fraser (smfr) 2010-09-17 16:40:04 PDT
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.
Comment 22 Matthew Delaney 2010-09-22 11:15:42 PDT
Created attachment 68399 [details]
Patch
Comment 23 Darin Adler 2010-09-22 11:22:47 PDT
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.
Comment 24 Matthew Delaney 2010-09-23 12:17:32 PDT
Created attachment 68562 [details]
Patch
Comment 25 WebKit Commit Bot 2010-09-23 13:35:39 PDT
Comment on attachment 68562 [details]
Patch

Clearing flags on attachment: 68562

Committed r68188: <http://trac.webkit.org/changeset/68188>
Comment 26 WebKit Commit Bot 2010-09-23 13:35:46 PDT
All reviewed patches have been landed.  Closing bug.