Unreviewed, reverting r266645.

https://bugs.webkit.org/show_bug.cgi?id=216251 Caused MotionMark regression Reverted changeset: "Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess" https://bugs.webkit.org/show_bug.cgi?id=216195 https://trac.webkit.org/changeset/266645 Canonical link: https://commits.webkit.org/229079@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266710 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Sep 7, 2020 · 5e7133c0d09eb82150f8e0d0e18b654d2e3485f6 · 5e7133c
1 parent 01dd9e4
commit 5e7133c0d09eb82150f8e0d0e18b654d2e3485f6
Showing 6 changed files with 74 additions and 36 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,17 @@
+2020-09-07  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, reverting r266645.
+        https://bugs.webkit.org/show_bug.cgi?id=216251
+
+        Caused MotionMark regression
+
+        Reverted changeset:
+
+        "Move lazy DisplayLink tear down logic from the WebProcess to
+        the UIProcess"
+        https://bugs.webkit.org/show_bug.cgi?id=216195
+        https://trac.webkit.org/changeset/266645
+
 2020-09-07  Sam Weinig  <weinig@apple.com>
 
         [WebIDL] Fix issues found by preprocess-idls.pl parser validation and enabled parser validation by default for the tests

diff --git a/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp b/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp
@@ -94,7 +94,12 @@ void DisplayRefreshMonitor::displayDidRefresh()
 {
     {
         LockHolder lock(m_mutex);
-        LOG(RequestAnimationFrame, "DisplayRefreshMonitor::displayDidRefresh(%p) - m_scheduled(%d)", this, m_scheduled);
+        LOG(RequestAnimationFrame, "DisplayRefreshMonitor::displayDidRefresh(%p) - m_scheduled(%d), m_unscheduledFireCount(%d)", this, m_scheduled, m_unscheduledFireCount);
+        if (!m_scheduled)
+            ++m_unscheduledFireCount;
+        else
+            m_unscheduledFireCount = 0;
+
         m_scheduled = false;
     }
 

diff --git a/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h b/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
@@ -60,7 +60,11 @@ class DisplayRefreshMonitor : public ThreadSafeRefCounted<DisplayRefreshMonitor>
 
     PlatformDisplayID displayID() const { return m_displayID; }
 
-    bool shouldBeTerminated() const { return !m_scheduled; }
+    bool shouldBeTerminated() const
+    {
+        const int maxInactiveFireCount = 20;
+        return !m_scheduled && m_unscheduledFireCount > maxInactiveFireCount;
+    }
 
     static RefPtr<DisplayRefreshMonitor> createDefaultDisplayRefreshMonitor(PlatformDisplayID);
 
@@ -90,6 +94,7 @@ class DisplayRefreshMonitor : public ThreadSafeRefCounted<DisplayRefreshMonitor>
     HashSet<DisplayRefreshMonitorClient*>* m_clientsToBeNotified { nullptr };
     Lock m_mutex;
     PlatformDisplayID m_displayID { 0 };
+    int m_unscheduledFireCount { 0 }; // Number of times the display link has fired with no clients.
     bool m_active { true };
     bool m_scheduled { false };
     bool m_previousFrameDone { true };

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,17 @@
+2020-09-07  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, reverting r266645.
+        https://bugs.webkit.org/show_bug.cgi?id=216251
+
+        Caused MotionMark regression
+
+        Reverted changeset:
+
+        "Move lazy DisplayLink tear down logic from the WebProcess to
+        the UIProcess"
+        https://bugs.webkit.org/show_bug.cgi?id=216195
+        https://trac.webkit.org/changeset/266645
+
 2020-09-07  Mike Gorse  <mgorse@suse.com>
 
         Build failure; cannot find seccomp.h

diff --git a/Source/WebKit/UIProcess/mac/DisplayLink.cpp b/Source/WebKit/UIProcess/mac/DisplayLink.cpp
@@ -32,8 +32,6 @@
 #include <wtf/ProcessPrivilege.h>
 
 namespace WebKit {
-
-constexpr unsigned maxFireCountWithObservers { 20 };
 
 DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID)
     : m_displayID(displayID)
@@ -74,13 +72,16 @@ Optional<unsigned> DisplayLink::nominalFramesPerSecond() const
 void DisplayLink::addObserver(IPC::Connection& connection, DisplayLinkObserverID observerID)
 {
     ASSERT(RunLoop::isMain());
+    bool isRunning = !m_observers.isEmpty();
 
-    LockHolder locker(m_observersLock);
-    m_observers.ensure(&connection, [] {
-        return Vector<DisplayLinkObserverID> { };
-    }).iterator->value.append(observerID);
+    {
+        LockHolder locker(m_observersLock);
+        m_observers.ensure(&connection, [] {
+            return Vector<DisplayLinkObserverID> { };
+        }).iterator->value.append(observerID);
+    }
 
-    if (!CVDisplayLinkIsRunning(m_displayLink)) {
+    if (!isRunning) {
         CVReturn error = CVDisplayLinkStart(m_displayLink);
         if (error)
             WTFLogAlways("Could not start the display link: %d", error);
@@ -91,46 +92,45 @@ void DisplayLink::removeObserver(IPC::Connection& connection, DisplayLinkObserve
 {
     ASSERT(RunLoop::isMain());
 
-    LockHolder locker(m_observersLock);
+    {
+        LockHolder locker(m_observersLock);
 
-    auto it = m_observers.find(&connection);
-    if (it == m_observers.end())
-        return;
-    bool removed = it->value.removeFirst(observerID);
-    ASSERT_UNUSED(removed, removed);
-    if (it->value.isEmpty())
-        m_observers.remove(it);
-
-    // We do not stop the display link right away when |m_observers| becomes empty. Instead, we
-    // let the display link fire up to |maxFireCountWithObservers| times without observers to avoid
-    // killing & restarting too many threads when observers gets removed & added in quick succession.
+        auto it = m_observers.find(&connection);
+        if (it == m_observers.end())
+            return;
+        bool removed = it->value.removeFirst(observerID);
+        ASSERT_UNUSED(removed, removed);
+        if (it->value.isEmpty())
+            m_observers.remove(it);
+    }
+
+    if (m_observers.isEmpty())
+        CVDisplayLinkStop(m_displayLink);
 }
 
 void DisplayLink::removeObservers(IPC::Connection& connection)
 {
     ASSERT(RunLoop::isMain());
 
-    LockHolder locker(m_observersLock);
-    m_observers.remove(&connection);
+    {
+        LockHolder locker(m_observersLock);
+        m_observers.remove(&connection);
+    }
 
-    // We do not stop the display link right away when |m_observers| becomes empty. Instead, we
-    // let the display link fire up to |maxFireCountWithObservers| times without observers to avoid
-    // killing & restarting too many threads when observers gets removed & added in quick succession.
+    if (m_observers.isEmpty())
+        CVDisplayLinkStop(m_displayLink);
+}
+
+bool DisplayLink::hasObservers() const
+{
+    ASSERT(RunLoop::isMain());
+    return !m_observers.isEmpty();
 }
 
 CVReturn DisplayLink::displayLinkCallback(CVDisplayLinkRef displayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data)
 {
-    ASSERT(!RunLoop::isMain());
     auto* displayLink = static_cast<DisplayLink*>(data);
-
     LockHolder locker(displayLink->m_observersLock);
-    if (displayLink->m_observers.isEmpty()) {
-        if (++(displayLink->m_fireCountWithoutObservers) >= maxFireCountWithObservers)
-            CVDisplayLinkStop(displayLink->m_displayLink);
-        return kCVReturnSuccess;
-    }
-    displayLink->m_fireCountWithoutObservers = 0;
-
     for (auto& connection : displayLink->m_observers.keys())
         connection->send(Messages::EventDispatcher::DisplayWasRefreshed(displayLink->m_displayID), 0);
     return kCVReturnSuccess;

diff --git a/Source/WebKit/UIProcess/mac/DisplayLink.h b/Source/WebKit/UIProcess/mac/DisplayLink.h
@@ -48,6 +48,7 @@ class DisplayLink {
     void addObserver(IPC::Connection&, DisplayLinkObserverID);
     void removeObserver(IPC::Connection&, DisplayLinkObserverID);
     void removeObservers(IPC::Connection&);
+    bool hasObservers() const;
 
     WebCore::PlatformDisplayID displayID() const { return m_displayID; }
 
@@ -60,7 +61,6 @@ class DisplayLink {
     Lock m_observersLock;
     HashMap<RefPtr<IPC::Connection>, Vector<DisplayLinkObserverID>> m_observers;
     WebCore::PlatformDisplayID m_displayID;
-    unsigned m_fireCountWithoutObservers { 0 };
 };
 
 } // namespace WebKit