Skip to content
Permalink
Browse files
Move lazy DisplayLink tear down logic from the WebProcess to the UIPr…
…ocess

https://bugs.webkit.org/show_bug.cgi?id=216195

Reviewed by Simon Fraser.

Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess, now that the
DisplayLink has been moved to the UIProcess due to sandboxing.

After a DisplayLink no longer has any clients, we keep it firing up to 20 times without
any clients in case a new client gets added shortly after. The idea was to avoid killing
and respawning too many threads when adding and removing clients in quick succession.
However, now that the DisplayLink lives in the UIProcess side and sends IPC to the
WebProcesses every time it fires, it makes a lot more sense to implement this logic in
the UIProcess side, to avoid sending unnecessary IPC to processes that do not care about
it.

Source/WebCore:

* platform/graphics/DisplayRefreshMonitor.cpp:
(WebCore::DisplayRefreshMonitor::displayDidRefresh):
* platform/graphics/DisplayRefreshMonitor.h:
(WebCore::DisplayRefreshMonitor::shouldBeTerminated const):

Source/WebKit:

* UIProcess/mac/DisplayLink.cpp:
(WebKit::DisplayLink::addObserver):
(WebKit::DisplayLink::removeObserver):
(WebKit::DisplayLink::removeObservers):
(WebKit::DisplayLink::displayLinkCallback):
(WebKit::DisplayLink::hasObservers const): Deleted.
* UIProcess/mac/DisplayLink.h:


Canonical link: https://commits.webkit.org/229014@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266645 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
cdumez committed Sep 4, 2020
1 parent 9fce9fc commit 3e300055ba2da5649906b90cf1eece66b7b3ae8c
@@ -1,3 +1,26 @@
2020-09-04 Chris Dumez <cdumez@apple.com>

Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=216195

Reviewed by Simon Fraser.

Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess, now that the
DisplayLink has been moved to the UIProcess due to sandboxing.

After a DisplayLink no longer has any clients, we keep it firing up to 20 times without
any clients in case a new client gets added shortly after. The idea was to avoid killing
and respawning too many threads when adding and removing clients in quick succession.
However, now that the DisplayLink lives in the UIProcess side and sends IPC to the
WebProcesses every time it fires, it makes a lot more sense to implement this logic in
the UIProcess side, to avoid sending unnecessary IPC to processes that do not care about
it.

* platform/graphics/DisplayRefreshMonitor.cpp:
(WebCore::DisplayRefreshMonitor::displayDidRefresh):
* platform/graphics/DisplayRefreshMonitor.h:
(WebCore::DisplayRefreshMonitor::shouldBeTerminated const):

2020-09-04 Jer Noble <jer.noble@apple.com>

[Cocoa,EME] -outputObscuredDueToInsufficientExternalProtection KVO will set OutputObscured for all attached MediaKeySessions.
@@ -94,12 +94,7 @@ void DisplayRefreshMonitor::displayDidRefresh()
{
{
LockHolder lock(m_mutex);
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;

LOG(RequestAnimationFrame, "DisplayRefreshMonitor::displayDidRefresh(%p) - m_scheduled(%d)", this, m_scheduled);
m_scheduled = false;
}

@@ -60,11 +60,7 @@ class DisplayRefreshMonitor : public ThreadSafeRefCounted<DisplayRefreshMonitor>

PlatformDisplayID displayID() const { return m_displayID; }

bool shouldBeTerminated() const
{
const int maxInactiveFireCount = 20;
return !m_scheduled && m_unscheduledFireCount > maxInactiveFireCount;
}
bool shouldBeTerminated() const { return !m_scheduled; }

static RefPtr<DisplayRefreshMonitor> createDefaultDisplayRefreshMonitor(PlatformDisplayID);

@@ -94,7 +90,6 @@ 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 };
@@ -1,3 +1,29 @@
2020-09-04 Chris Dumez <cdumez@apple.com>

Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=216195

Reviewed by Simon Fraser.

Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess, now that the
DisplayLink has been moved to the UIProcess due to sandboxing.

After a DisplayLink no longer has any clients, we keep it firing up to 20 times without
any clients in case a new client gets added shortly after. The idea was to avoid killing
and respawning too many threads when adding and removing clients in quick succession.
However, now that the DisplayLink lives in the UIProcess side and sends IPC to the
WebProcesses every time it fires, it makes a lot more sense to implement this logic in
the UIProcess side, to avoid sending unnecessary IPC to processes that do not care about
it.

* UIProcess/mac/DisplayLink.cpp:
(WebKit::DisplayLink::addObserver):
(WebKit::DisplayLink::removeObserver):
(WebKit::DisplayLink::removeObservers):
(WebKit::DisplayLink::displayLinkCallback):
(WebKit::DisplayLink::hasObservers const): Deleted.
* UIProcess/mac/DisplayLink.h:

2020-09-04 Sihui Liu <sihui_liu@apple.com>

Webpages flash when switching between windows
@@ -32,6 +32,8 @@
#include <wtf/ProcessPrivilege.h>

namespace WebKit {

constexpr unsigned maxFireCountWithObservers { 20 };

DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID)
: m_displayID(displayID)
@@ -72,16 +74,13 @@ 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 (!isRunning) {
if (!CVDisplayLinkIsRunning(m_displayLink)) {
CVReturn error = CVDisplayLinkStart(m_displayLink);
if (error)
WTFLogAlways("Could not start the display link: %d", error);
@@ -92,45 +91,46 @@ void DisplayLink::removeObserver(IPC::Connection& connection, DisplayLinkObserve
{
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);
}
LockHolder locker(m_observersLock);

if (m_observers.isEmpty())
CVDisplayLinkStop(m_displayLink);
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.
}

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);

if (m_observers.isEmpty())
CVDisplayLinkStop(m_displayLink);
}

bool DisplayLink::hasObservers() const
{
ASSERT(RunLoop::isMain());
return !m_observers.isEmpty();
// 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.
}

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;
@@ -48,7 +48,6 @@ 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; }

@@ -61,6 +60,7 @@ class DisplayLink {
Lock m_observersLock;
HashMap<RefPtr<IPC::Connection>, Vector<DisplayLinkObserverID>> m_observers;
WebCore::PlatformDisplayID m_displayID;
unsigned m_fireCountWithoutObservers { 0 };
};

} // namespace WebKit

0 comments on commit 3e30005

Please sign in to comment.