From 1b790f32463a299f122d24b51d9a5649893828bb Mon Sep 17 00:00:00 2001 From: Olli Pettay Date: Tue, 5 Mar 2019 18:47:05 +0200 Subject: [PATCH] Bug 1522316, use medium high priority queue for worker->main thread control messages, r=baku If main thread is busy handling runnables in the normal priority queue, control-type of messages from workers are possibly postponed to run after those. That can lead to bad performance, if the page expects workers to be able to proceed simultanously with the main thread. This patch makes the control messages to use medium high priority queue in order to try to ensure they are handled in timely manner. Pref dom.worker.use_medium_high_event_queue can be set to false to disable this new behavior. Differential Revision: https://phabricator.services.mozilla.com/D22128 --HG-- extra : rebase_source : 447dec6dbcccaa0206a1815c21ccf713c523fc91 --- dom/console/Console.cpp | 2 + dom/serviceworkers/ServiceWorkerPrivate.cpp | 8 +++- dom/workers/WorkerDebugger.cpp | 6 ++- dom/workers/WorkerPrivate.cpp | 51 +++++++++++++++------ dom/workers/WorkerPrivate.h | 10 ++++ dom/workers/WorkerRunnable.cpp | 5 +- dom/workers/WorkerRunnable.h | 2 + dom/workers/WorkerScope.cpp | 4 +- modules/libpref/init/StaticPrefList.h | 6 +++ xpcom/threads/ThrottledEventQueue.cpp | 29 +++++++++--- xpcom/threads/ThrottledEventQueue.h | 3 +- 11 files changed, 99 insertions(+), 27 deletions(-) diff --git a/dom/console/Console.cpp b/dom/console/Console.cpp index 2f0cbf5b4d03..56b4be3c81b4 100644 --- a/dom/console/Console.cpp +++ b/dom/console/Console.cpp @@ -712,6 +712,8 @@ class ConsoleWorkerRunnable : public WorkerProxyToMainThreadRunnable, // This method is called in the owning thread of the Console object. virtual void ReleaseData() = 0; + bool ForMessaging() const override { return true; } + // This must be released on the worker thread. RefPtr mConsole; diff --git a/dom/serviceworkers/ServiceWorkerPrivate.cpp b/dom/serviceworkers/ServiceWorkerPrivate.cpp index 059f6a4bbfbf..3bcfee4de70b 100644 --- a/dom/serviceworkers/ServiceWorkerPrivate.cpp +++ b/dom/serviceworkers/ServiceWorkerPrivate.cpp @@ -639,7 +639,9 @@ class LifeCycleEventWatcher final : public ExtendableEventCallback { [self]() { self->ReportResult(false); }); if (NS_WARN_IF(!mWorkerRef)) { mCallback->SetResult(false); - nsresult rv = workerPrivate->DispatchToMainThread(mCallback); + // Using DispatchToMainThreadForMessaging so that state update on + // the main thread doesn't happen too soon. + nsresult rv = workerPrivate->DispatchToMainThreadForMessaging(mCallback); Unused << NS_WARN_IF(NS_FAILED(rv)); return false; } @@ -655,7 +657,9 @@ class LifeCycleEventWatcher final : public ExtendableEventCallback { } mCallback->SetResult(aResult); - nsresult rv = mWorkerRef->Private()->DispatchToMainThread(mCallback); + // Using DispatchToMainThreadForMessaging so that state update on + // the main thread doesn't happen too soon. + nsresult rv = mWorkerRef->Private()->DispatchToMainThreadForMessaging(mCallback); if (NS_WARN_IF(NS_FAILED(rv))) { MOZ_CRASH("Failed to dispatch life cycle event handler."); } diff --git a/dom/workers/WorkerDebugger.cpp b/dom/workers/WorkerDebugger.cpp index 720587db6888..0d7d7edb3c0f 100644 --- a/dom/workers/WorkerDebugger.cpp +++ b/dom/workers/WorkerDebugger.cpp @@ -400,7 +400,8 @@ void WorkerDebugger::PostMessageToDebugger(const nsAString& aMessage) { RefPtr runnable = new PostDebuggerMessageRunnable(this, aMessage); - if (NS_FAILED(mWorkerPrivate->DispatchToMainThread(runnable.forget()))) { + if (NS_FAILED(mWorkerPrivate->DispatchToMainThreadForMessaging( + runnable.forget()))) { NS_WARNING("Failed to post message to debugger on main thread!"); } } @@ -422,7 +423,8 @@ void WorkerDebugger::ReportErrorToDebugger(const nsAString& aFilename, RefPtr runnable = new ReportDebuggerErrorRunnable(this, aFilename, aLineno, aMessage); - if (NS_FAILED(mWorkerPrivate->DispatchToMainThread(runnable.forget()))) { + if (NS_FAILED(mWorkerPrivate->DispatchToMainThreadForMessaging( + runnable.forget()))) { NS_WARNING("Failed to report error to debugger on main thread!"); } } diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index 7be78f405984..c032659417ab 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/dom/workers/WorkerPrivate.cpp @@ -1032,8 +1032,8 @@ class WorkerPrivate::MemoryReporter final : public nsIMemoryReporter { WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); MOZ_ASSERT(workerPrivate); - MOZ_ALWAYS_SUCCEEDS( - workerPrivate->DispatchToMainThread(mFinishCollectRunnable.forget())); + MOZ_ALWAYS_SUCCEEDS(workerPrivate->DispatchToMainThreadForMessaging( + mFinishCollectRunnable.forget())); } }; @@ -1337,9 +1337,9 @@ nsresult WorkerPrivate::Dispatch(already_AddRefed aRunnable, return DispatchLockHeld(std::move(aRunnable), aSyncLoopTarget, lock); } -nsresult WorkerPrivate::DispatchLockHeld(already_AddRefed aRunnable, - nsIEventTarget* aSyncLoopTarget, - const MutexAutoLock& aProofOfLock) { +nsresult WorkerPrivate::DispatchLockHeld( + already_AddRefed aRunnable, nsIEventTarget* aSyncLoopTarget, + const MutexAutoLock& aProofOfLock) { // May be called on any thread! RefPtr runnable(aRunnable); @@ -1378,8 +1378,7 @@ nsresult WorkerPrivate::DispatchLockHeld(already_AddRefed aRunna // they should not be delivered to a frozen worker. But frozen workers // aren't drawing from the thread's main event queue anyway, only from // mControlQueue. - rv = mThread->DispatchAnyThread(WorkerThreadFriendKey(), - runnable.forget()); + rv = mThread->DispatchAnyThread(WorkerThreadFriendKey(), runnable.forget()); } if (NS_WARN_IF(NS_FAILED(rv))) { @@ -2125,6 +2124,8 @@ WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent, // that ThrottledEventQueue can only be created on the main thread at the // moment. if (aParent) { + mMainThreadEventTargetForMessaging = + aParent->mMainThreadEventTargetForMessaging; mMainThreadEventTarget = aParent->mMainThreadEventTarget; mMainThreadDebuggeeEventTarget = aParent->mMainThreadDebuggeeEventTarget; return; @@ -2141,7 +2142,14 @@ WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent, // Throttle events to the main thread using a ThrottledEventQueue specific to // this tree of worker threads. - mMainThreadEventTarget = ThrottledEventQueue::Create(target); + mMainThreadEventTargetForMessaging = ThrottledEventQueue::Create(target); + if (StaticPrefs::dom_worker_use_medium_high_event_queue()) { + mMainThreadEventTarget = + ThrottledEventQueue::Create(GetMainThreadSerialEventTarget(), + nsIRunnablePriority::PRIORITY_MEDIUMHIGH); + } else { + mMainThreadEventTarget = mMainThreadEventTargetForMessaging; + } mMainThreadDebuggeeEventTarget = ThrottledEventQueue::Create(target); if (IsParentWindowPaused() || IsFrozen()) { MOZ_ALWAYS_SUCCEEDS(mMainThreadDebuggeeEventTarget->SetIsPaused(true)); @@ -2254,9 +2262,7 @@ already_AddRefed WorkerPrivate::Constructor( return worker.forget(); } -nsresult -WorkerPrivate::SetIsDebuggerReady(bool aReady) -{ +nsresult WorkerPrivate::SetIsDebuggerReady(bool aReady) { AssertIsOnParentThread(); MutexAutoLock lock(mMutex); @@ -2750,7 +2756,7 @@ void WorkerPrivate::DoRunLoop(JSContext* aCx) { // If the worker thread is spamming the main thread faster than it can // process the work, then pause the worker thread until the main thread // catches up. - size_t queuedEvents = mMainThreadEventTarget->Length() + + size_t queuedEvents = mMainThreadEventTargetForMessaging->Length() + mMainThreadDebuggeeEventTarget->Length(); if (queuedEvents > 5000) { // Note, postMessage uses mMainThreadDebuggeeEventTarget! @@ -2783,6 +2789,22 @@ void WorkerPrivate::AfterProcessNextEvent() { MOZ_ASSERT(CycleCollectedJSContext::Get()->RecursionDepth()); } +nsIEventTarget* WorkerPrivate::MainThreadEventTargetForMessaging() { + return mMainThreadEventTargetForMessaging; +} + +nsresult WorkerPrivate::DispatchToMainThreadForMessaging(nsIRunnable* aRunnable, + uint32_t aFlags) { + nsCOMPtr r = aRunnable; + return DispatchToMainThreadForMessaging(r.forget(), aFlags); +} + +nsresult WorkerPrivate::DispatchToMainThreadForMessaging( + already_AddRefed aRunnable, uint32_t aFlags) { + return mMainThreadEventTargetForMessaging->Dispatch(std::move(aRunnable), + aFlags); +} + nsIEventTarget* WorkerPrivate::MainThreadEventTarget() { return mMainThreadEventTarget; } @@ -3148,9 +3170,12 @@ void WorkerPrivate::ScheduleDeletion(WorkerRanOrNot aRanOrNot) { NS_WARNING("Failed to dispatch runnable!"); } } else { + // Note, this uses the lower priority DispatchToMainThreadForMessaging for + // dispatching TopLevelWorkerFinishedRunnable to the main thread so that + // other relevant runnables are guaranteed to run before it. RefPtr runnable = new TopLevelWorkerFinishedRunnable(this); - if (NS_FAILED(DispatchToMainThread(runnable.forget()))) { + if (NS_FAILED(DispatchToMainThreadForMessaging(runnable.forget()))) { NS_WARNING("Failed to dispatch runnable!"); } } diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h index 14d6d8b971c3..f4e48ff5b613 100644 --- a/dom/workers/WorkerPrivate.h +++ b/dom/workers/WorkerPrivate.h @@ -401,6 +401,15 @@ class WorkerPrivate : public RelativeTimeline { // Get the event target to use when dispatching to the main thread // from this Worker thread. This may be the main thread itself or // a ThrottledEventQueue to the main thread. + nsIEventTarget* MainThreadEventTargetForMessaging(); + + nsresult DispatchToMainThreadForMessaging( + nsIRunnable* aRunnable, uint32_t aFlags = NS_DISPATCH_NORMAL); + + nsresult DispatchToMainThreadForMessaging( + already_AddRefed aRunnable, + uint32_t aFlags = NS_DISPATCH_NORMAL); + nsIEventTarget* MainThreadEventTarget(); nsresult DispatchToMainThread(nsIRunnable* aRunnable, @@ -973,6 +982,7 @@ class WorkerPrivate : public RelativeTimeline { PRThread* mPRThread; // Accessed from main thread + RefPtr mMainThreadEventTargetForMessaging; RefPtr mMainThreadEventTarget; // Accessed from worker thread and destructing thread diff --git a/dom/workers/WorkerRunnable.cpp b/dom/workers/WorkerRunnable.cpp index 3ec1beec58f8..740357896922 100644 --- a/dom/workers/WorkerRunnable.cpp +++ b/dom/workers/WorkerRunnable.cpp @@ -619,7 +619,10 @@ bool WorkerProxyToMainThreadRunnable::Dispatch(WorkerPrivate* aWorkerPrivate) { MOZ_ASSERT(!mWorkerRef); mWorkerRef = new ThreadSafeWorkerRef(workerRef); - if (NS_WARN_IF(NS_FAILED(aWorkerPrivate->DispatchToMainThread(this)))) { + if (ForMessaging() + ? NS_WARN_IF(NS_FAILED( + aWorkerPrivate->DispatchToMainThreadForMessaging(this))) + : NS_WARN_IF(NS_FAILED(aWorkerPrivate->DispatchToMainThread(this)))) { ReleaseWorker(); RunBackOnWorkerThreadForCleanup(aWorkerPrivate); return false; diff --git a/dom/workers/WorkerRunnable.h b/dom/workers/WorkerRunnable.h index 7aab562e2cae..786b8c6d2482 100644 --- a/dom/workers/WorkerRunnable.h +++ b/dom/workers/WorkerRunnable.h @@ -397,6 +397,8 @@ class WorkerProxyToMainThreadRunnable : public Runnable { public: bool Dispatch(WorkerPrivate* aWorkerPrivate); + virtual bool ForMessaging() const { return false; } + private: NS_IMETHOD Run() override; diff --git a/dom/workers/WorkerScope.cpp b/dom/workers/WorkerScope.cpp index 311298f09e2d..d66de6f8247c 100644 --- a/dom/workers/WorkerScope.cpp +++ b/dom/workers/WorkerScope.cpp @@ -661,7 +661,7 @@ void ServiceWorkerGlobalScope::SetOnfetch( if (aCallback) { if (mWorkerPrivate->WorkerScriptExecutedSuccessfully()) { RefPtr r = new ReportFetchListenerWarningRunnable(mScope); - mWorkerPrivate->DispatchToMainThread(r.forget()); + mWorkerPrivate->DispatchToMainThreadForMessaging(r.forget()); } mWorkerPrivate->SetFetchHandlerWasAdded(); } @@ -678,7 +678,7 @@ void ServiceWorkerGlobalScope::EventListenerAdded(nsAtom* aType) { if (mWorkerPrivate->WorkerScriptExecutedSuccessfully()) { RefPtr r = new ReportFetchListenerWarningRunnable(mScope); - mWorkerPrivate->DispatchToMainThread(r.forget()); + mWorkerPrivate->DispatchToMainThreadForMessaging(r.forget()); } mWorkerPrivate->SetFetchHandlerWasAdded(); diff --git a/modules/libpref/init/StaticPrefList.h b/modules/libpref/init/StaticPrefList.h index 64b1ff250615..da5b7e078455 100644 --- a/modules/libpref/init/StaticPrefList.h +++ b/modules/libpref/init/StaticPrefList.h @@ -469,6 +469,12 @@ VARCACHE_PREF( RelaxedAtomicUint32, 30000 /* 30 seconds */ ) +VARCACHE_PREF( + "dom.worker.use_medium_high_event_queue", + dom_worker_use_medium_high_event_queue, + RelaxedAtomicBool, true +) + // Enable content type normalization of XHR uploads via MIME Sniffing standard // Disabled for now in bz1499136 VARCACHE_PREF( diff --git a/xpcom/threads/ThrottledEventQueue.cpp b/xpcom/threads/ThrottledEventQueue.cpp index 0569c0e9fbf1..8105f84eef4c 100644 --- a/xpcom/threads/ThrottledEventQueue.cpp +++ b/xpcom/threads/ThrottledEventQueue.cpp @@ -62,21 +62,31 @@ class ThrottledEventQueue::Inner final : public nsISupports { // The runnable which is dispatched to the underlying base target. Since // we only execute one event at a time we just re-use a single instance // of this class while there are events left in the queue. - class Executor final : public Runnable { + class Executor final : public Runnable, public nsIRunnablePriority { // The Inner whose runnables we execute. mInner->mExecutor points // to this executor, forming a reference loop. RefPtr mInner; + ~Executor() = default; + public: explicit Executor(Inner* aInner) : Runnable("ThrottledEventQueue::Inner::Executor"), mInner(aInner) {} + NS_DECL_ISUPPORTS_INHERITED + NS_IMETHODIMP Run() override { mInner->ExecuteRunnable(); return NS_OK; } + NS_IMETHODIMP + GetPriority(uint32_t* aPriority) override { + *aPriority = mInner->mPriority; + return NS_OK; + } + #ifdef MOZ_COLLECTING_RUNNABLE_TELEMETRY NS_IMETHODIMP GetName(nsACString& aName) override { return mInner->CurrentName(aName); } @@ -103,14 +113,17 @@ class ThrottledEventQueue::Inner final : public nsISupports { // Used from any thread; protected by mMutex. nsCOMPtr mExecutor; + const uint32_t mPriority; + // True if this queue is currently paused. // Used from any thread; protected by mMutex. bool mIsPaused; - explicit Inner(nsISerialEventTarget* aBaseTarget) + explicit Inner(nsISerialEventTarget* aBaseTarget, uint32_t aPriority) : mMutex("ThrottledEventQueue"), mIdleCondVar(mMutex, "ThrottledEventQueue:Idle"), mBaseTarget(aBaseTarget), + mPriority(aPriority), mIsPaused(false) {} ~Inner() { @@ -231,12 +244,13 @@ class ThrottledEventQueue::Inner final : public nsISupports { } public: - static already_AddRefed Create(nsISerialEventTarget* aBaseTarget) { + static already_AddRefed Create(nsISerialEventTarget* aBaseTarget, + uint32_t aPriority) { MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(ClearOnShutdown_Internal::sCurrentShutdownPhase == ShutdownPhase::NotInShutdown); - RefPtr ref = new Inner(aBaseTarget); + RefPtr ref = new Inner(aBaseTarget, aPriority); return ref.forget(); } @@ -332,6 +346,9 @@ class ThrottledEventQueue::Inner final : public nsISupports { NS_IMPL_ISUPPORTS(ThrottledEventQueue::Inner, nsISupports); +NS_IMPL_ISUPPORTS_INHERITED(ThrottledEventQueue::Inner::Executor, Runnable, + nsIRunnablePriority) + NS_IMPL_ISUPPORTS(ThrottledEventQueue, ThrottledEventQueue, nsIEventTarget, nsISerialEventTarget); @@ -341,11 +358,11 @@ ThrottledEventQueue::ThrottledEventQueue(already_AddRefed aInner) } already_AddRefed ThrottledEventQueue::Create( - nsISerialEventTarget* aBaseTarget) { + nsISerialEventTarget* aBaseTarget, uint32_t aPriority) { MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(aBaseTarget); - RefPtr inner = Inner::Create(aBaseTarget); + RefPtr inner = Inner::Create(aBaseTarget, aPriority); RefPtr ref = new ThrottledEventQueue(inner.forget()); return ref.forget(); diff --git a/xpcom/threads/ThrottledEventQueue.h b/xpcom/threads/ThrottledEventQueue.h index ebacc0478d42..2e6bb49282fa 100644 --- a/xpcom/threads/ThrottledEventQueue.h +++ b/xpcom/threads/ThrottledEventQueue.h @@ -68,7 +68,8 @@ class ThrottledEventQueue final : public nsISerialEventTarget { public: // Create a ThrottledEventQueue for the given target. static already_AddRefed Create( - nsISerialEventTarget* aBaseTarget); + nsISerialEventTarget* aBaseTarget, + uint32_t aPriority = nsIRunnablePriority::PRIORITY_NORMAL); // Determine if there are any events pending in the queue. bool IsEmpty() const;