WebKit Bugzilla
Attachment 361766 Details for
Bug 194003
: Responsiveness timers are too expensive for frequent events
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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(); >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194003
:
360549
|
360676
|
361354
|
361766
|
361773
|
361777