This is one of the most expensive thing we do from the audio callback: dispatching to a thread that is very active is very expensive, because a lock is taken for each call to Dispatch, and then it might even be calling into the OS, which takes even more locks. We're dispatching way to much to the main thread (this is ridiculously obvious in the profiles). I'm thinking, as a stop gap measure, to create another thread alongside the MSG rendering thread (audio callback / system clock driver), so that dispatching is very cheap (using a lock free queue), and then we can simply do the real dispatch to the main thread (for example, it's a good example of a thread that is busy and that we dispatch too much too) later. See attached picture, especially the right panel, that is very very sad.

fyi

Why do we need to dispatch to main thread at all from the audio callback?

A bunch of things, notably updating the main thread times and also invalidating pictures for layout or something. We do use async video though, so maybe it's possible to not do that. Matt, idea here? What is the purpose of [0]? Could we not do it? [0]: https://searchfox.org/mozilla-central/source/dom/media/VideoFrameContainer.cpp#197

(In reply to Paul Adenot (:padenot) from comment #4) > A bunch of things, notably updating the main thread times and also > invalidating pictures for layout or something. We do use async video though, > so maybe it's possible to not do that. > > Matt, idea here? What is the purpose of [0]? Could we not do it? > > [0]: > https://searchfox.org/mozilla-central/source/dom/media/VideoFrameContainer. > cpp#197 Why are we updating the VideoFrameContainer from the audio callback? That call is there because we don't always know (asychronously) if we actually have async video, and we sometimes need to update things on the main thread even for an async video change. Invalidating rendering observers is one example of the latter: https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#7282 I don't really see a reason we can't revert the logic though, and have the main thread be responsible for notifying the video thread when we transition in/out of being able to ignore async video updates. Just a bit of fiddly work to accumulate the state, and make sure we don't lose frames during the transition.

The attached patch helps significantly but it's not good enough, I've cut some corners in MediaStreamVideoSink::NotifyRealtimeTrackData. Profile looks good though.

Comment on attachment 8958151 [details] Bug 1443886 - Bounce dispatches to the main thread on another thread. f?pehrsons apparently f? does not work in mozreview...

Comment on attachment 8958151 [details] Bug 1443886 - Bounce dispatches to the main thread on another thread. f?pehrsons https://reviewboard.mozilla.org/r/227098/#review236058 ::: dom/media/MediaStreamGraph.cpp:1834 (Diff revision 2) > mCurrentTaskMessageQueue.AppendElement(Move(aMessage)); > EnsureRunInStableState(); > } > > void > MediaStreamGraphImpl::Dispatch(already_AddRefed<nsIRunnable>&& aRunnable) What's the reason we at all need this main thread dispatch instead of just DispatchToMainThreadAfterStreamStateUpdate? The latter will dispatch to main thread at most once per iteration so that would coalesce all the expensive dispatches into one if used everywhere. ::: dom/media/MediaStreamVideoSink.cpp:18 (Diff revision 2) > + VideoSegment copy; > + copy.AppendSlice(aMedia, 0, aMedia.GetDuration()); > + static_cast<MediaStreamGraphImpl*>(aGraph)->Dispatch( > + NewRunnableMethod<StoreCopyPassByRRef<VideoSegment>>( > + "MediaStreamVideoSink::SetCurrentFrame", > + this, > + &MediaStreamVideoSink::SetCurrentFrames, > + Move(copy))); Can we make this bounce conditional on if we're on the graph thread? It's not always on the graph thread, and long term it will never be on the graph thread. Also, certain MediaStreamVideoSink::SetCurrentFrames paths don't touch main thread but immediately dispatch to other threads, like MediaPipelineTransmit::PipelineListener [1] and MediaEncoder::VideoTrackListener [2]. If we could shrink this to only cover HTMLMediaElement/VideoFrameContainer that would be good. [1] https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#2027 [2] https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/media/encoder/MediaEncoder.cpp#305

Comment on attachment 8958151 [details] Bug 1443886 - Bounce dispatches to the main thread on another thread. f?pehrsons Overall I'm fine with this as a short term remedy. See the mozreview comments for some more high-level comments on specific parts.

(In reply to Andreas Pehrson [:pehrsons] from comment #10) > Comment on attachment 8958151 [details] > Bug 1443886 - Bounce dispatches to the main thread on another thread. > f?pehrsons > > https://reviewboard.mozilla.org/r/227098/#review236058 > > ::: dom/media/MediaStreamGraph.cpp:1834 > (Diff revision 2) > > mCurrentTaskMessageQueue.AppendElement(Move(aMessage)); > > EnsureRunInStableState(); > > } > > > > void > > MediaStreamGraphImpl::Dispatch(already_AddRefed<nsIRunnable>&& aRunnable) > > What's the reason we at all need this main thread dispatch instead of just > DispatchToMainThreadAfterStreamStateUpdate? > > The latter will dispatch to main thread at most once per iteration so that > would coalesce all the expensive dispatches into one if used everywhere. This bug is about removing main thread dispatch, because they are expensive indeed, but more importantly, not predictable. The goal is to remove all of them. > > ::: dom/media/MediaStreamVideoSink.cpp:18 > (Diff revision 2) > > + VideoSegment copy; > > + copy.AppendSlice(aMedia, 0, aMedia.GetDuration()); > > + static_cast<MediaStreamGraphImpl*>(aGraph)->Dispatch( > > + NewRunnableMethod<StoreCopyPassByRRef<VideoSegment>>( > > + "MediaStreamVideoSink::SetCurrentFrame", > > + this, > > + &MediaStreamVideoSink::SetCurrentFrames, > > + Move(copy))); > > Can we make this bounce conditional on if we're on the graph thread? Yes good idea. > It's not always on the graph thread, and long term it will never be on the > graph thread. > > Also, certain MediaStreamVideoSink::SetCurrentFrames paths don't touch main > thread but immediately dispatch to other threads, like > MediaPipelineTransmit::PipelineListener [1] and > MediaEncoder::VideoTrackListener [2]. This is not going to happen in the short term apparently apparently. > If we could shrink this to only cover HTMLMediaElement/VideoFrameContainer > that would be good. Yeah it's all hidden behind this MediaStreamVideoSink thing, unclear how to do that.