[macOS] Scrolling synchronization part 1: Have the scrolling thread wait half a frame...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 May 2020 06:47:24 +0000 (06:47 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 May 2020 06:47:24 +0000 (06:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212168

Reviewed by Tim Horton.

Source/WebCore:

Currently the scrolling thread is a free-running thread that moves layers around in response
to wheel events, and asynchronously posts data about scrolled layers back to the main thread.
That results in an almost guaranteed lack of synchronization between the displayed layer
positions, and the web-exposed values for scroll position (element.scrollTop, window.pageYOffset etc).
This is a frequent source of stuttering or jumpy web content when scrolling.

The first step to fixing this is to synchronize the scrolling thread layer positions
and the main thread state for the case where the main thread is responsive enough to
render once per frame. This is achieved as follow:
    - When the main thread is starting a rendering update, Page::updateRendering() informs
      the scrolling tree via ScrollingCoordinatorMac::willStartRenderingUpdate(). This
      atomically waits for the scrolling thread to take the m_treeMutex (via a BinarySemaphore)
      and starts waiting on the m_stateCondition Condition. Now the main thread pulls the
      state of the scrolling tree via synchronizeStateFromScrollingTree() and uses it for
      the rendering update.
    - If the rendering update finishes within half a frame (8ms), then m_stateCondition
      is released, and the scrolling thread assumes that the main thread is going to
      commit layers rapidly enough to preserve 60fps scrolling.
    - If the rendering update takes too long, m_stateCondition times out, and the scrolling
      thread applies layer positions, triggering a CA commit on that thread.

We no longer apply layer positions directly when handling wheel events.

synchronizeStateFromScrollingTree() has to only pull state from nodes that have moved on the scrolling thread,
so track that via ScrollingTreeScrollingNode::scrolledSinceLastCommit() and adjust the visitor function to
make it available during scrolling tree traversal.

* page/Page.cpp:
(WebCore::Page::updateRendering):
(WebCore::Page::finalizeRenderingUpdate):
* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
* page/scrolling/AsyncScrollingCoordinator.h:
* page/scrolling/ScrollingCoordinator.h:
(WebCore::ScrollingCoordinator::willStartRenderingUpdate):
(WebCore::ScrollingCoordinator::didCompleteRenderingUpdate):
(WebCore::ScrollingCoordinator::synchronizeStateFromScrollingTree): Deleted.
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::handleWheelEvent):
(WebCore::ScrollingTree::traverseScrollingTreeRecursive):
(WebCore::ScrollingTree::commitTreeState):
(WebCore::ScrollingTree::updateTreeFromStateNodeRecursive):
(WebCore::ScrollingTree::applyLayerPositionsInternal):
(WebCore::ScrollingTree::nominalFramesPerSecond):
* page/scrolling/ScrollingTree.h:
* page/scrolling/ScrollingTreeNode.h:
(WebCore::ScrollingTreeNode::didCompleteCommitForNode):
* page/scrolling/ScrollingTreeScrollingNode.cpp:
(WebCore::ScrollingTreeScrollingNode::didCompleteCommitForNode):
(WebCore::ScrollingTreeScrollingNode::currentScrollPositionChanged):
* page/scrolling/ScrollingTreeScrollingNode.h:
* page/scrolling/ThreadedScrollingTree.cpp:
(WebCore::ThreadedScrollingTree::willStartRenderingUpdate):
(WebCore::ThreadedScrollingTree::maxAllowableRenderingUpdateDurationForSynchronization):
(WebCore::ThreadedScrollingTree::waitForRenderingUpdateCompletionOrTimeout):
(WebCore::ThreadedScrollingTree::didCompleteRenderingUpdate):
(WebCore::ThreadedScrollingTree::displayDidRefreshOnScrollingThread):
* page/scrolling/ThreadedScrollingTree.h:
(WebCore::ThreadedScrollingTree::treeMutex):
* page/scrolling/mac/ScrollingCoordinatorMac.h:
* page/scrolling/mac/ScrollingCoordinatorMac.mm:
(WebCore::ScrollingCoordinatorMac::willStartRenderingUpdate):
(WebCore::ScrollingCoordinatorMac::didCompleteRenderingUpdate):

Source/WTF:

Some new trace points for scrolling thread activity.

* wtf/SystemTracing.h:

Tools:

Some new trace points for scrolling thread activity.

* Tracing/SystemTracePoints.plist:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@261985 268f45cc-cd09-0410-ab3c-d52691b4dbfc

18 files changed:
Source/WTF/ChangeLog
Source/WTF/wtf/SystemTracing.h
Source/WebCore/ChangeLog
Source/WebCore/page/Page.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingTree.cpp
Source/WebCore/page/scrolling/ScrollingTree.h
Source/WebCore/page/scrolling/ScrollingTreeNode.h
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
Source/WebCore/page/scrolling/ThreadedScrollingTree.h
Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h
Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm
Tools/ChangeLog
Tools/Tracing/SystemTracePoints.plist

index 6e74804..247a634 100644 (file)
@@ -1,3 +1,14 @@
+2020-05-20  Simon Fraser  <simon.fraser@apple.com>
+
+        [macOS] Scrolling synchronization part 1: Have the scrolling thread wait half a frame for the main thread to complete the rendering update
+        https://bugs.webkit.org/show_bug.cgi?id=212168
+
+        Reviewed by Tim Horton.
+
+        Some new trace points for scrolling thread activity.
+
+        * wtf/SystemTracing.h:
+
 2020-05-20  Tim Horton  <timothy_horton@apple.com>
 
         WKMouseGestureRecognizer should be implemented without using UIKit internals
index a9ade8a..1197730 100644 (file)
@@ -90,6 +90,10 @@ enum TracePointCode {
     ParseHTMLEnd,
     DisplayListReplayStart,
     DisplayListReplayEnd,
+    ScrollingThreadRenderUpdateSyncStart,
+    ScrollingThreadRenderUpdateSyncEnd,
+    ScrollingThreadDisplayDidRefreshStart,
+    ScrollingThreadDisplayDidRefreshEnd,
 
     WebKitRange = 10000,
     WebHTMLViewPaintStart,
index ed17acd..1b73d3b 100644 (file)
@@ -1,3 +1,74 @@
+2020-05-20  Simon Fraser  <simon.fraser@apple.com>
+
+        [macOS] Scrolling synchronization part 1: Have the scrolling thread wait half a frame for the main thread to complete the rendering update
+        https://bugs.webkit.org/show_bug.cgi?id=212168
+
+        Reviewed by Tim Horton.
+
+        Currently the scrolling thread is a free-running thread that moves layers around in response
+        to wheel events, and asynchronously posts data about scrolled layers back to the main thread.
+        That results in an almost guaranteed lack of synchronization between the displayed layer
+        positions, and the web-exposed values for scroll position (element.scrollTop, window.pageYOffset etc).
+        This is a frequent source of stuttering or jumpy web content when scrolling.
+
+        The first step to fixing this is to synchronize the scrolling thread layer positions
+        and the main thread state for the case where the main thread is responsive enough to
+        render once per frame. This is achieved as follow:
+            - When the main thread is starting a rendering update, Page::updateRendering() informs
+              the scrolling tree via ScrollingCoordinatorMac::willStartRenderingUpdate(). This
+              atomically waits for the scrolling thread to take the m_treeMutex (via a BinarySemaphore)
+              and starts waiting on the m_stateCondition Condition. Now the main thread pulls the
+              state of the scrolling tree via synchronizeStateFromScrollingTree() and uses it for
+              the rendering update.
+            - If the rendering update finishes within half a frame (8ms), then m_stateCondition
+              is released, and the scrolling thread assumes that the main thread is going to
+              commit layers rapidly enough to preserve 60fps scrolling.
+            - If the rendering update takes too long, m_stateCondition times out, and the scrolling
+              thread applies layer positions, triggering a CA commit on that thread.
+
+        We no longer apply layer positions directly when handling wheel events.
+
+        synchronizeStateFromScrollingTree() has to only pull state from nodes that have moved on the scrolling thread,
+        so track that via ScrollingTreeScrollingNode::scrolledSinceLastCommit() and adjust the visitor function to
+        make it available during scrolling tree traversal.
+
+        * page/Page.cpp:
+        (WebCore::Page::updateRendering):
+        (WebCore::Page::finalizeRenderingUpdate):
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
+        * page/scrolling/AsyncScrollingCoordinator.h:
+        * page/scrolling/ScrollingCoordinator.h:
+        (WebCore::ScrollingCoordinator::willStartRenderingUpdate):
+        (WebCore::ScrollingCoordinator::didCompleteRenderingUpdate):
+        (WebCore::ScrollingCoordinator::synchronizeStateFromScrollingTree): Deleted.
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::handleWheelEvent):
+        (WebCore::ScrollingTree::traverseScrollingTreeRecursive):
+        (WebCore::ScrollingTree::commitTreeState):
+        (WebCore::ScrollingTree::updateTreeFromStateNodeRecursive):
+        (WebCore::ScrollingTree::applyLayerPositionsInternal):
+        (WebCore::ScrollingTree::nominalFramesPerSecond):
+        * page/scrolling/ScrollingTree.h:
+        * page/scrolling/ScrollingTreeNode.h:
+        (WebCore::ScrollingTreeNode::didCompleteCommitForNode):
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::didCompleteCommitForNode):
+        (WebCore::ScrollingTreeScrollingNode::currentScrollPositionChanged):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+        * page/scrolling/ThreadedScrollingTree.cpp:
+        (WebCore::ThreadedScrollingTree::willStartRenderingUpdate):
+        (WebCore::ThreadedScrollingTree::maxAllowableRenderingUpdateDurationForSynchronization):
+        (WebCore::ThreadedScrollingTree::waitForRenderingUpdateCompletionOrTimeout):
+        (WebCore::ThreadedScrollingTree::didCompleteRenderingUpdate):
+        (WebCore::ThreadedScrollingTree::displayDidRefreshOnScrollingThread):
+        * page/scrolling/ThreadedScrollingTree.h:
+        (WebCore::ThreadedScrollingTree::treeMutex):
+        * page/scrolling/mac/ScrollingCoordinatorMac.h:
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::willStartRenderingUpdate):
+        (WebCore::ScrollingCoordinatorMac::didCompleteRenderingUpdate):
+
 2020-05-20  Chris Fleizach  <cfleizach@apple.com>
 
         REGRESSION (iOS 13.4.1): SpeechSynthesisUtterance.onend event won't fire on cancel().
index 14ac38c..2df9d2b 100644 (file)
@@ -1360,6 +1360,11 @@ void Page::updateRendering()
 
     layoutIfNeeded();
 
+#if ENABLE(ASYNC_SCROLLING)
+    if (auto* scrollingCoordinator = this->scrollingCoordinator())
+        scrollingCoordinator->willStartRenderingUpdate();
+#endif
+
     // Timestamps should not change while serving the rendering update steps.
     Vector<WeakPtr<Document>> initialDocuments;
     forEachDocument([&initialDocuments] (Document& document) {
@@ -1478,6 +1483,8 @@ void Page::finalizeRenderingUpdate(OptionSet<FinalizeRenderingUpdateFlags> flags
         scrollingCoordinator->commitTreeStateIfNeeded();
         if (flags.contains(FinalizeRenderingUpdateFlags::ApplyScrollingTreeLayerPositions))
             scrollingCoordinator->applyScrollingTreeLayerPositions();
+            
+        scrollingCoordinator->didCompleteRenderingUpdate();
     }
 #endif
 }
index eae41b0..008790c 100644 (file)
@@ -296,8 +296,8 @@ void AsyncScrollingCoordinator::synchronizeStateFromScrollingTree()
 {
     ASSERT(isMainThread());
 
-    m_scrollingTree->traverseScrollingTree([&](ScrollingNodeID nodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin) {
-        if (scrollPosition) {
+    m_scrollingTree->traverseScrollingTree([&](ScrollingNodeID nodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin, bool scrolledSinceLastCommit) {
+        if (scrollPosition && scrolledSinceLastCommit) {
             LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::synchronizeStateFromScrollingTree - node " << nodeID << " scroll position " << scrollPosition);
             updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition.value(), layoutViewportOrigin, ScrollType::User, ScrollingLayerPositionAction::Set);
         }
index 2208654..8a72041 100644 (file)
@@ -82,6 +82,7 @@ protected:
     WEBCORE_EXPORT String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const override;
     WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const override;
     WEBCORE_EXPORT void willCommitTree() override;
+    void synchronizeStateFromScrollingTree();
 
     bool eventTrackingRegionsDirty() const { return m_eventTrackingRegionsDirty; }
 
@@ -102,7 +103,6 @@ private:
     WEBCORE_EXPORT bool requestScrollPositionUpdate(ScrollableArea&, const IntPoint&, ScrollType, ScrollClamping) override;
 
     WEBCORE_EXPORT void applyScrollingTreeLayerPositions() override;
-    WEBCORE_EXPORT void synchronizeStateFromScrollingTree() override;
 
     WEBCORE_EXPORT ScrollingNodeID createNode(ScrollingNodeType, ScrollingNodeID newNodeID) override;
     WEBCORE_EXPORT ScrollingNodeID insertNode(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex) override;
index e1f567c..005bcc5 100644 (file)
@@ -113,8 +113,8 @@ public:
     // Traverses the scrolling tree, setting layer positions to represent the current scrolled state.
     virtual void applyScrollingTreeLayerPositions() { }
 
-    // Takes scroll positions from the scrolling tree and applies them to ScrollableAreas.
-    virtual void synchronizeStateFromScrollingTree() { }
+    virtual void willStartRenderingUpdate() { }
+    virtual void didCompleteRenderingUpdate() { }
 
 #if ENABLE(KINETIC_SCROLLING)
     // Dispatched by the scrolling tree during handleWheelEvent. This is required as long as scrollbars are painted on the main thread.
index 1adaca2..e080426 100644 (file)
@@ -158,9 +158,6 @@ ScrollingEventResult ScrollingTree::handleWheelEvent(const PlatformWheelEvent& w
         return ScrollingEventResult::DidNotHandleEvent;
     }();
 
-    if (result == ScrollingEventResult::DidHandleEvent)
-        applyLayerPositionsInternal();
-    
     return result;
 }
 
@@ -187,15 +184,19 @@ void ScrollingTree::traverseScrollingTree(VisitorFunction&& visitorFunction)
 
 void ScrollingTree::traverseScrollingTreeRecursive(ScrollingTreeNode& node, const VisitorFunction& visitorFunction)
 {
+    bool scrolledSinceLastCommit = false;
     Optional<FloatPoint> scrollPosition;
-    if (is<ScrollingTreeScrollingNode>(node))
-        scrollPosition = downcast<ScrollingTreeScrollingNode>(node).currentScrollPosition();
+    if (is<ScrollingTreeScrollingNode>(node)) {
+        auto& scrollingNode = downcast<ScrollingTreeScrollingNode>(node);
+        scrollPosition = scrollingNode.currentScrollPosition();
+        scrolledSinceLastCommit = scrollingNode.scrolledSinceLastCommit();
+    }
 
     Optional<FloatPoint> layoutViewportOrigin;
     if (is<ScrollingTreeFrameScrollingNode>(node))
         layoutViewportOrigin = downcast<ScrollingTreeFrameScrollingNode>(node).layoutViewport().location();
 
-    visitorFunction(node.scrollingNodeID(), node.nodeType(), scrollPosition, layoutViewportOrigin);
+    visitorFunction(node.scrollingNodeID(), node.nodeType(), scrollPosition, layoutViewportOrigin, scrolledSinceLastCommit);
 
     for (auto& child : node.children())
         traverseScrollingTreeRecursive(child.get(), visitorFunction);
@@ -217,9 +218,9 @@ void ScrollingTree::commitTreeState(std::unique_ptr<ScrollingStateTree>&& scroll
     LockHolder locker(m_treeMutex);
 
     bool rootStateNodeChanged = scrollingStateTree->hasNewRootStateNode();
-    
+
     LOG(ScrollingTree, "\nScrollingTree %p commitTreeState", this);
-    
+
     auto* rootNode = scrollingStateTree->rootStateNode();
     if (rootNode
         && (rootStateNodeChanged
@@ -327,7 +328,8 @@ void ScrollingTree::updateTreeFromStateNodeRecursive(const ScrollingStateNode* s
     }
 
     node->commitStateAfterChildren(*stateNode);
-    
+    node->didCompleteCommitForNode();
+
 #if ENABLE(SCROLLING_THREAD)
     if (is<ScrollingTreeScrollingNode>(*node) && !downcast<ScrollingTreeScrollingNode>(*node).synchronousScrollingReasons().isEmpty())
         state.synchronousScrollingNodes.add(nodeID);
@@ -354,6 +356,7 @@ void ScrollingTree::applyLayerPositions()
 
 void ScrollingTree::applyLayerPositionsInternal()
 {
+    ASSERT(m_treeMutex.isLocked());
     if (!m_rootNode)
         return;
 
@@ -540,6 +543,12 @@ PlatformDisplayID ScrollingTree::displayID()
     return m_treeState.displayID;
 }
 
+Optional<unsigned> ScrollingTree::nominalFramesPerSecond()
+{
+    LockHolder locker(m_treeStateMutex);
+    return m_treeState.nominalFramesPerSecond;
+}
+
 void ScrollingTree::setScrollingPerformanceLoggingEnabled(bool flag)
 {
     m_scrollingPerformanceLoggingEnabled = flag;
index 3bd821e..aa1be13 100644 (file)
@@ -90,7 +90,7 @@ public:
     
     WEBCORE_EXPORT ScrollingTreeNode* nodeForID(ScrollingNodeID) const;
 
-    using VisitorFunction = WTF::Function<void (ScrollingNodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin)>;
+    using VisitorFunction = WTF::Function<void (ScrollingNodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin, bool scrolledSinceLastCommit)>;
     void traverseScrollingTree(VisitorFunction&&);
 
     // Called after a scrolling tree node has handled a scroll and updated its layers.
@@ -188,12 +188,15 @@ protected:
 
     WEBCORE_EXPORT virtual ScrollingEventResult handleWheelEvent(const PlatformWheelEvent&);
 
+    Optional<unsigned> nominalFramesPerSecond();
+
+    void applyLayerPositionsInternal();
+    Lock m_treeMutex; // Protects the scrolling tree.
+
 private:
     void updateTreeFromStateNodeRecursive(const ScrollingStateNode*, struct CommitTreeState&);
     virtual void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) { }
 
-    void applyLayerPositionsInternal();
-
     void applyLayerPositionsRecursive(ScrollingTreeNode&);
     void notifyRelatedNodesRecursive(ScrollingTreeNode&);
     void traverseScrollingTreeRecursive(ScrollingTreeNode&, const VisitorFunction&);
@@ -202,8 +205,6 @@ private:
     WEBCORE_EXPORT virtual OptionSet<EventListenerRegionType> eventListenerRegionTypesForPoint(FloatPoint) const;
     virtual void receivedWheelEvent(const PlatformWheelEvent&) { }
 
-    Lock m_treeMutex; // Protects the scrolling tree.
-
     RefPtr<ScrollingTreeFrameScrollingNode> m_rootNode;
 
     using ScrollingTreeNodeMap = HashMap<ScrollingNodeID, RefPtr<ScrollingTreeNode>>;
index 193d217..3335d06 100644 (file)
@@ -60,6 +60,7 @@ public:
 
     virtual void commitStateBeforeChildren(const ScrollingStateNode&) = 0;
     virtual void commitStateAfterChildren(const ScrollingStateNode&) { }
+    virtual void didCompleteCommitForNode() { }
 
     ScrollingTreeNode* parent() const { return m_parent; }
     void setParent(ScrollingTreeNode* parent) { m_parent = parent; }
index 80216eb..fa45c97 100644 (file)
@@ -124,6 +124,11 @@ void ScrollingTreeScrollingNode::commitStateAfterChildren(const ScrollingStateNo
     m_isFirstCommit = false;
 }
 
+void ScrollingTreeScrollingNode::didCompleteCommitForNode()
+{
+    m_scrolledSinceLastCommit = false;
+}
+
 bool ScrollingTreeScrollingNode::isLatchedNode() const
 {
     return scrollingTree().latchedNodeID() == scrollingNodeID();
@@ -250,6 +255,7 @@ void ScrollingTreeScrollingNode::scrollTo(const FloatPoint& position, ScrollType
 void ScrollingTreeScrollingNode::currentScrollPositionChanged()
 {
     scrollingTree().scrollingTreeNodeDidScroll(*this);
+    m_scrolledSinceLastCommit = true;
 }
 
 bool ScrollingTreeScrollingNode::scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect>)
index 8ad9962..1dc42b3 100644 (file)
@@ -52,6 +52,7 @@ public:
 
     void commitStateBeforeChildren(const ScrollingStateNode&) override;
     void commitStateAfterChildren(const ScrollingStateNode&) override;
+    void didCompleteCommitForNode() final;
 
     virtual bool canHandleWheelEvent(const PlatformWheelEvent&) const;
     virtual ScrollingEventResult handleWheelEvent(const PlatformWheelEvent&);
@@ -103,6 +104,8 @@ public:
     bool useDarkAppearanceForScrollbars() const { return m_scrollableAreaParameters.useDarkAppearanceForScrollbars; }
 
     bool eventCanScrollContents(const PlatformWheelEvent&) const;
+    
+    bool scrolledSinceLastCommit() const { return m_scrolledSinceLastCommit; }
 
     const LayerRepresentation& scrollContainerLayer() const { return m_scrollContainerLayer; }
     const LayerRepresentation& scrolledContentsLayer() const { return m_scrolledContentsLayer; }
@@ -162,6 +165,7 @@ private:
     OptionSet<SynchronousScrollingReason> m_synchronousScrollingReasons;
 #endif
     bool m_isFirstCommit { true };
+    bool m_scrolledSinceLastCommit { false };
 
     LayerRepresentation m_scrollContainerLayer;
     LayerRepresentation m_scrolledContentsLayer;
index 65c4c34..3da50ee 100644 (file)
@@ -38,7 +38,9 @@
 #include "ScrollingTreeScrollingNode.h"
 #include <wtf/RunLoop.h>
 #include <wtf/SetForScope.h>
+#include <wtf/SystemTracing.h>
 #include <wtf/text/TextStream.h>
+#include <wtf/threads/BinarySemaphore.h>
 
 namespace WebCore {
 
@@ -202,6 +204,94 @@ void ThreadedScrollingTree::scrollingTreeNodeRequestsScroll(ScrollingNodeID node
 }
 #endif
 
+void ThreadedScrollingTree::willStartRenderingUpdate()
+{
+    ASSERT(isMainThread());
+
+    tracePoint(ScrollingThreadRenderUpdateSyncStart);
+
+    // Wait for the scrolling thread to acquire m_treeMutex. This ensures that any pending wheel events are processed.
+    BinarySemaphore semaphore;
+    ScrollingThread::dispatch([protectedThis = makeRef(*this), &semaphore]() {
+        LockHolder treeLocker(protectedThis->m_treeMutex);
+        semaphore.signal();
+        protectedThis->waitForRenderingUpdateCompletionOrTimeout();
+    });
+    semaphore.wait();
+    m_state = SynchronizationState::InRenderingUpdate;
+}
+
+Seconds ThreadedScrollingTree::maxAllowableRenderingUpdateDurationForSynchronization()
+{
+    constexpr double allowableFrameFraction = 0.5;
+    auto displayFPS = nominalFramesPerSecond().valueOr(60);
+    Seconds frameDuration = 1_s / (double)displayFPS;
+    return allowableFrameFraction * frameDuration;
+}
+
+// This code allows the main thread about half a frame to complete its rendering udpate. If the main thread
+// is responsive (i.e. managing to render every frame), then we expect to get a didCompleteRenderingUpdate()
+// within 8ms of willStartRenderingUpdate(). We time this via m_stateCondition, which blocks the scrolling
+// thread (with m_treeMutex locked at the start and end) so that we don't handle wheel events while waiting.
+// If the condition times out, we know the main thread is being slow, and allow the scrolling thread to
+// commit layer positions.
+void ThreadedScrollingTree::waitForRenderingUpdateCompletionOrTimeout()
+{
+    ASSERT(ScrollingThread::isCurrentThread());
+    ASSERT(m_treeMutex.isLocked());
+
+    auto startTime = MonotonicTime::now();
+    auto timeoutTime = startTime + maxAllowableRenderingUpdateDurationForSynchronization();
+
+    bool becameIdle = m_stateCondition.waitUntil(m_treeMutex, timeoutTime, [&] {
+        return m_state == SynchronizationState::Idle;
+    });
+
+    ASSERT(m_treeMutex.isLocked());
+
+    if (!becameIdle) {
+        m_state = SynchronizationState::Desynchronized;
+        // At this point we know the main thread is taking too long in the rendering update,
+        // so we give up trying to sync with the main thread and update layers here on the scrolling thread.
+        applyLayerPositionsInternal();
+        tracePoint(ScrollingThreadRenderUpdateSyncEnd, 1);
+    } else
+        tracePoint(ScrollingThreadRenderUpdateSyncEnd);
+}
+
+void ThreadedScrollingTree::didCompleteRenderingUpdate()
+{
+    ASSERT(isMainThread());
+    LockHolder treeLocker(m_treeMutex);
+
+    if (m_state == SynchronizationState::InRenderingUpdate)
+        m_stateCondition.notifyOne();
+
+    m_state = SynchronizationState::Idle;
+}
+
+void ThreadedScrollingTree::displayDidRefreshOnScrollingThread()
+{
+    TraceScope tracingScope(ScrollingThreadDisplayDidRefreshStart, ScrollingThreadDisplayDidRefreshEnd);
+    ASSERT(ScrollingThread::isCurrentThread());
+
+    LockHolder treeLocker(m_treeMutex);
+
+    if (m_state != SynchronizationState::Idle)
+        applyLayerPositionsInternal();
+
+    switch (m_state) {
+    case SynchronizationState::Idle: {
+        m_state = SynchronizationState::WaitingForRenderingUpdate;
+        break;
+    }
+    case SynchronizationState::WaitingForRenderingUpdate:
+    case SynchronizationState::InRenderingUpdate:
+    case SynchronizationState::Desynchronized:
+        break;
+    }
+}
+
 void ThreadedScrollingTree::displayDidRefresh(PlatformDisplayID displayID)
 {
     if (displayID != this->displayID())
@@ -216,11 +306,6 @@ void ThreadedScrollingTree::displayDidRefresh(PlatformDisplayID displayID)
 #endif
 }
 
-void ThreadedScrollingTree::displayDidRefreshOnScrollingThread()
-{
-    ASSERT(ScrollingThread::isCurrentThread());
-}
-
 } // namespace WebCore
 
 #endif // ENABLE(ASYNC_SCROLLING) && ENABLE(SCROLLING_THREAD)
index 948bc7d..263db84 100644 (file)
@@ -57,6 +57,11 @@ public:
 
     WEBCORE_EXPORT void displayDidRefresh(PlatformDisplayID);
 
+    void willStartRenderingUpdate();
+    void didCompleteRenderingUpdate();
+
+    Lock& treeMutex() { return m_treeMutex; }
+
 protected:
     explicit ThreadedScrollingTree(AsyncScrollingCoordinator&);
 
@@ -79,8 +84,21 @@ private:
     void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) override;
 
     void displayDidRefreshOnScrollingThread();
+    void waitForRenderingUpdateCompletionOrTimeout();
+    
+    Seconds maxAllowableRenderingUpdateDurationForSynchronization();
 
     RefPtr<AsyncScrollingCoordinator> m_scrollingCoordinator;
+
+    enum class SynchronizationState : uint8_t {
+        Idle,
+        WaitingForRenderingUpdate,
+        InRenderingUpdate,
+        Desynchronized,
+    };
+
+    SynchronizationState m_state { SynchronizationState::Idle };
+    Condition m_stateCondition;
 };
 
 } // namespace WebCore
index 93ea082..f2156af 100644 (file)
@@ -46,6 +46,9 @@ public:
 private:
     void scheduleTreeStateCommit() final;
 
+    void willStartRenderingUpdate() final;
+    void didCompleteRenderingUpdate() final;
+
     void updateTiledScrollingIndicator();
 
     void startMonitoringWheelEvents(bool clearLatchingState) final;
index fc2491f..bb0f2d9 100644 (file)
@@ -110,6 +110,18 @@ void ScrollingCoordinatorMac::commitTreeStateIfNeeded()
     updateTiledScrollingIndicator();
 }
 
+void ScrollingCoordinatorMac::willStartRenderingUpdate()
+{
+    RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
+    threadedScrollingTree->willStartRenderingUpdate();
+    synchronizeStateFromScrollingTree();
+}
+
+void ScrollingCoordinatorMac::didCompleteRenderingUpdate()
+{
+    downcast<ThreadedScrollingTree>(scrollingTree())->didCompleteRenderingUpdate();
+}
+
 void ScrollingCoordinatorMac::updateTiledScrollingIndicator()
 {
     FrameView* frameView = m_page->mainFrame().view();
index c344f5b..3fcb2cc 100644 (file)
@@ -1,3 +1,14 @@
+2020-05-20  Simon Fraser  <simon.fraser@apple.com>
+
+        [macOS] Scrolling synchronization part 1: Have the scrolling thread wait half a frame for the main thread to complete the rendering update
+        https://bugs.webkit.org/show_bug.cgi?id=212168
+
+        Reviewed by Tim Horton.
+
+        Some new trace points for scrolling thread activity.
+
+        * Tracing/SystemTracePoints.plist:
+
 2020-05-20  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [iOS] Layout tests in editing/pasteboard sporadically crash
index ee4c01e..6afcb5a 100644 (file)
              </dict>
              <dict>
                  <key>Name</key>
+                 <string>Scrolling Sync</string>
+                 <key>Type</key>
+                 <string>Interval</string>
+                 <key>Component</key>
+                 <string>47</string>
+                 <key>CodeBegin</key>
+                 <string>5040</string>
+                 <key>CodeEnd</key>
+                 <string>5041</string>
+             </dict>
+             <dict>
+                 <key>Name</key>
+                 <string>Scrolling Thread DisplayDidRefresh</string>
+                 <key>Type</key>
+                 <string>Interval</string>
+                 <key>Component</key>
+                 <string>47</string>
+                 <key>CodeBegin</key>
+                 <string>5042</string>
+                 <key>CodeEnd</key>
+                 <string>5043</string>
+             </dict>
+             <dict>
+                 <key>Name</key>
                  <string>Paint WebHTMLView</string>
                  <key>Type</key>
                  <string>Interval</string>