[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling #10259

zdobersek · 2023-02-17T10:25:49Z

`06b9ec1`

[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling
https://bugs.webkit.org/show_bug.cgi?id=252467

Reviewed by Carlos Garcia Campos.

In ThreadedCompositor, a timer is established on the composition thread that is
used to mimic display updates at the desired update frequency. This is necessary
in order to provide opportunities for update producers like the scrolling thread
to actually enact the desired composition changes, like scrolling, independently
of the main thread.

The timer is put on the composition thread, matching the compositing-thread
update timer in priority. Its delay is based on the established frequency of the
display updates.

ThreadedCompositor::displayUpdateFired() method is added and is dispatched upon
every firing of the timer. It notifies the scrolling thread(s) for the given
display ID through the EventDispatcher instance on the WebProcess singleton,
and the next firing of the timer is then scheduled.

When ThreadedCompositor enters into the composition phase, the display update
timer is stopped, and the displayUpdateFired() dispatcher is invoked manually
when the frame-complete signal is returned from the embedder's environment. This
allows aligning the display update timer with the environment's vsync signals
without disturbing the composition itself.

The target update frequency logic is pulled from ThreadedDisplayRefreshMonitor
into ThreadedCompositor, with the desired DisplayUpdate object now also kept in
that class. In order for ThreadedDisplayRefreshMonitor to use a sensible
DisplayUpdate object for each of its own dispatches, the DisplayUpdate object
is passed through the requiresDisplayRefreshCallback() method to the monitor,
where it's then stored for next invocation on the main thread, if it occurs.

ScrollingTreeNicosia::applyLayerPositionsInternal() override is provided,
establishing an update scope when running on the scrolling thread. This will
cause the applied positions to actually go through composition whenever the
scrolling is being handled on the scrolling thread, meaning outside of the usual
update process.

* Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.cpp:
(WebCore::ScrollingTreeNicosia::applyLayerPositionsInternal):
* Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.h:
* Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:
(WebKit::m_displayRefreshMonitor):
(WebKit::ThreadedCompositor::renderLayerTree):
(WebKit::ThreadedCompositor::sceneUpdateFinished):
(WebKit::ThreadedCompositor::frameComplete):
(WebKit::ThreadedCompositor::targetRefreshRateDidChange):
(WebKit::ThreadedCompositor::displayUpdateFired):
* Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:
* Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedDisplayRefreshMonitor.cpp:
(WebKit::ThreadedDisplayRefreshMonitor::ThreadedDisplayRefreshMonitor):
(WebKit::ThreadedDisplayRefreshMonitor::requiresDisplayRefreshCallback):
(WebKit::ThreadedDisplayRefreshMonitor::invalidate):
(WebKit::ThreadedDisplayRefreshMonitor::displayRefreshCallback):
(WebKit::ThreadedDisplayRefreshMonitor::setTargetRefreshRate): Deleted.
* Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedDisplayRefreshMonitor.h:
* Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:
(WebKit::LayerTreeHost::displayDidRefresh):

Canonical link: https://commits.webkit.org/261124@main

3817582

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	✅ 🧪 gtk-wk2
	~~🛠 tv~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	~~🛠 watch-sim~~

webkit-early-warning-system · 2023-02-17T10:29:40Z

EWS run on previous version of this PR (hash 8bbed56)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	❌ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-27T17:13:01Z

EWS run on previous version of this PR (hash c086f4f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-03-01T09:53:54Z

EWS run on previous version of this PR (hash cde889c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

carlosgcampos · 2023-03-01T11:31:57Z

Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp

+{
+    m_display.displayUpdate = m_display.displayUpdate.nextUpdate();
+
+    WebProcess::singleton().eventDispatcher().notifyScrollingTreesDisplayDidRefresh(m_display.displayID);


Why did you remove the Client::displayDidRefresh? EventDispatcher::notifyScrollingTreesDisplayDidRefresh iterates all scrolling trees to notify that a display has been refreshed. Every tree checks if its displayID matches the given one. This is required when there's a global display notifying about a refresh, but in our case we have one display refresh monitor with its own displayID for every threaded compositor. So, we know what tree we wat to notify without having to iterate all trees.

This is called from the composition thread, so LayerTreeHost::displayDidRefresh() ends up loading the Page's ScrollingCoordinator and then its ScrollingTree from a non-main thread.

Using EventDispatcher avoids that, and it locks over the scrolling trees that are registered and unregistered on the main thread, so no threading problems.

I see, in that case I would add:

void EventDispatcher::notifyWebPageScrollingTreeDisplayDidRefresh(WebPage& page, PlatformDisplayID displayID);

And call it from LayerTreeHost::displayDidRefresh. This avoids the tree iteration, fixes the threading issues and doesn't use WebProcess API from Shared.

I assume you would want to short-cut to the ScrollingTree in the new EventDispatcher method, through the WebPage. But that still operates on that WebPage on a non-main thread, so threading problems would remain.

Calling ThreadedScrollingTree::displayDidRefresh() on every scrolling tree isn't ideal, and it can be improved, but I don't see it as critical.

It would be just getting the identifier that is the key of the trees map. Anyway, it's not critical since in most of the cases there's only one page per process. But still I prefer to call the dispatcher from LayerTreeHost::displayDidRefresh, which is in WebProcess layer too. At some point we should move the threaded compositor code from Shared to WebProcess and stop pretending it's shared, though.

Yes, I'm adding back LayerTreeHost::displayDidRefresh.

webkit-early-warning-system · 2023-03-02T12:18:19Z

EWS run on previous version of this PR (hash 9f44e64)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

carlosgcampos

I still get some blocks, but less often, so let's try this. Thanks!

Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp

webkit-early-warning-system · 2023-03-03T08:02:27Z

EWS run on current version of this PR (hash 3817582)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	✅ 🧪 gtk-wk2
	~~🛠 tv~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	~~🛠 watch-sim~~

…ter scrolling handling https://bugs.webkit.org/show_bug.cgi?id=252467 Reviewed by Carlos Garcia Campos. In ThreadedCompositor, a timer is established on the composition thread that is used to mimic display updates at the desired update frequency. This is necessary in order to provide opportunities for update producers like the scrolling thread to actually enact the desired composition changes, like scrolling, independently of the main thread. The timer is put on the composition thread, matching the compositing-thread update timer in priority. Its delay is based on the established frequency of the display updates. ThreadedCompositor::displayUpdateFired() method is added and is dispatched upon every firing of the timer. It notifies the scrolling thread(s) for the given display ID through the EventDispatcher instance on the WebProcess singleton, and the next firing of the timer is then scheduled. When ThreadedCompositor enters into the composition phase, the display update timer is stopped, and the displayUpdateFired() dispatcher is invoked manually when the frame-complete signal is returned from the embedder's environment. This allows aligning the display update timer with the environment's vsync signals without disturbing the composition itself. The target update frequency logic is pulled from ThreadedDisplayRefreshMonitor into ThreadedCompositor, with the desired DisplayUpdate object now also kept in that class. In order for ThreadedDisplayRefreshMonitor to use a sensible DisplayUpdate object for each of its own dispatches, the DisplayUpdate object is passed through the requiresDisplayRefreshCallback() method to the monitor, where it's then stored for next invocation on the main thread, if it occurs. ScrollingTreeNicosia::applyLayerPositionsInternal() override is provided, establishing an update scope when running on the scrolling thread. This will cause the applied positions to actually go through composition whenever the scrolling is being handled on the scrolling thread, meaning outside of the usual update process. * Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.cpp: (WebCore::ScrollingTreeNicosia::applyLayerPositionsInternal): * Source/WebCore/page/scrolling/nicosia/ScrollingTreeNicosia.h: * Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp: (WebKit::m_displayRefreshMonitor): (WebKit::ThreadedCompositor::renderLayerTree): (WebKit::ThreadedCompositor::sceneUpdateFinished): (WebKit::ThreadedCompositor::frameComplete): (WebKit::ThreadedCompositor::targetRefreshRateDidChange): (WebKit::ThreadedCompositor::displayUpdateFired): * Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h: * Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedDisplayRefreshMonitor.cpp: (WebKit::ThreadedDisplayRefreshMonitor::ThreadedDisplayRefreshMonitor): (WebKit::ThreadedDisplayRefreshMonitor::requiresDisplayRefreshCallback): (WebKit::ThreadedDisplayRefreshMonitor::invalidate): (WebKit::ThreadedDisplayRefreshMonitor::displayRefreshCallback): (WebKit::ThreadedDisplayRefreshMonitor::setTargetRefreshRate): Deleted. * Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedDisplayRefreshMonitor.h: * Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp: (WebKit::LayerTreeHost::displayDidRefresh): Canonical link: https://commits.webkit.org/261124@main

webkit-commit-queue · 2023-03-03T09:21:30Z

Committed 261124@main (06b9ec1): https://commits.webkit.org/261124@main

Reviewed commits have been landed. Closing PR #10259 and removing active labels.

zdobersek requested review from magomez and cdumez as code owners February 17, 2023 10:25

zdobersek self-assigned this Feb 17, 2023

zdobersek added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Feb 17, 2023

zdobersek marked this pull request as draft February 18, 2023 10:02

zdobersek force-pushed the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch from 8bbed56 to c086f4f Compare February 27, 2023 17:12

zdobersek force-pushed the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch from c086f4f to cde889c Compare March 1, 2023 09:53

zdobersek marked this pull request as ready for review March 1, 2023 10:06

zdobersek requested a review from carlosgcampos March 1, 2023 10:07

carlosgcampos reviewed Mar 1, 2023

View reviewed changes

zdobersek force-pushed the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch from cde889c to 9f44e64 Compare March 2, 2023 12:18

carlosgcampos approved these changes Mar 2, 2023

View reviewed changes

Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp Show resolved Hide resolved

Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp Outdated Show resolved Hide resolved

zdobersek force-pushed the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch from 9f44e64 to 3817582 Compare March 3, 2023 08:02

zdobersek added the merge-queue Applied to send a pull request to merge-queue label Mar 3, 2023

webkit-early-warning-system force-pushed the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch from 3817582 to 06b9ec1 Compare March 3, 2023 09:21

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 3, 2023

webkit-early-warning-system merged commit 06b9ec1 into WebKit:main Mar 3, 2023

zdobersek deleted the eng/CoordinatedGraphics-Provide-scheduled-display-update-events-for-better-scrolling-handling branch March 3, 2023 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling #10259

[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling #10259

zdobersek commented Feb 17, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 17, 2023 •

edited

webkit-early-warning-system commented Feb 27, 2023 •

edited

webkit-early-warning-system commented Mar 1, 2023 •

edited

carlosgcampos Mar 1, 2023

zdobersek Mar 1, 2023

carlosgcampos Mar 2, 2023

zdobersek Mar 2, 2023

carlosgcampos Mar 2, 2023

zdobersek Mar 2, 2023

webkit-early-warning-system commented Mar 2, 2023 •

edited

carlosgcampos left a comment

webkit-early-warning-system commented Mar 3, 2023 •

edited

webkit-commit-queue commented Mar 3, 2023

[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling #10259

[CoordinatedGraphics] Provide scheduled display-update events for better scrolling handling #10259

Conversation

zdobersek commented Feb 17, 2023 • edited by webkit-early-warning-system

06b9ec1

webkit-early-warning-system commented Feb 17, 2023 • edited

webkit-early-warning-system commented Feb 27, 2023 • edited

webkit-early-warning-system commented Mar 1, 2023 • edited

carlosgcampos Mar 1, 2023

Choose a reason for hiding this comment

zdobersek Mar 1, 2023

Choose a reason for hiding this comment

carlosgcampos Mar 2, 2023

Choose a reason for hiding this comment

zdobersek Mar 2, 2023

Choose a reason for hiding this comment

carlosgcampos Mar 2, 2023

Choose a reason for hiding this comment

zdobersek Mar 2, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 2, 2023 • edited

carlosgcampos left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 3, 2023 • edited

webkit-commit-queue commented Mar 3, 2023

zdobersek commented Feb 17, 2023 •

edited by webkit-early-warning-system

`06b9ec1`

webkit-early-warning-system commented Feb 17, 2023 •

edited

webkit-early-warning-system commented Feb 27, 2023 •

edited

webkit-early-warning-system commented Mar 1, 2023 •

edited

webkit-early-warning-system commented Mar 2, 2023 •

edited

webkit-early-warning-system commented Mar 3, 2023 •

edited