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.
Created attachment 67137 [details] Patch
Do we want to keep WebKit2 in sync?
Created attachment 67276 [details] Patch
Attachment 67276 [details] did not build on qt: Build output: http://queues.webkit.org/results/3942360
Attachment 67276 [details] did not build on win: Build output: http://queues.webkit.org/results/3908379
Created attachment 67491 [details] Lower DOMTimer's min interval to 3ms Updated to build on windows.
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 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.
Bug 45763 says to change Chromium to 3ms.
@ 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.
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.
Created attachment 67856 [details] Patch
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.
Created attachment 67869 [details] Patch
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"
Created attachment 67873 [details] Patch
Comment on attachment 67873 [details] Patch Clearing flags on attachment: 67873 Committed r67758: <http://trac.webkit.org/changeset/67758>
All reviewed patches have been landed. Closing bug.
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.
It will still be tough to make such a test not be flaky.
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.
Created attachment 68399 [details] Patch
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.
Created attachment 68562 [details] Patch
Comment on attachment 68562 [details] Patch Clearing flags on attachment: 68562 Committed r68188: <http://trac.webkit.org/changeset/68188>