Attachment 361766 Details for Bug 194003 – Patch

[patch] Patch

bug-194003-20190211213831.patch (text/plain), 12.44 KB, created by Benjamin Poulain on 2019-02-11 21:38:31 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin Poulain

Created: 2019-02-11 21:38:31 PST

Size: 12.44 KB

patch

obsolete

>Subversion Revision: 241279
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index b3845ec333a0041f77ae30b772aba60a873d4ea2..675a2645d0f073a12dd5a16dfbbdf888da496e1d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,59 @@
>+2019-02-11  Benjamin Poulain  <benjamin@webkit.org>
>+
>+        Responsiveness timers are too expensive for frequent events
>+        https://bugs.webkit.org/show_bug.cgi?id=194003
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-11  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r241272 and r241276.
>diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>index 9ae5fd1c71103949716305c9d92bbdf524675f13..87bda7d5f60ce43d5d91bd76aededa9e9bf6f141 100644
>--- a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp
>@@ -32,27 +32,44 @@ static const Seconds responsivenessTimeout { 3_s };
> 
> 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;
>     }
>@@ -66,10 +83,31 @@ void ResponsivenessTimer::timerFired()
>     
> 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;
>+    }
> }
> 
> void ResponsivenessTimer::stop()
>@@ -83,13 +121,17 @@ 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();
>+    invalidate();
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.h b/Source/WebKit/UIProcess/ResponsivenessTimer.h
>index 7dd7dbbf5b3509b978a482a11016e9607a1369ab..32d75cfb5e42ef644a67a521bb117aa8be862809 100644
>--- a/Source/WebKit/UIProcess/ResponsivenessTimer.h
>+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.h
>@@ -46,23 +46,42 @@ public:
> 
>     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();
> 
> private:
>     void timerFired();
> 
>     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 };
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 6a177453fdcd25a99eeca01e799c5c5e24834b3c..570372b87beec01900c6b8d779f0b9915f6c4258 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -2232,15 +2232,19 @@ void WebPageProxy::processNextQueuedMouseEvent()
>     ASSERT(!m_mouseEventQueue.isEmpty());
> 
>     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);
> }
> 
>@@ -2360,7 +2364,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
>@@ -2393,7 +2397,12 @@ void WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
> 
>     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");
>         m_process->send(Messages::WebPage::KeyEvent(event), m_pageID);
>@@ -2555,7 +2564,12 @@ void WebPageProxy::handleGestureEvent(const NativeWebGestureEvent& event)
> 
>     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);
> }
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 920c25bc1d3b11fbf5042c6c81ea7789d9d95237..e686828590a54805df3e31790ff85042eaf573ce 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -1133,6 +1133,19 @@ void WebProcessProxy::isResponsive(WTF::Function<void(bool isWebProcessResponsiv
>     send(Messages::WebProcess::MainThreadPing(), 0);
> }
> 
>+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
> {
>     return processPool().configuration().isJITEnabled();
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index 1cfa3d2f15cb9aaca33e6ee45e14b141f38580a4..ea80979621e8162c82d6207cc3e2b4681a5f216f 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -202,6 +202,7 @@ public:
>     ProcessThrottler& throttler() { return m_throttler; }
> 
>     void isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&&);
>+    void isResponsiveWithLazyStop();
>     void didReceiveMainThreadPing();
>     void didReceiveBackgroundResponsivenessPing();
>

Flags:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

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