Bug 198664

Summary: REGRESSION (r244182) [WK1]: Page updates should always scheduleCompositingLayerFlush() immediately
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, ews-watchlist, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=177484
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future
none
Patch none

Said Abou-Hallawa
Reported 2019-06-07 10:03:53 PDT
WK1 is a single process which means the UI and the core functionalities are in the same process. If the page scrolls, it will needs update so it does scheduleRenderingUpdate(). scheduleRenderingUpdate() times calling the scheduleCompositingLayerFlush() via the DisplayRefreshMonitor. This timing on a single process makes the frame to be missed almost always. Therefore scrolling in WK1 is stuttering. Servicing rAF callbacks, WebAnimations, IntersectionObserver and ResizeObserver still have to time the next RenderingUpdate through DisplayRefreshMonitor as it is currently implemented.
Attachments
Patch (25.04 KB, patch)
2019-06-07 10:32 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (15.79 KB, patch)
2019-06-07 12:44 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (10.23 KB, patch)
2019-06-07 16:17 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (17.22 KB, patch)
2019-06-07 18:02 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews213 for win-future (13.33 MB, application/zip)
2019-06-07 19:37 PDT, EWS Watchlist 		no flags
Details
Patch (16.94 KB, patch)
2019-06-07 20:21 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-06-07 10:32:17 PDT
Created attachment 371594 [details] Patch
Said Abou-Hallawa
Comment 2 2019-06-07 10:33:02 PDT
<rdar://problem/50192194>
Simon Fraser (smfr)
Comment 3 2019-06-07 11:06:01 PDT
Comment on attachment 371594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371594&action=review > Source/WebCore/ChangeLog:22 > + -- RenderingUpdateScheduler::scheduleRenderingUpdate() is renamed to > + scheduleCompositingLayerFlush() because this is what it does when > + the DispayRefresMonitor fires. Page::updateRendering() is called > + from the flushLayers() methods. I think this is going in the wrong direction. The fact that rendering updates use composting is an implementation detail, so scheduleRenderingUpdate() is the correct name to use. > Source/WebCore/ChangeLog:24 > + -- RenderLayerCompositor::scheduleLayerFlush() will be calling will be calling -> now calls > Source/WebCore/page/RenderingUpdateScheduler.h:49 > + WEBCORE_EXPORT void scheduleCompositingLayerFlush(); Keep the old name. > Source/WebCore/page/RenderingUpdateScheduler.h:50 > + void scheduleCompositingLayerFlushImmediately(); Maybe this should be called scheduleImmediateRenderingUpdate? Or maybe scheduleRenderingUpdate () should take an enum RenderingUpdateType { Immediate, PerScreenUpdate } > Source/WebCore/rendering/RenderLayerCompositor.cpp:469 > + page().chrome().client().scheduleCompositingLayerFlush(); Why does this not go through renderingUpdateScheduler? > Tools/Tracing/SystemTracePoints.plist:-275 > - <string>Trigger rendering update</string> > - <key>Type</key> > - <string>Impulse</string> > - <key>Component</key> > - <string>47</string> > - <key>Code</key> > - <string>5029</string> > - </dict> > - <dict> > - <key>Name</key> > - <string>Rendering update</string> > - <key>Type</key> > - <string>Interval</string> > - <key>Component</key> > - <string>47</string> > - <key>CodeBegin</key> > - <string>5030</string> > - <key>CodeEnd</key> > - <string>5031</string> > - </dict> > - <dict> > - <key>Name</key> > - <string>Schedule rendering update</string> > - <key>Type</key> > - <string>Impulse</string> > - <key>Component</key> > - <string>47</string> > - <key>Code</key> > - <string>5028</string> > - </dict> > - <dict> > - <key>Name</key> > - <string>Trigger rendering update</string> Don't remove keys. This breaks backwards compatibility.
Said Abou-Hallawa
Comment 4 2019-06-07 12:44:16 PDT
Created attachment 371603 [details] Patch
Said Abou-Hallawa
Comment 5 2019-06-07 12:48:35 PDT
Comment on attachment 371594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371594&action=review >> Source/WebCore/ChangeLog:22 >> + from the flushLayers() methods. > > I think this is going in the wrong direction. The fact that rendering updates use composting is an implementation detail, so scheduleRenderingUpdate() is the correct name to use. Current name was kept. >> Source/WebCore/ChangeLog:24 >> + -- RenderLayerCompositor::scheduleLayerFlush() will be calling > > will be calling -> now calls Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.h:49 >> + WEBCORE_EXPORT void scheduleCompositingLayerFlush(); > > Keep the old name. Done. >> Source/WebCore/page/RenderingUpdateScheduler.h:50 >> + void scheduleCompositingLayerFlushImmediately(); > > Maybe this should be called scheduleImmediateRenderingUpdate? Or maybe scheduleRenderingUpdate () should take an enum RenderingUpdateType { Immediate, PerScreenUpdate } Names was changed to scheduleImmediateRenderingUpdate(). >> Source/WebCore/rendering/RenderLayerCompositor.cpp:469 >> + page().chrome().client().scheduleCompositingLayerFlush(); > > Why does this not go through renderingUpdateScheduler? RenderingUpdateScheduler::schedulePerScreenRenderingUpdate was added and was called here. >> Tools/Tracing/SystemTracePoints.plist:-275 >> - <string>Trigger rendering update</string> > > Don't remove keys. This breaks backwards compatibility. I am removing repeated entries with the codes: 5028, 5029, 5030 and 5031. In the atrace these entries appear twice.
Simon Fraser (smfr)
Comment 6 2019-06-07 12:58:41 PDT
Comment on attachment 371603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371603&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:927 > + if (Page* corePage = m_page.corePage()) > + corePage->renderingUpdateScheduler().scheduleRenderingUpdate(); This is backwards. ChromeClient is how WebCore talks to WebKit, but here WebChromeClient is going back and calling a function on Page, which is in WebCore. Callers of scheduleCompositingLayerFlush() should just change to call renderingUpdateScheduler().scheduleRenderingUpdate(). > Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h:179 > + void scheduleCompositingLayerFlushImmediately() final; scheduleImmediateCompositingLayerFlush
Said Abou-Hallawa
Comment 7 2019-06-07 16:17:00 PDT
Created attachment 371626 [details] Patch
EWS Watchlist
Comment 8 2019-06-07 16:44:06 PDT
Attachment 371626 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:323: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 9 2019-06-07 17:28:06 PDT
Comment on attachment 371626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371626&action=review > Source/WebCore/page/RenderingUpdateScheduler.h:51 > void scheduleRenderingUpdate(); > - void scheduleCompositingLayerFlush(); > + void scheduleImmediateRenderingUpdate(); > + void schedulePerScreenRenderingUpdate(); Let's call these: void scheduleTimedRenderingUpdate(); void scheduleImmediateRenderingUpdate(); void scheduleRenderingUpdate();
Said Abou-Hallawa
Comment 10 2019-06-07 18:02:20 PDT
Created attachment 371638 [details] Patch
EWS Watchlist
Comment 11 2019-06-07 18:03:34 PDT
Attachment 371638 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:323: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 12 2019-06-07 19:37:39 PDT
Comment on attachment 371638 [details] Patch Attachment 371638 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12414717 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 13 2019-06-07 19:37:41 PDT
Created attachment 371642 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Said Abou-Hallawa
Comment 14 2019-06-07 20:21:15 PDT
Created attachment 371644 [details] Patch
EWS Watchlist
Comment 15 2019-06-07 20:23:51 PDT
Attachment 371644 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:323: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 16 2019-06-07 22:20:02 PDT
Comment on attachment 371644 [details] Patch Clearing flags on attachment: 371644 Committed r246231: <https://trac.webkit.org/changeset/246231>
WebKit Commit Bot
Comment 17 2019-06-07 22:20:04 PDT
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.