WebKit Bugzilla
Attachment 361354 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-20190206180049.patch (text/plain), 19.35 KB, created by
Benjamin Poulain
on 2019-02-06 18:00:49 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Benjamin Poulain
Created:
2019-02-06 18:00:49 PST
Size:
19.35 KB
patch
obsolete
>Subversion Revision: 241111 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index f496024ce162c248c55b417751be1bea9b264c5e..ee4ff5bdad8340d230da99731c7a8c4624dfe5b4 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,22 @@ >+2019-02-06 Benjamin Poulain <benjamin@webkit.org> >+ >+ Unreviewed, rolling out r240759 and r240944. >+ >+ Some timer uses are done off the main thread, WebCore::Timer >+ cannot be used >+ >+ Reverted changesets: >+ >+ "<rdar://problem/47570443> Responsiveness timers are too >+ expensive for frequent events" >+ https://bugs.webkit.org/show_bug.cgi?id=194003 >+ https://trac.webkit.org/changeset/240759 >+ >+ "Use deferrable timer to restart the Responsiveness Timer on >+ each wheel event" >+ https://bugs.webkit.org/show_bug.cgi?id=194135 >+ https://trac.webkit.org/changeset/240944 >+ > 2019-02-06 Keith Rollin <krollin@apple.com> > > Update .xcfilelist files >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index 68247487d4360995925e7948224879923ff7c3c9..0f2114bcb4565922a1a209f3f99137dd60e2b1af 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,22 @@ >+2019-02-06 Benjamin Poulain <benjamin@webkit.org> >+ >+ Unreviewed, rolling out r240759 and r240944. >+ >+ Some timer uses are done off the main thread, WebCore::Timer >+ cannot be used >+ >+ Reverted changesets: >+ >+ "<rdar://problem/47570443> Responsiveness timers are too >+ expensive for frequent events" >+ https://bugs.webkit.org/show_bug.cgi?id=194003 >+ https://trac.webkit.org/changeset/240759 >+ >+ "Use deferrable timer to restart the Responsiveness Timer on >+ each wheel event" >+ https://bugs.webkit.org/show_bug.cgi?id=194135 >+ https://trac.webkit.org/changeset/240944 >+ > 2019-02-06 Daniel Bates <dabates@apple.com> > > Move toNSEventModifierFlags() and toNSButtonNumber() to WebEventFactory.mm >diff --git a/Source/WebCore/css/CSSImageGeneratorValue.cpp b/Source/WebCore/css/CSSImageGeneratorValue.cpp >index ba822e0c24dd461ebf7210121ed5619c55513a97..ffdd3d0aa657f25eaf31d570c5868ed5309e2efd 100644 >--- a/Source/WebCore/css/CSSImageGeneratorValue.cpp >+++ b/Source/WebCore/css/CSSImageGeneratorValue.cpp >@@ -56,7 +56,7 @@ private: > CSSImageGeneratorValue& m_owner; > const FloatSize m_size; > const Ref<GeneratedImage> m_image; >- ResettableOneShotTimer m_evictionTimer; >+ DeferrableOneShotTimer m_evictionTimer; > }; > > CSSImageGeneratorValue::CSSImageGeneratorValue(ClassType classType) >diff --git a/Source/WebCore/html/HTMLPlugInImageElement.h b/Source/WebCore/html/HTMLPlugInImageElement.h >index 1179620f96aa5a2aec0779e1d8bd73c8d2a4dcf0..ce0ea361cb4d50e6132dd6f8ace6f5b6cdf88f8f 100644 >--- a/Source/WebCore/html/HTMLPlugInImageElement.h >+++ b/Source/WebCore/html/HTMLPlugInImageElement.h >@@ -130,7 +130,7 @@ private: > bool m_needsWidgetUpdate { false }; > bool m_needsDocumentActivationCallbacks { false }; > RefPtr<MouseEvent> m_pendingClickEventFromSnapshot; >- ResettableOneShotTimer m_simulatedMouseClickTimer; >+ DeferrableOneShotTimer m_simulatedMouseClickTimer; > Timer m_removeSnapshotTimer; > RefPtr<Image> m_snapshotImage; > bool m_createdDuringUserGesture { false }; >diff --git a/Source/WebCore/loader/cache/CachedResource.h b/Source/WebCore/loader/cache/CachedResource.h >index c8f78805a84933c936ed7b2e37f1d954be095a2b..9b83848ea2249d587a3ed81aff25e7864b0290f2 100644 >--- a/Source/WebCore/loader/cache/CachedResource.h >+++ b/Source/WebCore/loader/cache/CachedResource.h >@@ -311,7 +311,7 @@ protected: > ResourceRequest m_resourceRequest; > ResourceResponse m_response; > >- ResettableOneShotTimer m_decodedDataDeletionTimer; >+ DeferrableOneShotTimer m_decodedDataDeletionTimer; > > // FIXME: Make the rest of these data members private and use functions in derived classes instead. > HashCountedSet<CachedResourceClient*> m_clients; >diff --git a/Source/WebCore/platform/Timer.cpp b/Source/WebCore/platform/Timer.cpp >index bd5b59ee321e2aac2764fe837bd10f6412b91931..fd82402235f3e72e699459a6021183858af7cb02 100644 >--- a/Source/WebCore/platform/Timer.cpp >+++ b/Source/WebCore/platform/Timer.cpp >@@ -260,7 +260,7 @@ TimerBase::TimerBase() > > TimerBase::~TimerBase() > { >- assertThreadSafety(); >+ ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); > RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || shouldSuppressThreadSafetyCheck()); > stop(); > ASSERT(!inHeap()); >@@ -273,7 +273,7 @@ TimerBase::~TimerBase() > > void TimerBase::start(Seconds nextFireInterval, Seconds repeatInterval) > { >- assertThreadSafety(); >+ ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); > > m_repeatInterval = repeatInterval; > setNextFireTime(MonotonicTime::now() + nextFireInterval); >@@ -281,7 +281,7 @@ void TimerBase::start(Seconds nextFireInterval, Seconds repeatInterval) > > void TimerBase::stop() > { >- assertThreadSafety(); >+ ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); > > m_repeatInterval = 0_s; > setNextFireTime(MonotonicTime { }); >@@ -465,7 +465,7 @@ void TimerBase::updateHeapIfNeeded(MonotonicTime oldTime) > > void TimerBase::setNextFireTime(MonotonicTime newTime) > { >- assertThreadSafety(); >+ ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); > RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || shouldSuppressThreadSafetyCheck()); > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_wasDeleted); > >@@ -520,53 +520,5 @@ Seconds TimerBase::nextUnalignedFireInterval() const > return std::max(m_unalignedNextFireTime - MonotonicTime::now(), 0_s); > } > >-DeferrableOneShotTimer::DeferrableOneShotTimer(WTF::Function<void()>&& function) >- : m_function(WTFMove(function)) >-{ >-} >- >-DeferrableOneShotTimer::~DeferrableOneShotTimer() = default; >- >-void DeferrableOneShotTimer::startOneShot(Seconds interval) >-{ >- assertThreadSafety(); >- >- MonotonicTime oldNextFireTime = TimerBase::nextFireTime(); >- if (!oldNextFireTime) { >- m_restartFireTime = MonotonicTime(); >- TimerBase::startOneShot(interval); >- return; >- } >- >- MonotonicTime newNextFireTime = MonotonicTime::now() + interval; >- if (newNextFireTime < oldNextFireTime) { >- m_restartFireTime = MonotonicTime(); >- TimerBase::setNextFireTime(newNextFireTime); >- return; >- } >- >- m_restartFireTime = newNextFireTime; >-} >- >-void DeferrableOneShotTimer::stop() >-{ >- TimerBase::stop(); >-} >- >-void DeferrableOneShotTimer::fired() >-{ >- if (m_restartFireTime) { >- MonotonicTime now = MonotonicTime::now(); >- MonotonicTime restartFireTime = m_restartFireTime; >- m_restartFireTime = MonotonicTime(); >- if (now < restartFireTime) { >- TimerBase::setNextFireTime(restartFireTime); >- return; >- } >- } >- >- m_function(); >-} >- > } // namespace WebCore > >diff --git a/Source/WebCore/platform/Timer.h b/Source/WebCore/platform/Timer.h >index 728db091f0a6c4c34782b5f9fa4b944ca17f6b4c..6601675fd6d9c288d6ea1a493e5eeae8fbc2be9b 100644 >--- a/Source/WebCore/platform/Timer.h >+++ b/Source/WebCore/platform/Timer.h >@@ -68,15 +68,6 @@ public: > > static void fireTimersInNestedEventLoop(); > >-protected: >- MonotonicTime nextFireTime() const { return m_heapItem ? m_heapItem->time : MonotonicTime { }; } >- void setNextFireTime(MonotonicTime); >- >- void assertThreadSafety() >- { >- ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); >- } >- > private: > virtual void fired() = 0; > >@@ -85,6 +76,8 @@ private: > void checkConsistency() const; > void checkHeapIndex() const; > >+ void setNextFireTime(MonotonicTime); >+ > bool inHeap() const { return m_heapItem && m_heapItem->isInHeap(); } > > bool hasValidHeapPosition() const; >@@ -99,6 +92,8 @@ private: > void heapPopMin(); > static void heapDeleteNullMin(ThreadTimerHeap&); > >+ MonotonicTime nextFireTime() const { return m_heapItem ? m_heapItem->time : MonotonicTime { }; } >+ > MonotonicTime m_unalignedNextFireTime; // m_nextFireTime not considering alignment interval > Seconds m_repeatInterval; // 0 if not repeating > bool m_wasDeleted { false }; >@@ -146,29 +141,16 @@ inline bool TimerBase::isActive() const > return static_cast<bool>(nextFireTime()); > } > >-// ResettableOneShotTimer is a optimization for timers that need to be delayed frequently. >-// >-// Changing the deadline of a timer is not a cheap operation. When it is done frequently, it can >-// affect performance. >-// >-// With ResettableOneShotTimer, calling restart() does not change the underlying timer. >-// Instead, when the underlying timer fires, a new dealine is set for "delay" seconds. >-// >-// When a ResettableOneShotTimer of 5 seconds is restarted before the deadline, the total >-// time before the function is called is 10 seconds. >-// >-// If a timer is unfrequently reset, or if it is usually stopped before the deadline, >-// prefer WebCore::Timer to avoid idle wake-up. >-class ResettableOneShotTimer : protected TimerBase { >+class DeferrableOneShotTimer : protected TimerBase { > WTF_MAKE_FAST_ALLOCATED; > public: > template<typename TimerFiredClass> >- ResettableOneShotTimer(TimerFiredClass& object, void (TimerFiredClass::*function)(), Seconds delay) >- : ResettableOneShotTimer(std::bind(function, &object), delay) >+ DeferrableOneShotTimer(TimerFiredClass& object, void (TimerFiredClass::*function)(), Seconds delay) >+ : DeferrableOneShotTimer(std::bind(function, &object), delay) > { > } > >- ResettableOneShotTimer(WTF::Function<void()>&& function, Seconds delay) >+ DeferrableOneShotTimer(WTF::Function<void ()>&& function, Seconds delay) > : m_function(WTFMove(function)) > , m_delay(delay) > , m_shouldRestartWhenTimerFires(false) >@@ -214,30 +196,4 @@ private: > bool m_shouldRestartWhenTimerFires; > }; > >-// DeferrableOneShotTimer is a optimization for timers that need to change the deadline frequently. >-// >-// With DeferrableOneShotTimer, if the new deadline is later than the original deadline, the timer >-// is not reset. Instead, the timer fires and schedule a new timer for the remaining time. >-// >-// DeferrableOneShotTimer supports absolute deadlines. >-// If a timer of 5 seconds is restarted after 2 seconds, the total time will be 7 seconds. >-// >-// Restarting a DeferrableOneShotTimer is more expensive than restarting a ResettableOneShotTimer. >-// If accumulating the delay is not important, prefer ResettableOneShotTimer. >-class WEBCORE_EXPORT DeferrableOneShotTimer : private TimerBase { >- WTF_MAKE_FAST_ALLOCATED; >-public: >- DeferrableOneShotTimer(WTF::Function<void()>&&); >- ~DeferrableOneShotTimer(); >- >- void startOneShot(Seconds interval); >- void stop(); >- >-private: >- void fired() override; >- >- WTF::Function<void()> m_function; >- MonotonicTime m_restartFireTime; >-}; >- > } >diff --git a/Source/WebCore/platform/graphics/ca/TileController.h b/Source/WebCore/platform/graphics/ca/TileController.h >index 4b93add4b597ac2d211e8a8295728543b8b00142..5797e8644080af1f064d06d3c8ae68718920d173 100644 >--- a/Source/WebCore/platform/graphics/ca/TileController.h >+++ b/Source/WebCore/platform/graphics/ca/TileController.h >@@ -207,7 +207,7 @@ private: > IntRect m_boundsAtLastRevalidate; > > Timer m_tileRevalidationTimer; >- ResettableOneShotTimer m_tileSizeChangeTimer; >+ DeferrableOneShotTimer m_tileSizeChangeTimer; > > TileCoverage m_tileCoverage { CoverageForVisibleArea }; > >diff --git a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h >index d0a1a02c94ae6a595b9f8e271926aaf120baedf2..1a461ca13d9f079d499800127059f9bd0b59703d 100644 >--- a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h >+++ b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h >@@ -94,7 +94,7 @@ private: > > HashCountedSet<CGImageRef> m_images; > SubimageCacheHashSet m_cache; >- ResettableOneShotTimer m_timer; >+ DeferrableOneShotTimer m_timer; > > static SubimageCacheWithTimer& subimageCache(); > static bool subimageCacheExists(); >diff --git a/Source/WebKit/NetworkProcess/watchos/NetworkProximityAssertion.h b/Source/WebKit/NetworkProcess/watchos/NetworkProximityAssertion.h >index caa55fc9d3bc582439bac02ced7f68fb26f3758e..91107124f506aa7950540ba1e9ce970213816f09 100644 >--- a/Source/WebKit/NetworkProcess/watchos/NetworkProximityAssertion.h >+++ b/Source/WebKit/NetworkProcess/watchos/NetworkProximityAssertion.h >@@ -87,8 +87,8 @@ private: > > uint64_t m_assertionCount { 0 }; > State m_state { State::Suspended }; >- WebCore::ResettableOneShotTimer m_releaseTimer; >- WebCore::ResettableOneShotTimer m_suspendAfterBackgroundingTimer; >+ WebCore::DeferrableOneShotTimer m_releaseTimer; >+ WebCore::DeferrableOneShotTimer m_suspendAfterBackgroundingTimer; > }; > > class BluetoothProximityAssertion final : public NetworkProximityAssertion { >diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp >index 4a29daa9217597d303a00bb90c4630a2286b836f..9ae5fd1c71103949716305c9d92bbdf524675f13 100644 >--- a/Source/WebKit/UIProcess/ResponsivenessTimer.cpp >+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.cpp >@@ -32,32 +32,27 @@ static const Seconds responsivenessTimeout { 3_s }; > > ResponsivenessTimer::ResponsivenessTimer(ResponsivenessTimer::Client& client) > : m_client(client) >- , m_timer(std::bind(&ResponsivenessTimer::timerFired, this)) >+ , m_isResponsive(true) >+ , m_timer(RunLoop::main(), this, &ResponsivenessTimer::timerFired) > { > } > >-ResponsivenessTimer::~ResponsivenessTimer() = default; >+ResponsivenessTimer::~ResponsivenessTimer() >+{ >+ m_timer.stop(); >+} > > 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; > > if (!m_client.mayBecomeUnresponsive()) { >- m_waitingForTimer = true; > m_timer.startOneShot(responsivenessTimeout); > return; > } >@@ -71,22 +66,12 @@ void ResponsivenessTimer::timerFired() > > void ResponsivenessTimer::start() > { >- if (m_waitingForTimer) >+ if (m_timer.isActive()) > 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) { >@@ -98,19 +83,13 @@ void ResponsivenessTimer::stop() > m_client.didBecomeResponsive(); > } > >- m_waitingForTimer = false; >- >- if (m_useLazyStop) >- m_useLazyStop = false; >- else >- m_timer.stop(); >+ m_timer.stop(); > } > > void ResponsivenessTimer::processTerminated() > { > // Since there is no web process, we must not be waiting for it anymore. >- m_waitingForTimer = false; >- m_timer.stop(); >+ stop(); > } > > } // namespace WebKit >diff --git a/Source/WebKit/UIProcess/ResponsivenessTimer.h b/Source/WebKit/UIProcess/ResponsivenessTimer.h >index 2a7095d550e68c8195c4edecf65a95abdedf4f22..7dd7dbbf5b3509b978a482a11016e9607a1369ab 100644 >--- a/Source/WebKit/UIProcess/ResponsivenessTimer.h >+++ b/Source/WebKit/UIProcess/ResponsivenessTimer.h >@@ -26,7 +26,7 @@ > #ifndef ResponsivenessTimer_h > #define ResponsivenessTimer_h > >-#include <WebCore/Timer.h> >+#include <wtf/RunLoop.h> > > namespace WebKit { > >@@ -48,17 +48,6 @@ 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(); >@@ -71,10 +60,9 @@ private: > void timerFired(); > > ResponsivenessTimer::Client& m_client; >- WebCore::DeferrableOneShotTimer m_timer; >- bool m_isResponsive { true }; >- bool m_waitingForTimer { false }; >- bool m_useLazyStop { false }; >+ bool m_isResponsive; >+ >+ RunLoop::Timer<ResponsivenessTimer> m_timer; > }; > > } // namespace WebKit >diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp >index 7ab56f111e61149a6f1429f54a0d3b2aba748b37..f399f0ef01ee4e8e601ecfda2e1e5321c2c226d0 100644 >--- a/Source/WebKit/UIProcess/WebPageProxy.cpp >+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp >@@ -2357,7 +2357,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->isResponsiveWithLazyStop(); >+ m_process->isResponsive(nullptr); > } > > bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event) const >diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp >index b2da3bb8765e166f972e596de1e37ef6bed39f72..b769d53206ebde70c19279ce250396715db4e377 100644 >--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp >+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp >@@ -1120,15 +1120,6 @@ 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 8df5c25344c23f031d1d1ba05f3962aaa85fc8b0..b1039ecb8af6cce514ef5a4c514015f8e55bf4c1 100644 >--- a/Source/WebKit/UIProcess/WebProcessProxy.h >+++ b/Source/WebKit/UIProcess/WebProcessProxy.h >@@ -206,7 +206,6 @@ public: > ProcessThrottler& throttler() { return m_throttler; } > > void isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&&); >- void isResponsiveWithLazyStop(); > void didReceiveMainThreadPing(); > void didReceiveBackgroundResponsivenessPing(); > >diff --git a/Source/WebKit/WebProcess/Plugins/PluginView.h b/Source/WebKit/WebProcess/Plugins/PluginView.h >index de0994182f4c406218187e54d1a28896840750e2..84d4264ce0d06886c399c85648d25a9ca1eb8166 100644 >--- a/Source/WebKit/WebProcess/Plugins/PluginView.h >+++ b/Source/WebKit/WebProcess/Plugins/PluginView.h >@@ -284,7 +284,7 @@ private: > // This snapshot is used to avoid side effects should the plugin run JS during painting. > RefPtr<ShareableBitmap> m_transientPaintingSnapshot; > // This timer is used when plugin snapshotting is enabled, to capture a plugin placeholder. >- WebCore::ResettableOneShotTimer m_pluginSnapshotTimer; >+ WebCore::DeferrableOneShotTimer m_pluginSnapshotTimer; > #if ENABLE(PRIMARY_SNAPSHOTTED_PLUGIN_HEURISTIC) || PLATFORM(COCOA) > unsigned m_countSnapshotRetries { 0 }; > #endif
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=361354">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194003
:
360549
|
360676
|
361354
|
361766
|
361773
|
361777