Attachment 360549 Details for Bug 194003 – Patch

[patch] Patch

bug-194003-20190129211335.patch (text/plain), 8.25 KB, created by Benjamin Poulain on 2019-01-29 21:13:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin Poulain

Created: 2019-01-29 21:13:35 PST

Size: 8.25 KB

patch

obsolete

>Subversion Revision: 240595
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 4ab33d8124bb3b31afe31ff8bc80f32aa6af0dae..017438bf77da1a19b361199a3e81efaadc4a7a2c 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,51 @@
>+2019-01-29  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 NOBODY (OOPS!).
>+
>+        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 find 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-28  Antoine Quint  <graouts@apple.com>
> 
>         Limit user-agent interactions based on the touch-action property on iOS
>diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>index 9ae5fd1c71103949716305c9d92bbdf524675f13..f2eedfb515e86b246e4792ec9d37da4aefcb6a23 100644
>--- a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>@@ -32,23 +32,27 @@ 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;
> 
>@@ -66,12 +70,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 +97,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 1207c67a2c2f98bff095f569d42539488d650743..aa19aa897246b0501950309bcd71ef2f537a907f 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -2350,7 +2350,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 050f7c39f2492aa8184b4317f47eb93c38d5f1da..63e67ddd150175acfb8e8c6569c099fb3374a8c5 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 835686c63013cbcd9d03357b01757f81c40ebd02..bc1b7bb8bd69d60dfa777a8eb394e5e94a05a86c 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