WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
198664
REGRESSION (
r244182
) [WK1]: Page updates should always scheduleCompositingLayerFlush() immediately
https://bugs.webkit.org/show_bug.cgi?id=198664
Summary
REGRESSION (r244182) [WK1]: Page updates should always scheduleCompositingLay...
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
Details
Formatted Diff
Diff
Patch
(15.79 KB, patch)
2019-06-07 12:44 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(10.23 KB, patch)
2019-06-07 16:17 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(17.22 KB, patch)
2019-06-07 18:02 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug