Attachment 360676 Details for Bug 194003 – Patch

[patch] Patch

bug-194003-20190130192759.patch (text/plain), 8.44 KB, created by Benjamin Poulain on 2019-01-30 19:28:00 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin Poulain

Created: 2019-01-30 19:28:00 PST

Size: 8.44 KB

patch

obsolete

>Subversion Revision: 240742
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6d0f8350f698bbe32eddaa430009b0a5883f268f..61052e341141217a6a3f0eef51b097d44461a074 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,51 @@
>+2019-01-30  Benjamin Poulain  <benjamin@webkit.org>
>+
>+        <rdar://problem/47570443> Responsiveness timers are too expensive for frequent events
>+        https://bugs.webkit.org/show_bug.cgi?id=194003
>+
>+        Reviewed by Geoffrey Garen.
>+
>+        The problem here is specific to wheel events.
>+
>+        For every wheel event, we start a responsiveness timer and send
>+        a ping to the WebProcess. When the WebProcess respond, we stop the timer.
>+
>+        The cost of setting up the timers adds up since we get many events.
>+
>+        The first step to improve the situation was to switch ResponsivenessTimer
>+        to WebCore::Timer. Since WebCore::Timer reuse the same CFRunLoopTimerRef,
>+        we save the allocation/deallocation, insertion in the event loop, etc.
>+
>+        Using WebCore::Timer saves some instructions but we were still hitting
>+        the kernel at 120hz to set up then kill each timer.
>+        The second improvement of the patch is to avoid that by not killing the timer
>+        when we hear back from the WebProcess.
>+
>+        Instead of killing the timer, we let it run and ignore the result.
>+        When the next event comes, we reschedule the existing timer.
>+        This brings down the timers to 60Hz, the same rate as the events.
>+
>+        The very last event does time out. In that case, we have a bad idle wake up:
>+        we wake up a sleeping CPU do do nothing.
>+        In the case of wheel events, this is fine since we saved a bunch of CPU already.
>+        For all the other cases, I kept the normal operating mode to avoid the idle wake.
>+
>+        * UIProcess/ResponsivenessTimer.cpp:
>+        (WebKit::ResponsivenessTimer::ResponsivenessTimer):
>+        (WebKit::ResponsivenessTimer::invalidate):
>+        (WebKit::ResponsivenessTimer::timerFired):
>+        (WebKit::ResponsivenessTimer::start):
>+        (WebKit::ResponsivenessTimer::startWithLazyStop):
>+        (WebKit::ResponsivenessTimer::stop):
>+        (WebKit::ResponsivenessTimer::processTerminated):
>+        (WebKit::ResponsivenessTimer::~ResponsivenessTimer): Deleted.
>+        * UIProcess/ResponsivenessTimer.h:
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::sendWheelEvent):
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::isResponsiveWithLazyStop):
>+        * UIProcess/WebProcessProxy.h:
>+
> 2019-01-30  Daniel Bates  <dabates@apple.com>
> 
>         [iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element
>diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>index 9ae5fd1c71103949716305c9d92bbdf524675f13..9002ba1e2ce6f57d94d68a7b11d9c87ab9216aa1 100644
>--- a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>@@ -32,27 +32,32 @@ static const Seconds responsivenessTimeout { 3_s };
> 
> ResponsivenessTimer::ResponsivenessTimer(ResponsivenessTimer::Client& client)
>     : m_client(client)
>-    , m_isResponsive(true)
>-    , m_timer(RunLoop::main(), this, &ResponsivenessTimer::timerFired)
>+    , m_timer(*this, &ResponsivenessTimer::timerFired)
> {
> }
> 
>-ResponsivenessTimer::~ResponsivenessTimer()
>-{
>-    m_timer.stop();
>-}
>+ResponsivenessTimer::~ResponsivenessTimer() = default;
> 
> void ResponsivenessTimer::invalidate()
> {
>+    m_waitingForTimer = false;
>+    m_useLazyStop = false;
>     m_timer.stop();
> }
> 
> void ResponsivenessTimer::timerFired()
> {
>+    if (!m_waitingForTimer)
>+        return;
>+
>+    m_waitingForTimer = false;
>+    m_useLazyStop = false;
>+
>     if (!m_isResponsive)
>         return;
> 
>     if (!m_client.mayBecomeUnresponsive()) {
>+        m_waitingForTimer = true;
>         m_timer.startOneShot(responsivenessTimeout);
>         return;
>     }
>@@ -66,12 +71,22 @@ void ResponsivenessTimer::timerFired()
>     
> void ResponsivenessTimer::start()
> {
>-    if (m_timer.isActive())
>+    if (m_waitingForTimer)
>         return;
> 
>+    m_waitingForTimer = true;
>+    m_useLazyStop = false;
>     m_timer.startOneShot(responsivenessTimeout);
> }
> 
>+void ResponsivenessTimer::startWithLazyStop()
>+{
>+    if (!m_waitingForTimer) {
>+        start();
>+        m_useLazyStop = true;
>+    }
>+}
>+
> void ResponsivenessTimer::stop()
> {
>     if (!m_isResponsive) {
>@@ -83,13 +98,19 @@ void ResponsivenessTimer::stop()
>         m_client.didBecomeResponsive();
>     }
> 
>-    m_timer.stop();
>+    m_waitingForTimer = false;
>+
>+    if (m_useLazyStop)
>+        m_useLazyStop = false;
>+    else
>+        m_timer.stop();
> }
> 
> void ResponsivenessTimer::processTerminated()
> {
>     // Since there is no web process, we must not be waiting for it anymore.
>-    stop();
>+    m_waitingForTimer = false;
>+    m_timer.stop();
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.h b/Source/WebKit/UIProcess/ResponsivenessTimer.h
>index 7dd7dbbf5b3509b978a482a11016e9607a1369ab..d2df806027080014a9c481474c0f288b134f50fe 100644
>--- a/Source/WebKit/UIProcess/ResponsivenessTimer.h
>+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.h
>@@ -26,7 +26,7 @@
> #ifndef ResponsivenessTimer_h
> #define ResponsivenessTimer_h
> 
>-#include <wtf/RunLoop.h>
>+#include <WebCore/Timer.h>
> 
> namespace WebKit {
> 
>@@ -48,6 +48,17 @@ public:
>     ~ResponsivenessTimer();
>     
>     void start();
>+
>+    // A responsiveness timer with lazy stop does not stop the underlying system timer when stopped.
>+    // Instead, it ignores the timeout if stop() was already called.
>+    //
>+    // This exists to reduce the rate at which we reset the timer.
>+    //
>+    // With a non lazy timer, we may set a timer and reset it soon after because the process is responsive.
>+    // For events, this means reseting a timer 120 times/s for a 60 Hz event source.
>+    // By not reseting the timer when responsive, we cut that in half to 60 timeout changes.
>+    void startWithLazyStop();
>+
>     void stop();
> 
>     void invalidate();
>@@ -60,9 +71,10 @@ private:
>     void timerFired();
> 
>     ResponsivenessTimer::Client& m_client;
>-    bool m_isResponsive;
>-
>-    RunLoop::Timer<ResponsivenessTimer> m_timer;
>+    WebCore::Timer m_timer;
>+    bool m_isResponsive { true };
>+    bool m_waitingForTimer { false };
>+    bool m_useLazyStop { false };
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 520fe2ce7823cff705f386a92bd33ec8bee50a61..f8a299508792811026c6bf24967a9563fcca8ee2 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -2357,7 +2357,7 @@ void WebPageProxy::sendWheelEvent(const WebWheelEvent& event)
> 
>     // Manually ping the web process to check for responsiveness since our wheel
>     // event will dispatch to a non-main thread, which always responds.
>-    m_process->isResponsive(nullptr);
>+    m_process->isResponsiveWithLazyStop();
> }
> 
> bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event) const
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 22cb195b3cd90351ad683b79997075f986e3090d..86d56b46d370d59b190bf71fddd017ed54d546b5 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -1117,6 +1117,15 @@ void WebProcessProxy::isResponsive(WTF::Function<void(bool isWebProcessResponsiv
>     send(Messages::WebProcess::MainThreadPing(), 0);
> }
> 
>+void WebProcessProxy::isResponsiveWithLazyStop()
>+{
>+    if (m_isResponsive == NoOrMaybe::No)
>+        return;
>+
>+    responsivenessTimer().startWithLazyStop();
>+    send(Messages::WebProcess::MainThreadPing(), 0);
>+}
>+
> bool WebProcessProxy::isJITEnabled() const
> {
>     return processPool().configuration().isJITEnabled();
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index b1039ecb8af6cce514ef5a4c514015f8e55bf4c1..8df5c25344c23f031d1d1ba05f3962aaa85fc8b0 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -206,6 +206,7 @@ public:
>     ProcessThrottler& throttler() { return m_throttler; }
> 
>     void isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&&);
>+    void isResponsiveWithLazyStop();
>     void didReceiveMainThreadPing();
>     void didReceiveBackgroundResponsivenessPing();
>

Actions: View | Formatted Diff | Diff

Attachments on bug 194003: 360549 | 360676 | 361354 | 361766 | 361773 | 361777