Bug 1456115 - Re-serialize inbound NotifyPull. r?jya draft
authorPaul Adenot <paul@paul.cx>
Thu, 12 Apr 2018 14:23:03 +0200
changeset 787247 960ca31f6b6fb2738d54f6683431484fb976e12d
parent 787246 910254a18841b98287e6f8986601279f4e2f945d
child 787248 990a600b616bfb730be60d8af1154f4303d2597a
push id107695
push userpaul@paul.cx
push dateTue, 24 Apr 2018 16:04:50 +0000
reviewersjya
bugs1456115
milestone61.0a1
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
dom/media/MediaStreamGraph.cpp
dom/media/MediaStreamGraph.h
dom/media/MediaStreamGraphImpl.h
dom/media/MediaStreamListener.h
media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
--- a/dom/media/MediaStreamGraph.cpp
+++ b/dom/media/MediaStreamGraph.cpp
@@ -1154,31 +1154,19 @@ MediaStreamGraphImpl::UpdateGraph(GraphT
   // means the driver would have been blocking indefinitly, but the graph has
   // been woken up right after having been to sleep.
   MOZ_ASSERT(aEndBlockingDecisions >= mStateComputedTime);
 
   UpdateStreamOrder();
 
   bool ensureNextIteration = false;
 
-  // Grab pending stream input and compute blocking time
-  AutoTArray<RefPtr<SourceMediaStream::NotifyPullPromise>, 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) {
       // The stream's not suspended, and since it's finished, underruns won't
       // stop it playing out. So there's no blocking other than what we impose
       // here.
       GraphTime endTime = stream->GetStreamTracks().GetAllTracksEnd() +
           stream->mTracksStartTime;
@@ -1485,20 +1473,16 @@ public:
     // objects owning streams, or for expiration of mGraph->mShutdownTimer,
     // which won't otherwise release its reference on the graph until
     // nsTimerImpl::Shutdown(), which runs after xpcom-shutdown-threads.
     {
       MonitorAutoLock mon(mGraph->mMonitor);
       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) {
       MOZ_ASSERT(false,
         "AudioCallbackDriver took too long to shut down and we let shutdown"
         " continue - freezing and leaking");
 
       // The timer fired, so we may be deeper in shutdown now.  Block any further
@@ -2768,19 +2752,17 @@ SourceMediaStream::SetPullEnabled(bool a
     }
     SourceMediaStream* mStream;
     bool mEnabled;
   };
   GraphImpl()->AppendMessage(MakeUnique<Message>(this, aEnabled));
 }
 
 bool
-SourceMediaStream::PullNewData(
-  StreamTime aDesiredUpToTime,
-  nsTArray<RefPtr<SourceMediaStream::NotifyPullPromise>>& aPromises)
+SourceMediaStream::PullNewData(StreamTime aDesiredUpToTime)
 {
   TRACE_AUDIO_CALLBACK();
   MutexAutoLock lock(mMutex);
   if (!mPullEnabled || mFinished) {
     return false;
   }
   // Compute how much stream time we'll need assuming we don't block
   // the stream at all.
@@ -2793,17 +2775,17 @@ SourceMediaStream::PullNewData(
         GraphImpl()->MediaTimeToSeconds(current)));
   if (t <= current) {
     return false;
   }
   for (uint32_t j = 0; j < mListeners.Length(); ++j) {
     MediaStreamListener* l = mListeners[j];
     {
       MutexAutoUnlock unlock(mMutex);
-      aPromises.AppendElement(l->AsyncNotifyPull(GraphImpl(), t));
+      l->NotifyPull(GraphImpl(), t);
     }
   }
   return true;
 }
 
 void
 SourceMediaStream::ExtractPendingInput()
 {
@@ -3608,17 +3590,16 @@ MediaStreamGraphImpl::MediaStreamGraphIm
   , mPostedRunInStableStateEvent(false)
   , mDetectedNotRunning(false)
   , mPostedRunInStableState(false)
   , mRealtime(aDriverRequested != OFFLINE_THREAD_DRIVER)
   , mNonRealtimeProcessing(false)
   , mStreamOrderDirty(false)
   , mLatencyLog(AsyncLatencyLogger::Get())
   , mAbstractMainThread(aMainThread)
-  , mThreadPool(GetMediaThreadPool(MediaThreadType::MSG_CONTROL))
   , mSelfRef(this)
   , mOutputChannels(std::min<uint32_t>(8, CubebUtils::MaxNumberOfChannels()))
 #ifdef DEBUG
   , mCanRunMessagesSynchronously(false)
 #endif
 {
   if (mRealtime) {
     if (aDriverRequested == AUDIO_THREAD_DRIVER) {
--- a/dom/media/MediaStreamGraph.h
+++ b/dom/media/MediaStreamGraph.h
@@ -7,17 +7,16 @@
 #define MOZILLA_MEDIASTREAMGRAPH_H_
 
 #include "AudioStream.h"
 #include "MainThreadUtils.h"
 #include "MediaStreamTypes.h"
 #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"
 #include "nsAutoRef.h"
 #include "nsIRunnable.h"
 #include "nsTArray.h"
 #include <speex/speex_resampler.h>
 
@@ -712,23 +711,20 @@ public:
   // MediaStreamGraph thread only
   void DestroyImpl() override;
 
   // Call these on any thread.
   /**
    * 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<bool, bool, true /* is exclusive */ > NotifyPullPromise;
-  bool PullNewData(StreamTime aDesiredUpToTime,
-                   nsTArray<RefPtr<NotifyPullPromise>>& aPromises);
+  bool PullNewData(StreamTime aDesiredUpToTime);
 
   /**
    * Extract any state updates pending in the stream, and apply them.
    */
   void ExtractPendingInput();
 
   /**
    * These add/remove DirectListeners, which allow bypassing the graph and any
--- a/dom/media/MediaStreamGraphImpl.h
+++ b/dom/media/MediaStreamGraphImpl.h
@@ -814,17 +814,16 @@ public:
    */
   bool mStreamOrderDirty;
   /**
    * Hold a ref to the Latency logger
    */
   RefPtr<AsyncLatencyLogger> mLatencyLog;
   AudioMixer mMixer;
   const RefPtr<AbstractThread> mAbstractMainThread;
-  RefPtr<SharedThreadPool> mThreadPool;
 
   // used to limit graph shutdown time
   // Only accessed on the main thread.
   nsCOMPtr<nsITimer> mShutdownTimer;
 
 private:
   virtual ~MediaStreamGraphImpl();
 
--- a/dom/media/MediaStreamListener.h
+++ b/dom/media/MediaStreamListener.h
@@ -56,24 +56,16 @@ public:
    * calls to add or remove MediaStreamListeners. It is not allowed to block
    * for any length of time.
    * aDesiredTime is the stream time we would like to get data up to. Data
    * beyond this point will not be played until NotifyPull runs again, so there's
    * not much point in providing it. Note that if the stream is blocked for
    * some reason, then data before aDesiredTime may not be played immediately.
    */
   virtual void NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime) {}
-  virtual RefPtr<SourceMediaStream::NotifyPullPromise> AsyncNotifyPull(
-    MediaStreamGraph* aGraph,
-    StreamTime aDesiredTime)
-  {
-    NotifyPull(aGraph, aDesiredTime);
-    return SourceMediaStream::NotifyPullPromise::CreateAndResolve(true,
-                                                                  __func__);
-  }
 
   enum Blocking {
     BLOCKED,
     UNBLOCKED
   };
   /**
    * Notify that the blocking status of the stream changed. The initial state
    * is assumed to be BLOCKED.
--- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -2211,28 +2211,16 @@ public:
 
   // Implement MediaStreamListener
   void NotifyPull(MediaStreamGraph* aGraph,
                   StreamTime aDesiredTime) override
   {
     NotifyPullImpl(aDesiredTime);
   }
 
-  RefPtr<SourceMediaStream::NotifyPullPromise> AsyncNotifyPull(
-    MediaStreamGraph* aGraph,
-    StreamTime aDesiredTime) override
-  {
-    RefPtr<PipelineListener> self = this;
-    return InvokeAsync(mTaskQueue, __func__, [self, aDesiredTime]() {
-      self->NotifyPullImpl(aDesiredTime);
-      return SourceMediaStream::NotifyPullPromise::CreateAndResolve(true,
-                                                                    __func__);
-    });
-  }
-
 private:
   ~PipelineListener()
   {
     NS_ReleaseOnMainThreadSystemGroup("MediaPipeline::mConduit",
                                       mConduit.forget());
   }
 
   void NotifyPullImpl(StreamTime aDesiredTime)