From f412eb58aeb9fe5c493edfe1eabc6c0956d0c882 Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Thu, 12 Apr 2018 14:23:03 +0200 Subject: [PATCH] Bug 1456115 - Re-serialize inbound NotifyPull. r=jya We made NotifyPull parallel to try to lower the load, and we initially measured an improvement. However, we did the measurements with a profiler that did an aggregation of the results. Our results had an high variance, so the mean load was in fact not meaningful. More careful measurement performed without doing any aggregation show that, under load, relying on the fact that the scheduler schedules the tasks on time is too risky, and that the code is fast enough to not have to parallelize. MozReview-Commit-ID: CMhSn8Sc0OO --HG-- extra : rebase_source : cfb41f861089bce9e10446bee81c13f8565ba90e --- dom/media/MediaStreamGraph.cpp | 25 +++---------------- dom/media/MediaStreamGraph.h | 6 +---- dom/media/MediaStreamGraphImpl.h | 1 - dom/media/MediaStreamListener.h | 8 ------ .../src/mediapipeline/MediaPipeline.cpp | 12 --------- 5 files changed, 4 insertions(+), 48 deletions(-) diff --git a/dom/media/MediaStreamGraph.cpp b/dom/media/MediaStreamGraph.cpp index 4b575ccfe596..1c6683c96608 100644 --- a/dom/media/MediaStreamGraph.cpp +++ b/dom/media/MediaStreamGraph.cpp @@ -1159,21 +1159,9 @@ MediaStreamGraphImpl::UpdateGraph(GraphTime aEndBlockingDecisions) bool ensureNextIteration = false; - // Grab pending stream input and compute blocking time - AutoTArray, 64> promises; - for (MediaStream* stream : mStreams) { - if (SourceMediaStream* is = stream->AsSourceStream()) { - ensureNextIteration |= is->PullNewData(aEndBlockingDecisions, promises); - } - } - - // Wait until all PullEnabled stream's listeners have completed. - if (!promises.IsEmpty()) { - AwaitAll(do_AddRef(mThreadPool), promises); - } - for (MediaStream* stream : mStreams) { if (SourceMediaStream* is = stream->AsSourceStream()) { + ensureNextIteration |= is->PullNewData(aEndBlockingDecisions); is->ExtractPendingInput(); } if (stream->mFinished) { @@ -1490,10 +1478,6 @@ public: mGraph->SetCurrentDriver(nullptr); } - // Do not hold on our threadpool, global shutdown will hang otherwise as - // it waits for all thread pools to shutdown. - mGraph->mThreadPool = nullptr; - // Safe to access these without the monitor since the graph isn't running. // We may be one of several graphs. Drop ticket to eventually unblock shutdown. if (mGraph->mShutdownTimer && !mGraph->mForceShutdownTicket) { @@ -2773,9 +2757,7 @@ SourceMediaStream::SetPullEnabled(bool aEnabled) } bool -SourceMediaStream::PullNewData( - StreamTime aDesiredUpToTime, - nsTArray>& aPromises) +SourceMediaStream::PullNewData(StreamTime aDesiredUpToTime) { TRACE_AUDIO_CALLBACK(); MutexAutoLock lock(mMutex); @@ -2798,7 +2780,7 @@ SourceMediaStream::PullNewData( MediaStreamListener* l = mListeners[j]; { MutexAutoUnlock unlock(mMutex); - aPromises.AppendElement(l->AsyncNotifyPull(GraphImpl(), t)); + l->NotifyPull(GraphImpl(), t); } } return true; @@ -3613,7 +3595,6 @@ MediaStreamGraphImpl::MediaStreamGraphImpl(GraphDriverType aDriverRequested, , mStreamOrderDirty(false) , mLatencyLog(AsyncLatencyLogger::Get()) , mAbstractMainThread(aMainThread) - , mThreadPool(GetMediaThreadPool(MediaThreadType::MSG_CONTROL)) , mSelfRef(this) , mOutputChannels(std::min(8, CubebUtils::MaxNumberOfChannels())) #ifdef DEBUG diff --git a/dom/media/MediaStreamGraph.h b/dom/media/MediaStreamGraph.h index 55e3684d0caf..a441a2eb493f 100644 --- a/dom/media/MediaStreamGraph.h +++ b/dom/media/MediaStreamGraph.h @@ -12,7 +12,6 @@ #include "StreamTracks.h" #include "VideoSegment.h" #include "mozilla/LinkedList.h" -#include "mozilla/MozPromise.h" #include "mozilla/Mutex.h" #include "mozilla/TaskQueue.h" #include "nsAutoPtr.h" @@ -717,13 +716,10 @@ public: * Call all MediaStreamListeners to request new data via the NotifyPull API * (if enabled). * aDesiredUpToTime (in): end time of new data requested. - * aPromises (out): NotifyPullPromises if async API is enabled. * * Returns true if new data is about to be added. */ - typedef MozPromise NotifyPullPromise; - bool PullNewData(StreamTime aDesiredUpToTime, - nsTArray>& aPromises); + bool PullNewData(StreamTime aDesiredUpToTime); /** * Extract any state updates pending in the stream, and apply them. diff --git a/dom/media/MediaStreamGraphImpl.h b/dom/media/MediaStreamGraphImpl.h index 7cf8cc999300..26cbe532faab 100644 --- a/dom/media/MediaStreamGraphImpl.h +++ b/dom/media/MediaStreamGraphImpl.h @@ -819,7 +819,6 @@ public: RefPtr mLatencyLog; AudioMixer mMixer; const RefPtr mAbstractMainThread; - RefPtr mThreadPool; // used to limit graph shutdown time // Only accessed on the main thread. diff --git a/dom/media/MediaStreamListener.h b/dom/media/MediaStreamListener.h index ccbc32588f78..af14b7717884 100644 --- a/dom/media/MediaStreamListener.h +++ b/dom/media/MediaStreamListener.h @@ -61,14 +61,6 @@ public: * some reason, then data before aDesiredTime may not be played immediately. */ virtual void NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime) {} - virtual RefPtr AsyncNotifyPull( - MediaStreamGraph* aGraph, - StreamTime aDesiredTime) - { - NotifyPull(aGraph, aDesiredTime); - return SourceMediaStream::NotifyPullPromise::CreateAndResolve(true, - __func__); - } enum Blocking { BLOCKED, diff --git a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp index 21a489a27fba..f5bbdff9882e 100644 --- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp +++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ -2216,18 +2216,6 @@ public: NotifyPullImpl(aDesiredTime); } - RefPtr AsyncNotifyPull( - MediaStreamGraph* aGraph, - StreamTime aDesiredTime) override - { - RefPtr self = this; - return InvokeAsync(mTaskQueue, __func__, [self, aDesiredTime]() { - self->NotifyPullImpl(aDesiredTime); - return SourceMediaStream::NotifyPullPromise::CreateAndResolve(true, - __func__); - }); - } - private: ~PipelineListener() {