Attachment 361559 Details for Bug 194463 – Patch

[patch] Patch

bug-194463-20190208163543.patch (text/plain), 15.00 KB, created by Chris Dumez on 2019-02-08 16:35:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-02-08 16:35:43 PST

Size: 15.00 KB

patch

obsolete

>Subversion Revision: 241222
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6c91ad44a356cda83f144c6abe46dea086af2baf..3d967802c9bbf6522e93ab20ad79b7c93977d5b4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,41 @@
>+2019-02-08  Chris Dumez  <cdumez@apple.com>
>+
>+        [WK2][macOS] Avoid creating new CVDisplayLink objects for each WebProcess
>+        https://bugs.webkit.org/show_bug.cgi?id=194463
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Avoid creating new CVDisplayLink objects for each WebProcess. We really only need one per
>+        display, creating such object is expensive and it is even worse in a PSON world where we
>+        swap process on navigation.
>+
>+        This patch moves the DisplayLink storing from WebProcessProxy to WebProcessPool. Also,
>+        a DisplayLink can now be associated to several IPC connections instead of having a 1:1
>+        mapping. When a DisplayLink no longer has any observers, we now merely stop it instead
>+        of destroying it.
>+
>+        * UIProcess/Cocoa/WebProcessPoolCocoa.mm:
>+        (WebKit::WebProcessPool::startDisplayLink):
>+        (WebKit::WebProcessPool::stopDisplayLink):
>+        (WebKit::WebProcessPool::stopDisplayLinks):
>+        * UIProcess/WebProcessPool.h:
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::~WebProcessProxy):
>+        (WebKit::WebProcessProxy::processWillShutDown):
>+        (WebKit::WebProcessProxy::shutDown):
>+        * UIProcess/WebProcessProxy.h:
>+        * UIProcess/mac/DisplayLink.cpp:
>+        (WebKit::DisplayLink::DisplayLink):
>+        (WebKit::DisplayLink::addObserver):
>+        (WebKit::DisplayLink::removeObserver):
>+        (WebKit::DisplayLink::removeObservers):
>+        (WebKit::DisplayLink::hasObservers const):
>+        (WebKit::DisplayLink::displayLinkCallback):
>+        * UIProcess/mac/DisplayLink.h:
>+        * UIProcess/mac/WebProcessProxyMac.mm:
>+        (WebKit::WebProcessProxy::startDisplayLink):
>+        (WebKit::WebProcessProxy::stopDisplayLink):
>+
> 2019-02-08  Truitt Savell  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r241197.
>diff --git a/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm b/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm
>index 2daf75571ccb1a9d705baa057ad624a5a698a439..821bdce263a0eaa184f061f0eaee372e6e51e44a 100644
>--- a/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm
>+++ b/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2010-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2010-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -463,6 +463,37 @@ void WebProcessPool::resetHSTSHostsAddedAfterDate(double startDateIntervalSince1
>     _CFNetworkResetHSTSHostsSinceDate(privateBrowsingSession(), (__bridge CFDateRef)startDate);
> }
> 
>+#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>+void WebProcessPool::startDisplayLink(IPC::Connection& connection, unsigned observerID, uint32_t displayID)
>+{
>+    for (auto& displayLink : m_displayLinks) {
>+        if (displayLink->displayID() == displayID) {
>+            displayLink->addObserver(connection, observerID);
>+            return;
>+        }
>+    }
>+    auto displayLink = std::make_unique<DisplayLink>(displayID);
>+    displayLink->addObserver(connection, observerID);
>+    m_displayLinks.append(WTFMove(displayLink));
>+}
>+
>+void WebProcessPool::stopDisplayLink(IPC::Connection& connection, unsigned observerID, uint32_t displayID)
>+{
>+    for (auto& displayLink : m_displayLinks) {
>+        if (displayLink->displayID() == displayID) {
>+            displayLink->removeObserver(connection, observerID);
>+            return;
>+        }
>+    }
>+}
>+
>+void WebProcessPool::stopDisplayLinks(IPC::Connection& connection)
>+{
>+    for (auto& displayLink : m_displayLinks)
>+        displayLink->removeObservers(connection);
>+}
>+#endif
>+
> // FIXME: Deprecated. Left here until a final decision is made.
> void WebProcessPool::setCookieStoragePartitioningEnabled(bool enabled)
> {
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.h b/Source/WebKit/UIProcess/WebProcessPool.h
>index 3aecd424d8b27fea3e75eef1064d80a6447f2a23..61dc85351ea073eac04378352b3a8b2b75f320d8 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.h
>+++ b/Source/WebKit/UIProcess/WebProcessPool.h
>@@ -71,6 +71,10 @@ OBJC_CLASS NSObject;
> OBJC_CLASS NSString;
> #endif
> 
>+#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>+#include "DisplayLink.h"
>+#endif
>+
> namespace API {
> class AutomationClient;
> class CustomProtocolManagerClient;
>@@ -208,6 +212,12 @@ public:
>     const HashMap<String, HashMap<String, HashMap<String, uint8_t>>>& pluginLoadClientPolicies() const { return m_pluginLoadClientPolicies; }
> #endif
> 
>+#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>+    void startDisplayLink(IPC::Connection&, unsigned observerID, uint32_t displayID);
>+    void stopDisplayLink(IPC::Connection&, unsigned observerID, uint32_t displayID);
>+    void stopDisplayLinks(IPC::Connection&);
>+#endif
>+
>     void addSupportedPlugin(String&& matchingDomain, String&& name, HashSet<String>&& mimeTypes, HashSet<String> extensions);
>     void clearSupportedPlugins();
> 
>@@ -728,6 +738,10 @@ private:
> 
>     HashMap<String, std::unique_ptr<WebCore::PrewarmInformation>> m_prewarmInformationPerRegistrableDomain;
> 
>+#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>+    Vector<std::unique_ptr<DisplayLink>> m_displayLinks;
>+#endif
>+
> #if PLATFORM(GTK) || PLATFORM(WPE)
>     bool m_sandboxEnabled { false };
>     HashMap<CString, SandboxPermission> m_extraSandboxPaths;
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index ace34d5b31166dc22061df9ea5c5c0ea74a3c834..d4d6202858bbab134d8d8a241a542acc8580d043 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -162,6 +162,9 @@ WebProcessProxy::~WebProcessProxy()
> 
>     WebPasteboardProxy::singleton().removeWebProcessProxy(*this);
> 
>+    if (state() == State::Running)
>+        processPool().stopDisplayLinks(*connection());
>+
>     if (m_webConnection)
>         m_webConnection->invalidate();
> 
>@@ -240,6 +243,8 @@ void WebProcessProxy::processWillShutDown(IPC::Connection& connection)
> {
>     ASSERT_UNUSED(connection, this->connection() == &connection);
> 
>+    processPool().stopDisplayLinks(connection);
>+
>     for (auto& page : m_pageMap.values())
>         page->webProcessWillShutDown();
> }
>@@ -248,6 +253,8 @@ void WebProcessProxy::shutDown()
> {
>     RELEASE_ASSERT(isMainThreadOrCheckDisabled());
> 
>+    processPool().stopDisplayLinks(*connection());
>+
>     shutDownProcess();
> 
>     if (m_webConnection) {
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index b1039ecb8af6cce514ef5a4c514015f8e55bf4c1..1cfa3d2f15cb9aaca33e6ee45e14b141f38580a4 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -50,10 +50,6 @@
> #include <wtf/RefCounted.h>
> #include <wtf/RefPtr.h>
> 
>-#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>-#include "DisplayLink.h"
>-#endif
>-
> namespace API {
> class Navigation;
> class PageConfiguration;
>@@ -421,10 +417,6 @@ private:
> #if PLATFORM(WATCHOS)
>     ProcessThrottler::BackgroundActivityToken m_backgroundActivityTokenForFullscreenFormControls;
> #endif
>-
>-#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
>-    Vector<std::unique_ptr<DisplayLink>> m_displayLinks;
>-#endif
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/mac/DisplayLink.cpp b/Source/WebKit/UIProcess/mac/DisplayLink.cpp
>index bfb032426d98cff388b5104934843811b110ef89..1ad173beec46e5e863139d2d4f2bfab1ee444494 100644
>--- a/Source/WebKit/UIProcess/mac/DisplayLink.cpp
>+++ b/Source/WebKit/UIProcess/mac/DisplayLink.cpp
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2018-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -28,16 +28,13 @@
> 
> #if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
>-#include "DrawingAreaMessages.h"
> #include "WebProcessMessages.h"
>-#include "WebProcessProxy.h"
> #include <wtf/ProcessPrivilege.h>
> 
> namespace WebKit {
>     
>-DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID, WebProcessProxy& webProcessProxy)
>-    : m_connection(webProcessProxy.connection())
>-    , m_displayID(displayID)
>+DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID)
>+    : m_displayID(displayID)
> {
>     ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
>     CVReturn error = CVDisplayLinkCreateWithCGDisplay(displayID, &m_displayLink);
>@@ -51,10 +48,6 @@ DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID, WebProcessProxy&
>         WTFLogAlways("Could not set the display link output callback: %d", error);
>         return;
>     }
>-
>-    error = CVDisplayLinkStart(m_displayLink);
>-    if (error)
>-        WTFLogAlways("Could not start the display link: %d", error);
> }
> 
> DisplayLink::~DisplayLink()
>@@ -68,28 +61,73 @@ DisplayLink::~DisplayLink()
>     CVDisplayLinkRelease(m_displayLink);
> }
> 
>-void DisplayLink::addObserver(unsigned observerID)
>+void DisplayLink::addObserver(IPC::Connection& connection, unsigned observerID)
>+{
>+    ASSERT(RunLoop::isMain());
>+    bool isRunning = !m_observers.isEmpty();
>+
>+    {
>+        LockHolder locker(m_observersLock);
>+        m_observers.ensure(&connection, [] {
>+            return Vector<unsigned> { };
>+        }).iterator->value.append(observerID);
>+    }
>+
>+    if (!isRunning) {
>+        CVReturn error = CVDisplayLinkStart(m_displayLink);
>+        if (error)
>+            WTFLogAlways("Could not start the display link: %d", error);
>+    }
>+}
>+
>+void DisplayLink::removeObserver(IPC::Connection& connection, unsigned observerID)
> {
>-    m_observers.add(observerID);
>+    ASSERT(RunLoop::isMain());
>+
>+    {
>+        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);
>+    }
>+
>+    if (m_observers.isEmpty())
>+        CVDisplayLinkStop(m_displayLink);
> }
> 
>-void DisplayLink::removeObserver(unsigned observerID)
>+void DisplayLink::removeObservers(IPC::Connection& connection)
> {
>-    m_observers.remove(observerID);
>+    ASSERT(RunLoop::isMain());
>+
>+    {
>+        LockHolder locker(m_observersLock);
>+        m_observers.remove(&connection);
>+    }
>+
>+    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)
> {
>-    DisplayLink* displayLink = static_cast<DisplayLink*>(data);
>-    displayLink->m_connection->send(Messages::WebProcess::DisplayWasRefreshed(displayLink->m_displayID), 0);
>+    auto* displayLink = static_cast<DisplayLink*>(data);
>+    LockHolder locker(displayLink->m_observersLock);
>+    for (auto& connection : displayLink->m_observers.keys())
>+        connection->send(Messages::WebProcess::DisplayWasRefreshed(displayLink->m_displayID), 0);
>     return kCVReturnSuccess;
> }
> 
>-}
>+} // namespace WebKit
> 
> #endif
>diff --git a/Source/WebKit/UIProcess/mac/DisplayLink.h b/Source/WebKit/UIProcess/mac/DisplayLink.h
>index fc9c8d7b92ef35449d3e389acc22abe99603d8a0..f34a4651bf51835b34e638fd0a18214fd0b3ed44 100644
>--- a/Source/WebKit/UIProcess/mac/DisplayLink.h
>+++ b/Source/WebKit/UIProcess/mac/DisplayLink.h
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2018-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -28,26 +28,25 @@
> #if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> #include <CoreVideo/CVDisplayLink.h>
>-
> #include <WebCore/PlatformScreen.h>
>-#include <wtf/HashSet.h>
>+#include <wtf/HashMap.h>
>+#include <wtf/Lock.h>
> 
> namespace IPC {
> class Connection;
> }
> 
> namespace WebKit {
>-
>-class WebProcessProxy;
>     
> class DisplayLink {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    DisplayLink(WebCore::PlatformDisplayID, WebProcessProxy&);
>+    explicit DisplayLink(WebCore::PlatformDisplayID);
>     ~DisplayLink();
>     
>-    void addObserver(unsigned observerID);
>-    void removeObserver(unsigned observerID);
>+    void addObserver(IPC::Connection&, unsigned observerID);
>+    void removeObserver(IPC::Connection&, unsigned observerID);
>+    void removeObservers(IPC::Connection&);
>     bool hasObservers() const;
> 
>     WebCore::PlatformDisplayID displayID() const { return m_displayID; }
>@@ -56,12 +55,12 @@ private:
>     static CVReturn displayLinkCallback(CVDisplayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data);
>     
>     CVDisplayLinkRef m_displayLink { nullptr };
>-    HashSet<unsigned> m_observers;
>-    RefPtr<IPC::Connection> m_connection;
>-    WebCore::PlatformDisplayID m_displayID { 0 };
>+    Lock m_observersLock;
>+    HashMap<RefPtr<IPC::Connection>, Vector<unsigned>> m_observers;
>+    WebCore::PlatformDisplayID m_displayID;
> };
> 
>-}
>+} // namespace WebKit
> 
> #endif
> 
>diff --git a/Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm b/Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm
>index 839ea6829b33eb7eb07ec93d647b79efc397bb91..e758c0edb17edb0d68a84dd208c923214d7bf308 100644
>--- a/Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm
>+++ b/Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm
>@@ -74,29 +74,14 @@ bool WebProcessProxy::shouldAllowNonValidInjectedCode() const
> void WebProcessProxy::startDisplayLink(unsigned observerID, uint32_t displayID)
> {
>     ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
>-    for (auto& displayLink : m_displayLinks) {
>-        if (displayLink->displayID() == displayID) {
>-            displayLink->addObserver(observerID);
>-            return;
>-        }
>-    }
>-    auto displayLink = std::make_unique<DisplayLink>(displayID, *this);
>-    displayLink->addObserver(observerID);
>-    m_displayLinks.append(WTFMove(displayLink));
>+    ASSERT(connection());
>+    processPool().startDisplayLink(*connection(), observerID, displayID);
> }
> 
> void WebProcessProxy::stopDisplayLink(unsigned observerID, uint32_t displayID)
> {
>-    size_t pos = 0;
>-    for (auto& displayLink : m_displayLinks) {
>-        if (displayLink->displayID() == displayID) {
>-            displayLink->removeObserver(observerID);
>-            if (!displayLink->hasObservers())
>-                m_displayLinks.remove(pos);
>-            return;
>-        }
>-        pos++;
>-    }
>+    ASSERT(connection());
>+    processPool().stopDisplayLink(*connection(), observerID, displayID);
> }
> #endif
>

Actions: View | Formatted Diff | Diff

Attachments on bug 194463: 361557 | 361559 | 361563 | 361565