Changeset 286932 in webkit

Timestamp:

Dec 12, 2021, 6:42:35 PM (5 years ago)

Author:

Simon Fraser

Message:

Ensure that the scrolling thread always commits layer position changes to reduce scrolling stutters
​https://bugs.webkit.org/show_bug.cgi?id=234213

Reviewed by Tim Horton.

On a page where CA commits on the main thread take a long time (e.g. because of expensive
painting), it's possible that the main thread has updated the scrolling layer position, and
then the scrolling thread detects that the commit is taking a long time and attempts to
trigger its own commit, but because the layer position property doesn't change, no commit
occurs.

Work around this by setting the layer position to 0,0 and back when we're on the scrolling
thread. Only do this if the scroll position changed since the last display refresh to avoid
triggering redundant commits.

Ideally we'd traverse the scrolling tree and do this for every scrolling node, but scrolling
trees can get large so for now just apply this to the root node.

• page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::updateScrollPositionAtLastDisplayRefresh):

• page/scrolling/ScrollingTreeScrollingNode.h:
• page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::displayDidRefreshOnScrollingThread):
(WebCore::ThreadedScrollingTree::storeScrollPositionsAtLastDisplayRefresh):

• page/scrolling/ThreadedScrollingTree.h:
• page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:

(WebCore::ScrollingTreeFrameScrollingNodeMac::repositionScrollingLayers):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ScrollingTreeScrollingNode.cpp (modified) (1 diff)
• page/scrolling/ScrollingTreeScrollingNode.h (modified) (2 diffs)
• page/scrolling/ThreadedScrollingTree.cpp (modified) (2 diffs)
• page/scrolling/ThreadedScrollingTree.h (modified) (1 diff)
• page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-12-12  Simon Fraser  <simon.fraser@apple.com>
+
+        Ensure that the scrolling thread always commits layer position changes to reduce scrolling stutters
+        https://bugs.webkit.org/show_bug.cgi?id=234213
+
+        Reviewed by Tim Horton.
+
+        On a page where CA commits on the main thread take a long time (e.g. because of expensive
+        painting), it's possible that the main thread has updated the scrolling layer position, and
+        then the scrolling thread detects that the commit is taking a long time and attempts to
+        trigger its own commit, but because the layer position property doesn't change, no commit
+        occurs.
+
+        Work around this by setting the layer position to 0,0 and back when we're on the scrolling
+        thread. Only do this if the scroll position changed since the last display refresh to avoid
+        triggering redundant commits.
+
+        Ideally we'd traverse the scrolling tree and do this for every scrolling node, but scrolling
+        trees can get large so for now just apply this to the root node.
+
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::updateScrollPositionAtLastDisplayRefresh):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+        * page/scrolling/ThreadedScrollingTree.cpp:
+        (WebCore::ThreadedScrollingTree::displayDidRefreshOnScrollingThread):
+        (WebCore::ThreadedScrollingTree::storeScrollPositionsAtLastDisplayRefresh):
+        * page/scrolling/ThreadedScrollingTree.h:
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::repositionScrollingLayers):
+
 2021-12-12  Alan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

@@ -304 +304 @@
 }
 
+void ScrollingTreeScrollingNode::updateScrollPositionAtLastDisplayRefresh()
+{
+    m_scrollPositionAtLastDisplayRefresh = m_currentScrollPosition;
+}
+
 bool ScrollingTreeScrollingNode::scrollPositionAndLayoutViewportMatch(const FloatPoint& position, std::optional<FloatRect>)
 {

trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h

@@ -142 +142 @@
     virtual void repositionScrollingLayers() { }
     virtual void repositionRelatedLayers() { }
+    
+    void updateScrollPositionAtLastDisplayRefresh();
+    std::optional<FloatPoint> scrollPositionAtLastDisplayRefresh() { return m_scrollPositionAtLastDisplayRefresh; };
 
     void applyLayerPositions() override;
@@ -175 +178 @@
     std::optional<unsigned> m_currentVerticalSnapPointIndex;
     ScrollableAreaParameters m_scrollableAreaParameters;
+    std::optional<FloatPoint> m_scrollPositionAtLastDisplayRefresh;
 #if ENABLE(SCROLLING_THREAD)
     OptionSet<SynchronousScrollingReason> m_synchronousScrollingReasons;

trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp

@@ -501 +501 @@
         break;
     }
+    
+    storeScrollPositionsAtLastDisplayRefresh();
 }
 
@@ -526 +528 @@
 }
 
+void ThreadedScrollingTree::storeScrollPositionsAtLastDisplayRefresh()
+{
+    // Ideally this would be a tree walk for every scrolling node, but scrolling trees can get big so for now just do this for the root;
+    // ScrollingTreeFrameScrollingNodeMac::repositionScrollingLayers() uses the state to know whether it should force a commit.
+    if (auto* rootNode = this->rootNode())
+        rootNode->updateScrollPositionAtLastDisplayRefresh();
+}
+
 } // namespace WebCore
 

trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h

@@ -108 +108 @@
     void hasNodeWithAnimatedScrollChanged(bool) final;
     
+    void storeScrollPositionsAtLastDisplayRefresh() WTF_REQUIRES_LOCK(m_treeLock);
+    
     void serviceScrollAnimations(MonotonicTime) WTF_REQUIRES_LOCK(m_treeLock);
 

trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm

@@ -36 +36 @@
 #import "ScrollingCoordinator.h"
 #import "ScrollingStateTree.h"
+#import "ScrollingThread.h"
 #import "ScrollingTree.h"
 #import "TileController.h"
@@ -175 +176 @@
 {
     BEGIN_BLOCK_OBJC_EXCEPTIONS
+
+    auto* layer = static_cast<CALayer*>(scrolledContentsLayer());
+    if (ScrollingThread::isCurrentThread()) {
+        // If we're committing on the scrolling thread, it means that ThreadedScrollingTree is in "desynchronized" mode.
+        // The main thread may already have set the same layer position, but here we need to trigger a scrolling thread commit to
+        // ensure that the scroll happens even when the main thread commit is taking a long time. So make sure the layer property changes
+        // when there has been a scroll position change.
+        if (scrollPositionAtLastDisplayRefresh() && scrollPositionAtLastDisplayRefresh().value() != currentScrollPosition())
+            layer.position = CGPointZero;
+    }
+
     // We use scroll position here because the root content layer is offset to account for scrollOrigin (see FrameView::positionForRootContentLayer).
-    static_cast<CALayer*>(scrolledContentsLayer()).position = -currentScrollPosition();
+    layer.position = -currentScrollPosition();
     END_BLOCK_OBJC_EXCEPTIONS
 }