Changeset 266710 in webkit

Timestamp:

Sep 7, 2020 1:55:31 PM (4 years ago)

Author:

commit-queue@webkit.org

Message:

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

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/platform/graphics/DisplayRefreshMonitor.cpp (modified) (1 diff)
• WebCore/platform/graphics/DisplayRefreshMonitor.h (modified) (2 diffs)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/UIProcess/mac/DisplayLink.cpp (modified) (4 diffs)
• WebKit/UIProcess/mac/DisplayLink.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp

@@ -95 +95 @@
     {
         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;
     }

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h

@@ -61 +61 @@
     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);
@@ -91 +95 @@
     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 };

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/UIProcess/mac/DisplayLink.cpp

@@ -33 +33 @@
 
 namespace WebKit {
-
-constexpr unsigned maxFireCountWithObservers { 20 };
     
 DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID)
@@ -75 +73 @@
 {
     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)
@@ -92 +93 @@
     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);
+        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.
+    if (m_observers.isEmpty())
+        CVDisplayLinkStop(m_displayLink);
 }
 
@@ -111 +113 @@
     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);

trunk/Source/WebKit/UIProcess/mac/DisplayLink.h

@@ -49 +49 @@
     void removeObserver(IPC::Connection&, DisplayLinkObserverID);
     void removeObservers(IPC::Connection&);
+    bool hasObservers() const;
 
     WebCore::PlatformDisplayID displayID() const { return m_displayID; }
@@ -61 +62 @@
     HashMap<RefPtr<IPC::Connection>, Vector<DisplayLinkObserverID>> m_observers;
     WebCore::PlatformDisplayID m_displayID;
-    unsigned m_fireCountWithoutObservers { 0 };
 };