Changeset 240759 in webkit

Timestamp:

Jan 30, 2019 8:53:34 PM (5 years ago)

Author:

benjamin@webkit.org

Message:

<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:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/ResponsivenessTimer.cpp (modified) (5 diffs)
• UIProcess/ResponsivenessTimer.h (modified) (3 diffs)
• UIProcess/WebPageProxy.cpp (modified) (1 diff)
• UIProcess/WebProcessProxy.cpp (modified) (1 diff)
• UIProcess/WebProcessProxy.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

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

trunk/Source/WebKit/UIProcess/ResponsivenessTimer.cpp

@@ -33 +33 @@
 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();
 }
@@ -50 +48 @@
 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;
@@ -67 +72 @@
 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;
+    }
 }
 
@@ -84 +99 @@
     }
 
-    m_timer.stop();
+    m_waitingForTimer = false;
+
+    if (m_useLazyStop)
+        m_useLazyStop = false;
+    else
+        m_timer.stop();
 }
 
@@ -90 +110 @@
 {
     // Since there is no web process, we must not be waiting for it anymore.
-    stop();
+    m_waitingForTimer = false;
+    m_timer.stop();
 }
 

trunk/Source/WebKit/UIProcess/ResponsivenessTimer.h

@@ -27 +27 @@
 #define ResponsivenessTimer_h
 
-#include <wtf/RunLoop.h>
+#include <WebCore/Timer.h>
 
 namespace WebKit {
@@ -49 +49 @@
     
     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();
 
@@ -61 +72 @@
 
     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 };
 };
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -2358 +2358 @@
     // 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();
 }
 

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -1118 +1118 @@
 }
 
+void WebProcessProxy::isResponsiveWithLazyStop()
+{
+    if (m_isResponsive == NoOrMaybe::No)
+        return;
+
+    responsivenessTimer().startWithLazyStop();
+    send(Messages::WebProcess::MainThreadPing(), 0);
+}
+
 bool WebProcessProxy::isJITEnabled() const
 {

trunk/Source/WebKit/UIProcess/WebProcessProxy.h

@@ -207 +207 @@
 
     void isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&&);
+    void isResponsiveWithLazyStop();
     void didReceiveMainThreadPing();
     void didReceiveBackgroundResponsivenessPing();