Changeset 241351 in webkit

Timestamp:

Feb 13, 2019 12:41:22 AM (5 years ago)

Author:

benjamin@webkit.org

Message:

Responsiveness timers are too expensive for frequent events
​https://bugs.webkit.org/show_bug.cgi?id=194003

Reviewed by Geoffrey Garen.

With each event, we set a responsivness timer to check if the WebProcess
is responsive, and reset the timer when the WebProcess sends an answer.

For frequent events (e.g. wheel events, mouse force events, etc),
we are spamming the kernel with hundreds of timers per second.
That is a bit inefficient.

Another source of inefficiency comes from the timer implementation
itself. Stopping a RunLoop::Timer removes the timer from every mode
and invalidate the timer. It becomes costly since we do it a lot.

With this patch, I tweak ResponsivenessTimer and its use to minimize
how often we schedule system timers.

The first change is to not stop the timer when we get the stop()
calls if we expect more events to come in. Instead, we keep track
if we care about the timeout or not in the attribute "m_waitingForTimer".
When the next event starts, we can reschedule the timer without ever
having told the kernel about the stop.
If there are no next events, the timeout fires but m_waitingForTimer
is false. To avoid idle wake up, the lazy stop is only used when having
following events is common.

The second improvements comes from not even rescheduling the timer
when restarted. Instead of changing the timer, we let the original timer
fire and re-shedule a new one with the missing time.

For more context, also see patches r240759 and r240944.

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

(WebKit::ResponsivenessTimer::hasActiveTimer const):

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::processNextQueuedMouseEvent):
(WebKit::WebPageProxy::sendWheelEvent):
(WebKit::WebPageProxy::handleKeyboardEvent):
(WebKit::WebPageProxy::handleGestureEvent):

• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::isResponsiveWithLazyStop):

• UIProcess/WebProcessProxy.h:

Location:

trunk/Source/WebKit

Files:

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

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-02-13  Benjamin Poulain  <benjamin@webkit.org>
+
+        Responsiveness timers are too expensive for frequent events
+        https://bugs.webkit.org/show_bug.cgi?id=194003
+
+        Reviewed by Geoffrey Garen.
+
+        With each event, we set a responsivness timer to check if the WebProcess
+        is responsive, and reset the timer when the WebProcess sends an answer.
+
+        For frequent events (e.g. wheel events, mouse force events, etc),
+        we are spamming the kernel with hundreds of timers per second.
+        That is a bit inefficient.
+
+        Another source of inefficiency comes from the timer implementation
+        itself. Stopping a RunLoop::Timer removes the timer from every mode
+        and invalidate the timer. It becomes costly since we do it a lot.
+
+        With this patch, I tweak ResponsivenessTimer and its use to minimize
+        how often we schedule system timers.
+
+        The first change is to not stop the timer when we get the stop()
+        calls if we expect more events to come in. Instead, we keep track
+        if we care about the timeout or not in the attribute "m_waitingForTimer".
+        When the next event starts, we can reschedule the timer without ever
+        having told the kernel about the stop.
+        If there are no next events, the timeout fires but m_waitingForTimer
+        is false. To avoid idle wake up, the lazy stop is only used when having
+        following events is common.
+
+        The second improvements comes from not even rescheduling the timer
+        when restarted. Instead of changing the timer, we let the original timer
+        fire and re-shedule a new one with the missing time.
+
+        For more context, also see patches r240759 and r240944.
+
+        * 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:
+        (WebKit::ResponsivenessTimer::hasActiveTimer const):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::processNextQueuedMouseEvent):
+        (WebKit::WebPageProxy::sendWheelEvent):
+        (WebKit::WebPageProxy::handleKeyboardEvent):
+        (WebKit::WebPageProxy::handleGestureEvent):
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::isResponsiveWithLazyStop):
+        * UIProcess/WebProcessProxy.h:
+
 2019-02-12  Tim Horton  <timothy_horton@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)
 {
 }
 
-ResponsivenessTimer::~ResponsivenessTimer()
-{
-    m_timer.stop();
-}
+ResponsivenessTimer::~ResponsivenessTimer() = default;
 
 void ResponsivenessTimer::invalidate()
 {
     m_timer.stop();
+    m_restartFireTime = MonotonicTime();
+    m_waitingForTimer = false;
+    m_useLazyStop = false;
 }
 
 void ResponsivenessTimer::timerFired()
 {
+    if (!m_waitingForTimer)
+        return;
+
+    if (m_restartFireTime) {
+        MonotonicTime now = MonotonicTime::now();
+        MonotonicTime restartFireTime = m_restartFireTime;
+        m_restartFireTime = MonotonicTime();
+
+        if (restartFireTime > now) {
+            m_timer.startOneShot(now - restartFireTime);
+            return;
+        }
+    }
+
+    m_waitingForTimer = false;
+    m_useLazyStop = false;
+
     if (!m_isResponsive)
         return;
 
     if (!m_client.mayBecomeUnresponsive()) {
+        m_waitingForTimer = true;
         m_timer.startOneShot(responsivenessTimeout);
         return;
@@ -67 +84 @@
 void ResponsivenessTimer::start()
 {
-    if (m_timer.isActive())
+    if (m_waitingForTimer)
         return;
 
-    m_timer.startOneShot(responsivenessTimeout);
+    m_waitingForTimer = true;
+    m_useLazyStop = false;
+
+    if (m_timer.isActive()) {
+        // The timer is still active from a lazy stop.
+        // Instead of restarting the timer, we schedule a new delay after this one finishes.
+        //
+        // In most cases, stop is called before we get to schedule the second timer, saving us
+        // the scheduling of the timer entirely.
+        m_restartFireTime = MonotonicTime::now() + responsivenessTimeout;
+    } else {
+        m_restartFireTime = MonotonicTime();
+        m_timer.startOneShot(responsivenessTimeout);
+    }
+}
+
+void ResponsivenessTimer::startWithLazyStop()
+{
+    if (!m_waitingForTimer) {
+        start();
+        m_useLazyStop = true;
+    }
 }
 
@@ -84 +122 @@
     }
 
-    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();
+    invalidate();
 }
 

trunk/Source/WebKit/UIProcess/ResponsivenessTimer.h

@@ -47 +47 @@
     explicit ResponsivenessTimer(ResponsivenessTimer::Client&);
     ~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();
-    
+
+    // Return true if stop() was not called betfore the responsiveness timeout.
     bool isResponsive() const { return m_isResponsive; }
+
+    // Return true if there is an active timer. The state could be responsive or not.
+    bool hasActiveTimer() const { return m_waitingForTimer; }
 
     void processTerminated();
@@ -61 +76 @@
 
     ResponsivenessTimer::Client& m_client;
-    bool m_isResponsive;
 
     RunLoop::Timer<ResponsivenessTimer> m_timer;
+    MonotonicTime m_restartFireTime;
+
+    bool m_isResponsive { true };
+    bool m_waitingForTimer { false };
+    bool m_useLazyStop { false };
 };
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -2233 +2233 @@
 
     const NativeWebMouseEvent& event = m_mouseEventQueue.first();
-    
+
     if (pageClient().windowIsFrontWindowUnderMouse(event))
         setToolTip(String());
 
-    // NOTE: This does not start the responsiveness timer because mouse move should not indicate interaction.
-    if (event.type() != WebEvent::MouseMove)
+    WebEvent::Type eventType = event.type();
+    if (eventType == WebEvent::MouseDown || eventType == WebEvent::MouseForceChanged || eventType == WebEvent::MouseForceDown)
+        m_process->responsivenessTimer().startWithLazyStop();
+    else if (eventType != WebEvent::MouseMove) {
+        // NOTE: This does not start the responsiveness timer because mouse move should not indicate interaction.
         m_process->responsivenessTimer().start();
-
-    LOG(MouseHandling, "UIProcess: sent mouse event %s (queue size %zu)", webMouseEventTypeString(event.type()), m_mouseEventQueue.size());
+    }
+
+    LOG(MouseHandling, "UIProcess: sent mouse event %s (queue size %zu)", webMouseEventTypeString(eventType), m_mouseEventQueue.size());
     m_process->send(Messages::WebPage::MouseEvent(event), m_pageID);
 }
@@ -2361 +2365 @@
     // 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();
 }
 
@@ -2394 +2398 @@
     m_keyEventQueue.append(event);
 
-    m_process->responsivenessTimer().start();
+    ResponsivenessTimer& responsivenessTimer = m_process->responsivenessTimer();
+    if (event.type() == WebEvent::KeyDown)
+        responsivenessTimer.startWithLazyStop();
+    else
+        responsivenessTimer.start();
+
     if (m_keyEventQueue.size() == 1) { // Otherwise, sent from DidReceiveEvent message handler.
         LOG(KeyHandling, " UI process: sent keyEvent from handleKeyboardEvent");
@@ -2556 +2565 @@
     m_gestureEventQueue.append(event);
     // FIXME: Consider doing some coalescing here.
-    m_process->responsivenessTimer().start();
+
+    ResponsivenessTimer& responsivenessTimer = m_process->responsivenessTimer();
+    if (event.type() == WebEvent::GestureStart || event.type() == WebEvent::GestureChange)
+        responsivenessTimer.startWithLazyStop();
+    else
+        responsivenessTimer.start();
 
     m_process->send(Messages::EventDispatcher::GestureEvent(m_pageID, event), 0);

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -1134 +1134 @@
 }
 
+void WebProcessProxy::isResponsiveWithLazyStop()
+{
+    if (m_isResponsive == NoOrMaybe::No)
+        return;
+
+    if (!responsivenessTimer().hasActiveTimer()) {
+        // We do not send a ping if we are already waiting for the WebProcess.
+        // Spamming pings on a slow web process is not helpful.
+        responsivenessTimer().startWithLazyStop();
+        send(Messages::WebProcess::MainThreadPing(), 0);
+    }
+}
+
 bool WebProcessProxy::isJITEnabled() const
 {

trunk/Source/WebKit/UIProcess/WebProcessProxy.h

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