- Revision
- 267043
- Author
- [email protected]
- Date
- 2020-09-14 15:17:05 -0700 (Mon, 14 Sep 2020)
Log Message
Cherry-pick r266797. rdar://problem/68881018
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.
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:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266797 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-610-branch/Source/WebCore/ChangeLog (267042 => 267043)
--- branches/safari-610-branch/Source/WebCore/ChangeLog 2020-09-14 21:50:38 UTC (rev 267042)
+++ branches/safari-610-branch/Source/WebCore/ChangeLog 2020-09-14 22:17:05 UTC (rev 267043)
@@ -1,3 +1,66 @@
+2020-09-14 Alan Coon <[email protected]>
+
+ Cherry-pick r266797. rdar://problem/68881018
+
+ 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.
+
+ 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:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266797 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-09-09 Chris Dumez <[email protected]>
+
+ 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-11 Russell Epstein <[email protected]>
Cherry-pick r266743. rdar://problem/68652752
Modified: branches/safari-610-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (267042 => 267043)
--- branches/safari-610-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2020-09-14 21:50:38 UTC (rev 267042)
+++ branches/safari-610-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2020-09-14 22:17:05 UTC (rev 267043)
@@ -62,7 +62,7 @@
bool shouldBeTerminated() const
{
- const int maxInactiveFireCount = 20;
+ const int maxInactiveFireCount = 1;
return !m_scheduled && m_unscheduledFireCount > maxInactiveFireCount;
}
Modified: branches/safari-610-branch/Source/WebKit/ChangeLog (267042 => 267043)
--- branches/safari-610-branch/Source/WebKit/ChangeLog 2020-09-14 21:50:38 UTC (rev 267042)
+++ branches/safari-610-branch/Source/WebKit/ChangeLog 2020-09-14 22:17:05 UTC (rev 267043)
@@ -1,3 +1,69 @@
+2020-09-14 Alan Coon <[email protected]>
+
+ Cherry-pick r266797. rdar://problem/68881018
+
+ 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.
+
+ 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:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266797 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-09-09 Chris Dumez <[email protected]>
+
+ 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-11 Russell Epstein <[email protected]>
Cherry-pick r266771. rdar://problem/68666453
Modified: branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.cpp (267042 => 267043)
--- branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.cpp 2020-09-14 21:50:38 UTC (rev 267042)
+++ branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.cpp 2020-09-14 22:17:05 UTC (rev 267043)
@@ -35,6 +35,7 @@
namespace WebKit {
bool DisplayLink::shouldSendIPCOnBackgroundQueue { false };
+constexpr unsigned maxFireCountWithoutObservers { 20 };
DisplayLink::DisplayLink(WebCore::PlatformDisplayID displayID)
: m_displayID(displayID)
@@ -75,7 +76,6 @@
void DisplayLink::addObserver(IPC::Connection& connection, DisplayLinkObserverID observerID)
{
ASSERT(RunLoop::isMain());
- bool isRunning = !m_observers.isEmpty();
{
LockHolder locker(m_observersLock);
@@ -84,7 +84,7 @@
}).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);
@@ -95,20 +95,19 @@
{
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);
- if (m_observers.isEmpty())
- CVDisplayLinkStop(m_displayLink);
+ // We do not stop the display link right away when |m_observers| becomes empty. Instead, we
+ // let the display link fire up to |maxFireCountWithoutObservers| times without observers to avoid
+ // killing & restarting too many threads when observers gets removed & added in quick succession.
}
void DisplayLink::removeObservers(IPC::Connection& connection)
@@ -115,32 +114,38 @@
{
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);
+ // We do not stop the display link right away when |m_observers| becomes empty. Instead, we
+ // let the display link fire up to |maxFireCountWithoutObservers| times without observers to avoid
+ // killing & restarting too many threads when observers gets removed & added in quick succession.
}
-bool DisplayLink::hasObservers() const
+CVReturn DisplayLink::displayLinkCallback(CVDisplayLinkRef displayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data)
{
- ASSERT(RunLoop::isMain());
- return !m_observers.isEmpty();
+ static_cast<DisplayLink*>(data)->notifyObserversDisplayWasRefreshed();
+ return kCVReturnSuccess;
}
-CVReturn DisplayLink::displayLinkCallback(CVDisplayLinkRef displayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data)
+void DisplayLink::notifyObserversDisplayWasRefreshed()
{
- auto* displayLink = static_cast<DisplayLink*>(data);
- LockHolder locker(displayLink->m_observersLock);
- for (auto& connection : displayLink->m_observers.keys()) {
+ ASSERT(!RunLoop::isMain());
+
+ LockHolder locker(m_observersLock);
+ if (m_observers.isEmpty()) {
+ if (++m_fireCountWithoutObservers >= maxFireCountWithoutObservers)
+ CVDisplayLinkStop(m_displayLink);
+ return;
+ }
+ m_fireCountWithoutObservers = 0;
+
+ for (auto& connection : m_observers.keys()) {
if (shouldSendIPCOnBackgroundQueue)
- connection->send(Messages::EventDispatcher::DisplayWasRefreshed(displayLink->m_displayID), 0);
+ connection->send(Messages::EventDispatcher::DisplayWasRefreshed(m_displayID), 0);
else
- connection->send(Messages::WebProcess::DisplayWasRefreshed(displayLink->m_displayID), 0);
+ connection->send(Messages::WebProcess::DisplayWasRefreshed(m_displayID), 0);
}
- return kCVReturnSuccess;
}
} // namespace WebKit
Modified: branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.h (267042 => 267043)
--- branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.h 2020-09-14 21:50:38 UTC (rev 267042)
+++ branches/safari-610-branch/Source/WebKit/UIProcess/mac/DisplayLink.h 2020-09-14 22:17:05 UTC (rev 267043)
@@ -48,7 +48,6 @@
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,11 +59,13 @@
private:
static CVReturn displayLinkCallback(CVDisplayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data);
-
+ void notifyObserversDisplayWasRefreshed();
+
CVDisplayLinkRef m_displayLink { nullptr };
Lock m_observersLock;
HashMap<RefPtr<IPC::Connection>, Vector<DisplayLinkObserverID>> m_observers;
WebCore::PlatformDisplayID m_displayID;
+ unsigned m_fireCountWithoutObservers { 0 };
static bool shouldSendIPCOnBackgroundQueue;
};
