Closed Bug 1314833 Opened 8 years ago Closed 8 years ago

Factor out AbstractThread::MainThread() so that we can pass in different values

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: billm, Assigned: bevis)

References

Details

Attachments

(6 files, 25 obsolete files)

Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. r=jwwang
13.12 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Part 1.2: Define AbstractThreadFor(TaskCategory aCategory) in Dispatcher and DispatcherTrait. r=billm
14.49 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Part 2.1: Factor out AbstractThread::MainThread() used in Media Playback. r=billm,jwwang
73.84 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Part 2.2: Use AbstractThread::CreateDirectTaskDrainer() to Drain Direct Tasks Dispatched to MediaStreamGraph. f=rjesup,r=padenot,jwwang
73.68 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Part 3: Factor out AbstractThread::MainThread() used in FlyWeb. r=billm
6.50 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Part 4: Factor out AbstractThread::MainThread() used in U2F. r=billm
22.47 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Attached patch incomplete patch (obsolete) — — Splinter Review
A while ago Bobby and I talked about how we can label runnables for media code. The plan we came up with is as follows. There are lots of places where we use AbstractThread::MainThread() to decide where a promise or a runnable should run. We want to replace these calls with an AbstractThread instance that's specific to a particular DocGroup. The first step is to allow us to pass in different values for the "main thread". When we create all the objects relevant for media decoding (like MediaDecoder), we'll pass an AbstractThread to their constructors. They will store this AbstractThread in a field and then use that field in place of AbstractThread::MainThread(). Once we've done this, HTMLMediaElement can create an AbstractThread that dispatches to its DocGroup and pass that in for the "main thread". I'm attaching a partial patch for the first part that hopefully gives an idea of what needs to be done.
Take this bug to follow up.
Assignee: nobody → btseng
Component: Audio/Video → Audio/Video: Playback
This WIP patch defines a new class called AbstractMainThread as a wrapper of AbstractThread::MainThread() and contains a DocGroup info for the labeling once labeling API in bug 1305926 is ready. A static method: AbstractMainThread::GetOrCreate(DocGroup*) is defined to get or create a AbstractMainThread instance belonging to the specified DocGroup to reduce the number of AbstractMainThread instances. (See HTMLMediaElement, MediaSource and MP4Decoder::IsVideoAccelerated() as examples.) Hi Bill, May I have your feedback to see if this new class meets your expectation? Thanks! Note: This patch is not comlete yet because there are following tasks to be resolved: 1. In some classes like MediaMemoryTracker, DecoderAllocPolicy are defined as singletons and there is no strong relationship to the a docgroup. 2. Instances like MediaStreamGraph::GetInstance() could be shared if the AudioChannel is the same. 3. New ordering problem in AutoTaskDispatcher comes after introducing this new wrapper class: - PerThreadTaskGroup is identified by the reference of a AbstractThread. See AutoTaskDispatcher::EnsureTaskGroup(DocGroup*). - With this new wrapper, tasks dispatched to main thread will be divided into multiple PerThreadTaskGroups if their docgroups are different which cause the order changed when dispatching these tasks in ~AutoTaskDispatcher(). 4. Not pretty sure if we have to deprecate the static function of AbstractThread::MainThread() to force the developer to label all the runnables to the AbstractThread of the main thread.
Attachment #8810332 - Flags: feedback?(wmccloskey)
Comment on attachment 8810332 [details] [diff] [review] (WIP) Add AbstractMainThread class to support labeling on AbstractThread::MainThread()::Dispatch(). Review of attachment 8810332 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Thanks Bevis. > 1. In some classes like MediaMemoryTracker, DecoderAllocPolicy are defined as singletons and there is no strong relationship to the a docgroup. I didn't see any uses of AbstractThread::MainThread() in MediaMemoryTracker. In DecoderAllocPolicy, I see one: http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/dom/media/MediaFormatReader.cpp#106 I think this can be converted to a normal NS_DispatchToMainThread for now. Eventually we'll want to label it as a task that's not related to any DocGroup. But there's no way to do that now. > 2. Instances like MediaStreamGraph::GetInstance() could be shared if the AudioChannel is the same. The only use of AbstractThread::MainThread I see for MediaStreamGraph is here: http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/dom/media/MediaStreamGraph.cpp#1778 However, I don't understand what's going on here. Hopefully Bobby can advise. > 3. New ordering problem in AutoTaskDispatcher comes after introducing this new wrapper class: > - PerThreadTaskGroup is identified by the reference of a AbstractThread. See > AutoTaskDispatcher::EnsureTaskGroup(DocGroup*). > - With this new wrapper, tasks dispatched to main thread will be divided into multiple > PerThreadTaskGroups if their docgroups are different which cause the order changed when dispatching > these tasks in ~AutoTaskDispatcher(). I think Bobby and I discussed this issue and concluded that this is the desired behavior. > 4. Not pretty sure if we have to deprecate the static function of AbstractThread::MainThread() to force the > developer to label all the runnables to the AbstractThread of the main thread. I think we want to get rid of it when this patch is done. ::: dom/base/AbstractMainThread.cpp @@ +24,5 @@ > +/* static */ > +already_AddRefed<AbstractMainThread> > +AbstractMainThread::GetOrCreate(DocGroup* aDocGroup) > +{ > + StaticMutexAutoLock lock(gAbstractMainThreadsMutex); Can you MOZ_RELEASE_ASSERT(NS_IsMainThread()) here? It seems like it's true for all callsites. Getting a docgroup off a window isn't threadsafe, so we might as well require this function be called on the main thread. I guess the lock might still be necessary if the destructor is called off the main thread. Is that true?
Attachment #8810332 - Flags: feedback?(wmccloskey)
Attachment #8810332 - Flags: feedback?(bobbyholley)
Attachment #8810332 - Flags: feedback+
(In reply to Bill McCloskey (:billm) from comment #3) > I didn't see any uses of AbstractThread::MainThread() in MediaMemoryTracker. Sorry for not pointing out clearly. It's here: http://searchfox.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1613 > > 4. Not pretty sure if we have to deprecate the static function of AbstractThread::MainThread() to force the > > developer to label all the runnables to the AbstractThread of the main thread. > > I think we want to get rid of it when this patch is done. Oops! I'll look into all the use cases besides the ones in dom/media. :p Then, there is generic problem to remove AbstractThread::MainThread() with this new class: More precisely speaking, in MozPromise::Then(), an AbstractThread* is needed as an input. However, the docgroup is not always available in these use cases: http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/dom/flyweb/FlyWebPublishedServer.cpp#488 http://searchfox.org/mozilla-central/source/dom/media/gmp/GMPServiceParent.cpp#908 http://searchfox.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1613 It seems that we also need another factory method called , for example, AbstractMainThread::GetNonDocGroupInstance() for these use cases and add MOZ_ASSERT(aDocGroup) in AbstractMainThread::GetOrCreate() to ensure that aDocGroup is always valid. > > +already_AddRefed<AbstractMainThread> > > +AbstractMainThread::GetOrCreate(DocGroup* aDocGroup) > > +{ > > + StaticMutexAutoLock lock(gAbstractMainThreadsMutex); > > Can you MOZ_RELEASE_ASSERT(NS_IsMainThread()) here? Will do.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4) > It seems that we also need another factory method called , for example, > AbstractMainThread::GetNonDocGroupInstance() for these use cases and add > MOZ_ASSERT(aDocGroup) in AbstractMainThread::GetOrCreate() to ensure that > aDocGroup is always valid. I agree. Sounds good to me.
Comment on attachment 8810332 [details] [diff] [review] (WIP) Add AbstractMainThread class to support labeling on AbstractThread::MainThread()::Dispatch(). Review of attachment 8810332 [details] [diff] [review]: ----------------------------------------------------------------- I don't have the cycles to look at this. Maybe JW can review?
Attachment #8810332 - Flags: feedback?(bobbyholley) → review?(jwwang)
Comment on attachment 8810332 [details] [diff] [review] (WIP) Add AbstractMainThread class to support labeling on AbstractThread::MainThread()::Dispatch(). Review of attachment 8810332 [details] [diff] [review]: ----------------------------------------------------------------- I've done some concrete change locally on AbstractMainThread to resolve the use case in MediaStreamGraphImpl::RunInStableState() by providing a static method called AbstractMainThread::DrainDirectTask() to drain the direct tasks in main thread's TrailDispatcher, because all AbstractMainThread instance share the same DirectTask queues. There are several issues remained to be fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1411346201c75547fb0b9fe0eed3c4ab1d36a5b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified I'll update the WIP patch again for the feedback/review once these issues are clarified or fixed.
Attachment #8810332 - Flags: review?(jwwang)
Comment on attachment 8813120 [details] [diff] [review] (v1) Part 1: Define AbstractMainThread class to replace AbstractThread::MainThread(). In AbstractThread, 1. Remove AbstractThread::MainThread() from AbstractThread class. 2. We modify AbstractThread::GetCurrent() to ensure that Main Thread instance of AbstractThread will always be an instance of AbstractMainThread, so sMainThread will only be accessed by AbstractMainThread class internally for the access of the unique TailDispatcher in main thread. In AbstractMainThread, 1. ::NonDocGroupInstance() is the replacement of AbstractThread::MainThread() without strong relationship to a DocGroup. 2. ::GetOrCreate(aDocGroup) returns the singleton for the specified aDocGroup. 3. ::DrainDirectTask() is a utility function to drain the Direct Task queue in the TailDispatcher of the main thread which is useful to MediaStreamGraphImpl::RunInStableState(). Changes in DocGroup.h & nsXHTMLContentSerializer.cpp are to fix the compiling error after this new class is added into UNIFIED_SOURCES. Hi Bill, May I have your review for the changes in AbstractThread? Thanks!
Attachment #8813120 - Flags: review?(wmccloskey)
Comment on attachment 8813122 [details] [diff] [review] (v1) Part 2: Factor out AbstractThread::MainThread() used in Media Playback. This patch factors out the use of AbtractThread::MainThread() in dom/media. The basic rule is that: 1. Hold a reference of a AbstractMainThread with the connected docGroup for AbstractThread::Dispatch(). - Most of the changes are related to the MediaDecoder/MediaDecoderOwner, MediaSourceDemuxer, and MediaSource/SourceBuffer. 2. Use AbstractMainThread::NonDocGroupInstance() if there is no connection to a docGroup. - The use case are in Parent actors and some services in parent and the feedback of the telemetry. 3. Replace AbstractThread::MainThread()->TailDispatcher().DrainDirectTasks() with AbstractMainThread::DrainDirectTask(). - See MediaStreamGraphImpl::RunInStableState(). The only issue to be clarified is that DocGroup could be unavailable when creating a HTMLMediaElement in these failure test cases: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd2bc2c86e1d84cdf2c004dd69325996e7b3ae5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified If it's true, currernt solution is to have NonDocGroupInstance() provided for these failed cases. Ask both Bill & JW in Media team for the review. BTW, Hi JW, I have ensure that current MDA tests are green in comment 12 but I am not pretty sure if the test coverage is good enough for this change. Could your help to confirm this as well? Thanks!
Attachment #8813122 - Flags: review?(wmccloskey)
Attachment #8813122 - Flags: review?(jwwang)
Comment on attachment 8813124 [details] [diff] [review] (v1) Part 3: Factor out AbstractThread::MainThread() used in FlyWeb. Changes in FlyWeb.
Attachment #8813124 - Flags: review?(wmccloskey)
Comment on attachment 8813130 [details] [diff] [review] (v1) Part 4: Factor out AbstractThread::MainThread() used in U2F. Changes in U2F.
Attachment #8813130 - Flags: review?(wmccloskey)
Comment on attachment 8813122 [details] [diff] [review] (v1) Part 2: Factor out AbstractThread::MainThread() used in Media Playback. Review of attachment 8813122 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: dom/media/Benchmark.cpp @@ +57,4 @@ > Preferences::GetUint("media.benchmark.timeout", 1000)) > }); > estimiser->Run()->Then( > + AbstractMainThread::NonDocGroupInstance(), __func__, Indention. Align with "[](...)" below. ::: dom/media/MediaDecoder.h @@ +92,4 @@ > MediaDecoder* mDecoder = nullptr; > nsCOMPtr<nsITimer> mTimer; > bool mTimerArmed = false; > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/MediaDecoderStateMachine.cpp @@ +1238,2 @@ > SetState<SeekingState>(Move(seekJob), EventVisibility::Suppressed)->Then( > + mainThread, __func__, Might use AbstractMainThread::NonDocGroupInstance() for telemetry as OggDemuxer.cpp does. ::: dom/media/mediasink/DecodedStream.cpp @@ +94,5 @@ > RefPtr<MediaStream> mStream; > // Main thread only. > MozPromiseHolder<GenericPromise> mFinishPromise; > + > + RefPtr<AbstractThread> mAbstractMainThread; const @@ +338,4 @@ > Promise mPromise; > RefPtr<OutputStreamManager> mOutputStreamManager; > UniquePtr<DecodedStreamData> mData; > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/mediasource/AutoTaskQueue.h @@ +56,3 @@ > } > RefPtr<TaskQueue> mTaskQueue; > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/mediasource/MediaSource.h @@ +149,4 @@ > > RefPtr<nsIPrincipal> mPrincipal; > > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/mediasource/MediaSourceDecoder.cpp @@ +44,5 @@ > MediaSourceDecoder::CreateStateMachine() > { > MOZ_ASSERT(NS_IsMainThread()); > + RefPtr<AbstractThread> mainThread = AbstractMainThread(); > + mDemuxer = new MediaSourceDemuxer(mainThread.forget()); AbstractMainThread() returns a raw pointer. Why not letting MediaSourceDemuxer() also take a raw pointer? ::: dom/media/mediasource/SourceBuffer.h @@ +168,4 @@ > void AppendDataErrored(const MediaResult& aError); > > RefPtr<MediaSource> mMediaSource; > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/mediasource/TrackBuffersManager.h @@ +468,4 @@ > // Strong references to external objects. > nsMainThreadPtrHandle<MediaSourceDecoder> mParentDecoder; > > + RefPtr<AbstractThread> mAbstractMainThread; const ::: dom/media/webaudio/BufferDecoder.h @@ +49,5 @@ > private: > virtual ~BufferDecoder(); > RefPtr<TaskQueue> mTaskQueueIdentity; > RefPtr<MediaResource> mResource; > + RefPtr<AbstractThread> mAbstractMainThread; const ::: xpcom/threads/StateWatching.h @@ +187,4 @@ > explicit WatchManager(OwnerType* aOwner, AbstractThread* aOwnerThread) > : mOwner(aOwner), mOwnerThread(aOwnerThread) {} > > + explicit WatchManager(OwnerType* aOwner, already_AddRefed<AbstractThread> aOwnerThread) Why changing |AbstractThread*| to |already_AddRefed<AbstractThread>|?
Attachment #8813122 - Flags: review?(jwwang) → review+
Comment on attachment 8813122 [details] [diff] [review] (v1) Part 2: Factor out AbstractThread::MainThread() used in Media Playback. Review of attachment 8813122 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1238,2 @@ > SetState<SeekingState>(Move(seekJob), EventVisibility::Suppressed)->Then( > + mainThread, __func__, Thanks for capturing this. I'll replace with the NonDocGroupInstance() for telemetry. ::: dom/media/mediasource/MediaSourceDecoder.cpp @@ +44,5 @@ > MediaSourceDecoder::CreateStateMachine() > { > MOZ_ASSERT(NS_IsMainThread()); > + RefPtr<AbstractThread> mainThread = AbstractMainThread(); > + mDemuxer = new MediaSourceDemuxer(mainThread.forget()); Because already_AddRefed<AbstractThread> is required in the constructors of MediaSourceDemuxer & AutoTaskQueue. ::: xpcom/threads/StateWatching.h @@ +187,4 @@ > explicit WatchManager(OwnerType* aOwner, AbstractThread* aOwnerThread) > : mOwner(aOwner), mOwnerThread(aOwnerThread) {} > > + explicit WatchManager(OwnerType* aOwner, already_AddRefed<AbstractThread> aOwnerThread) Thanks for capturing this. This is not necessary anymore after I create HTMLMediaElement::InitAbstractMainThread() for the initialization of HTMLMediaElement::mWatchManager. I'll remove this in next update.
Comment on attachment 8813122 [details] [diff] [review] (v1) Part 2: Factor out AbstractThread::MainThread() used in Media Playback. Review of attachment 8813122 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/mediasource/MediaSourceDecoder.cpp @@ +44,5 @@ > MediaSourceDecoder::CreateStateMachine() > { > MOZ_ASSERT(NS_IsMainThread()); > + RefPtr<AbstractThread> mainThread = AbstractMainThread(); > + mDemuxer = new MediaSourceDemuxer(mainThread.forget()); m... looks not necessary too to AutoTaskQuque. I'll update the patch to take a raw pointer in MediaSourceDemuxer & AutoTaskQuque instead.
Comment on attachment 8813122 [details] [diff] [review] (v1) Part 2: Factor out AbstractThread::MainThread() used in Media Playback. Review of attachment 8813122 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/StateWatching.h @@ +187,4 @@ > explicit WatchManager(OwnerType* aOwner, AbstractThread* aOwnerThread) > : mOwner(aOwner), mOwnerThread(aOwnerThread) {} > > + explicit WatchManager(OwnerType* aOwner, already_AddRefed<AbstractThread> aOwnerThread) This is still required for mWatchManager(AbstractMainThread::GetOrCreate()) in the constructor of TextTrackCue, so I'll keep this change.
Comment on attachment 8813120 [details] [diff] [review] (v1) Part 1: Define AbstractMainThread class to replace AbstractThread::MainThread(). Review of attachment 8813120 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good, but I think it could be simpler. Please ask JW to review the next version. I'm not familiar enough with tail dispatching to understand if that part works. ::: dom/base/AbstractMainThread.cpp @@ +51,5 @@ > +} > + > +/* static */ > +already_AddRefed<AbstractMainThread> > +AbstractMainThread::GetOrCreate(DocGroup* aDocGroup) I think this code could be a lot cleaner if you: 1. Refactor XPCOMThreadWrapper so that mTarget is an nsIEventTarget rather than an nsIThread. 2. Then GetOrCreate could create a new XPCOMThreadWrapper from DocGroup->CreateEventTarget(TaskCategory::Media) or something like that. @@ +60,5 @@ > + RefPtr<AbstractMainThread> thread = gAbstractMainThreads.Get(aDocGroup); > + > + if (!thread) { > + thread = new AbstractMainThread(aDocGroup); > + gAbstractMainThreads.Put(aDocGroup, thread); Is there any requirement that the DocGroup will last longer than the AbstractMainThread? I don't see why there would be. I think it might be better to have a field on the DocGroup that points to the AbstractMainThread (as a RefPtr). It would start out null and be created lazily. ::: xpcom/threads/AbstractThread.cpp @@ +192,5 @@ > +/* static */ AbstractThread* > +AbstractThread::GetCurrent() > +{ > + return NS_IsMainThread() ? dom::AbstractMainThread::NonDocGroupInstance() : > + sCurrentThreadTLS.get(); I guess we're going to have to set sCurrentThreadTLS when dispatching code for a given DocGroup?
Attachment #8813120 - Flags: review?(wmccloskey)
Attachment #8813124 - Flags: review?(wmccloskey) → review+
Attachment #8813130 - Flags: review?(wmccloskey) → review+
Attachment #8813122 - Flags: review?(wmccloskey) → review+
It seems that the current patch goes the wrong way that still sticks to a single AbstractThread of the main thread for tail dispatching. According to comment 21 and further offlined discussion with JW, the expected solution is to do the capability similar to TaskQueue: " Multiple TaskQueues could share the same nsIEventTarget but represents different sCurrentThreadTLS when their runnable is running. " I'd like to summarize the tasks here for the upcoming implementation: 1. Each DocGroup-specific AbstractThread is an instance of a new derived class of XPCOMThreadWrapper with DocGroup-specific EventTarget provided. We could define a new function called DocGroup::GetOrCreateAbstractThread(TaskCategory) as a factory method for this use case. 2. This derived class overrides the Dispatch() function to introduce the AutoTaskGuard that already used by TaskQueue to change the sCurrentThreadTLS when each runnable runs. 3. To ensure that the direct tasks of a DocGroup-specific AbstractThread could be drained correctly in MediaStreamGraphImpl::RunInStableState(), a wrapper runnable class with an AbstractMainThread specified is required by MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate() to have AutoTaskGuard protected for each runnable and have the correct TaskDispatcher handle to drain the direct tasks right after runnable is done in http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/dom/media/MediaStreamGraph.cpp#1768-1778
Thanks for the new API of DocGroup::EventTargetFor() introduced in bug 1320753. Now, we can provide an AbstractThread instance per EventTarget in this WIP with the following changes: 1. Refator XPCOMThreadWrapper as EventTargetWrapper. 2. Define AbstractThread::CreateEventTargetWrapper() for DocGroup::AbstractThreadFor() in next WIP patch. 3. Rename AbstractThread::MainThread() to AbstractThread::NonDocGroupMainThread() 4. Set AbstractThread::sCurrentThreadTLS correctly when each runnable dispatched to the AbstractThread is run to have expected response from AbstractThread::GetCurrent() and AbstractThread::IsCurrentThreadIn(). 5. Define AbstractThreadRunner class for the use case in MediaStreamGraphImpl::RunInStableState(). Hi JW & Bill, May I have your feedback again for this design change? To JW: Even though we both agreed that the IsCurrentThreadIn() shall behave more properly as what has been done in TaskQueue. However, there are couples of use cases that assert IsCurrentThreadIn() improperly in a normal runnable of the main thread instead of the one in Abstract::MainThread(). (You can check the call stack below for example.) I found couple of these issues in StateWatching.h and StateMirroring.h so far. (There might be more if they were not well-considered.) It seems that we have to review & revise the call sites of AbstractThread::IsCurrentThreadIn() to remove these improper assertions. Note: Similar problem can also be found in the use cases of AbstractThread::GetCurrent(). One option is to replaced with use DocGroup::AbstractThreadFor() in next WIP patch when possible. Do you have any suggestion on these issues? -- [Example of an improper assertion] The runnable is dispatched from ipc channel of PContentChild to the main thread. #01: mozilla::Canonical<double>::Impl::Set(double const&) (StateMirroring.h:170, in XUL) #02: mozilla::Canonical<double>::Set(double const&) (StateMirroring.h:246, in XUL) #03: mozilla::Canonical<double>::operator=(double const&) (StateMirroring.h:247, in XUL) #04: mozilla::MediaDecoder::SetVolume(double) (MediaDecoder.cpp:332, in XUL) #05: mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*, mozilla::MediaResource*, nsIStreamListener**) (HTMLMediaElement.cpp:4399, in XUL) #06: mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) (HTMLMediaElement.cpp:4378, in XUL) #07: mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) (HTMLMediaElement.cpp:450, in XUL) #08: mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) (HttpChannelChild.cpp:543, in XUL) #09: mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, unsigned int const&, nsCString const&) (HttpChannelChild.cpp:475, in XUL) #10: mozilla::net::StartRequestEvent::Run() (HttpChannelChild.cpp:344, in XUL) #11: mozilla::net::ChannelEventQueue::RunOrEnqueue(mozilla::net::ChannelEvent*, bool) (ChannelEventQueue.h:134, in XUL) #12: mozilla::net::HttpChannelChild::RecvOnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, short const&, unsigned int const&, nsCString const&) (HttpChannelChild.cpp:397, in XUL) #13: mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) (PHttpChannelChild.cpp:649, in XUL) #14: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (PContentChild.cpp:5895, in XUL) #15: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (MessageChannel.cpp:1750, in XUL) #16: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (MessageChannel.cpp:1688, in XUL) #17: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (MessageChannel.cpp:1573, in XUL) #18: mozilla::ipc::MessageChannel::MessageTask::Run() (MessageChannel.cpp:1598, in XUL) #19: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1214, in XUL)
Attachment #8813120 - Attachment is obsolete: true
Attachment #8813122 - Attachment is obsolete: true
Attachment #8813124 - Attachment is obsolete: true
Attachment #8813130 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jwwang)
Attachment #8817143 - Flags: feedback?
Hi Bill, The AbstractMainThread in patch v2 is now replaced with this new API and Abstract::NonDocGroupMainThread(). May I have your feedback for this design change? Thanks!
The AbstractThread::IsCurrentThreadIn() seems more related to the PR_Thread instead of each AbstractThread instance. I'll revise the patch again for further review. Cancel the ni? flags.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jwwang)
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #23) > To JW: > Even though we both agreed that the IsCurrentThreadIn() shall behave more > properly as what has been done in TaskQueue. > However, there are couples of use cases that assert IsCurrentThreadIn() > improperly in a normal runnable of the main thread instead of the one in > Abstract::MainThread(). (You can check the call stack below for example.) > I found couple of these issues in StateWatching.h and StateMirroring.h so > far. (There might be more if they were not well-considered.) > It seems that we have to review & revise the call sites of > AbstractThread::IsCurrentThreadIn() to remove these improper assertions. One solution I think of is to do an additional dispatch to ensure all media element code is run on its own thread. However, we should review the assertions case by case. For example, MediaLoadListener::OnStartRequest is called by HttpChannelChild asynchronously. It should be fine to do an additional dispatch.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #33) > One solution I think of is to do an additional dispatch to ensure all media > element code is run on its own thread. However, we should review the > assertions case by case. For example, MediaLoadListener::OnStartRequest is > called by HttpChannelChild asynchronously. It should be fine to do an > additional dispatch. After further review of the use cases and the implementation of IsCurrentThreadIn() in both XPCOMThreadWrapper and TaskQueue, this function seems more related to check if the current call stack is run in correct nsThread/PRThread level instead of in the AbstractThread level. So I revise the patch accordingly in comment 26. Checking whether current stack is inside its AbstractThread or not seems not in the scope of this change. What do you think? If you are okay, I like to start the review process of these new patches. :)
Flags: needinfo?(jwwang)
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #34) > (In reply to JW Wang [:jwwang] [:jw_wang] from comment #33) > > One solution I think of is to do an additional dispatch to ensure all media > > element code is run on its own thread. However, we should review the > > assertions case by case. For example, MediaLoadListener::OnStartRequest is > > called by HttpChannelChild asynchronously. It should be fine to do an > > additional dispatch. > > After further review of the use cases and the implementation of > IsCurrentThreadIn() in both XPCOMThreadWrapper and TaskQueue, this function > seems more related to check if the current call stack is run in correct > nsThread/PRThread level instead of in the AbstractThread level. > So I revise the patch accordingly in comment 26. > > Checking whether current stack is inside its AbstractThread or not seems not > in the scope of this change. > What do you think? > > If you are okay, I like to start the review process of these new patches. :) BTW, the GetCurrent() does related to the current running AbstractThread. So, the AbstractThreadRunner added in comment 26 does ensure that the sCurrentThreadTLS is updated accordingly when its runnable is being executed.
We had some conclusion on IRC.
Flags: needinfo?(jwwang)
Comment on attachment 8817397 [details] [diff] [review] (v3) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8817397 [details] [diff] [review]: ----------------------------------------------------------------- In this revision, EventTarget is now possible to be an AbstractThread. In addition, an AbstractThreadRunner class is defined to allow the runnables to MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate() to drain the direct tasks generated after executing. Hi JW, May I have your review again for this change? Thanks! Note: the issue of making XPCOMThreadWrapper::IsCurrentThreadIn() be AbstractThread-specific filed in bug 1323742.
Attachment #8817397 - Flags: review?(jwwang)
Comment on attachment 8817398 [details] [diff] [review] (v3) Part 1.2: Define DocGroup::AbstractThreadFor(TaskCategory aCategory). Review of attachment 8817398 [details] [diff] [review]: ----------------------------------------------------------------- The replacement of the AbstractMainThread class in previous revision. The non-docgroup one is replaced by the renamed function of AbstracThread::NonDocGroupMainThread() in part 1.1. Hi Bill, May I have your review again for this change? Thanks!
Attachment #8817398 - Flags: review?(wmccloskey)
Comment on attachment 8817400 [details] [diff] [review] (v3) Part 2.2: Drain Direct Tasks using AbstractThreadRunner in Media Playback. Review of attachment 8817400 [details] [diff] [review]: ----------------------------------------------------------------- This is a fix to have an AbstractThreadRunner instance created for each runnable dispatched to MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate(). Hi JW, May I have your review for this change? Thanks!
Attachment #8817400 - Flags: review?(jwwang)
Comment on attachment 8817397 [details] [diff] [review] (v3) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8817397 [details] [diff] [review]: ----------------------------------------------------------------- What's the purpose of this patch? If you want to create an AbstractThread from an nsIEventTarget, you can just use TaskQueue.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #40) > Comment on attachment 8817397 [details] [diff] [review] > (v3) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow > EventTargets to be AbstractThreads. > > Review of attachment 8817397 [details] [diff] [review]: > ----------------------------------------------------------------- > > What's the purpose of this patch? If you want to create an AbstractThread > from an nsIEventTarget, you can just use TaskQueue. It's to make the EventTarget from DocGroup::EventTargetFor() to be an DocGroup-specific AbstractThread::MainThread(). It's not possible to wrap this as TaskQueue which could make the assertion of IsCurrentThreadIn() failed as mentioned in comment 23. :(
I didn't notice that the TaskCategory::Count was introduced when rebasing. Revise the patch again using TaskCategory::Count.
Attachment #8817398 - Attachment is obsolete: true
Attachment #8817398 - Flags: review?(wmccloskey)
Attachment #8819713 - Flags: review?(wmccloskey)
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #41) > It's not possible to wrap this as TaskQueue which could make the assertion > of IsCurrentThreadIn() failed as mentioned in comment 23. :( Why don't we fix IsCurrentThreadIn() and remove XPCOMThreadWrapper?
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #43) > Why don't we fix IsCurrentThreadIn() and remove XPCOMThreadWrapper? It's not only about fixing the IsCurrentThreadIn(). In addition to fix these assertions, TaskQueue is more powerful and have more control regarding the order of the dispatched runnables which is not needed for the call sites of AbstractThread::MainThread(). IMHO, to make an EventTarget as an AbstractThread or a TaskQueue is different. Moreover, there are other use cases for XPCOMThreadWrapper, so we cannot just remove the XPCOMThreadWrapper: https://dxr.mozilla.org/mozilla-central/search?q=CreateXPCOMThreadWrapper&redirect=false
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #44) > Moreover, there are other use cases for XPCOMThreadWrapper, so we cannot > just remove the XPCOMThreadWrapper: > https://dxr.mozilla.org/mozilla-central/ > search?q=CreateXPCOMThreadWrapper&redirect=false I think they can be replaced by TaskQueue.
Comment on attachment 8817397 [details] [diff] [review] (v3) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8817397 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/AbstractThread.cpp @@ +268,5 @@ > + MOZ_ASSERT(aEventTarget); > + nsCOMPtr<nsIThread> thread(do_QueryInterface(aEventTarget)); > + Unused << thread; > + MOZ_ASSERT(!thread, "nsIThread should be wrapped by CreateXPCOMThreadWrapper!"); > + Why don't you have |sCurrentThreadTLS.set(wrapper);| as above? I think AbstractThread::GetCurrent() is broken for this implementation. ::: xpcom/threads/AbstractThread.h @@ +97,5 @@ > > + // The non-DocGroup version of AbstractThread on the main thread. > + // A DocGroup-versioned one is available in DocGroup::AbstractThreadFor(). > + // Note: DocGroup::AbstractThreadFor() SHALL be used when possible. > + static AbstractThread* NonDocGroupMainThread(); I don't think we need to change the function name for the semantics doesn't change at all. Just add a comment to note that MainThread() returns an AbstractThread that is not associated with any doc group.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #46) > Why don't you have |sCurrentThreadTLS.set(wrapper);| as above? I think > AbstractThread::GetCurrent() is broken for this implementation. This has now been protected in the new Runner class which will override sCurrentThreadTLS when a runnable is running and rollback after this runnable is complete. > > + // Note: DocGroup::AbstractThreadFor() SHALL be used when possible. > > + static AbstractThread* NonDocGroupMainThread(); > > I don't think we need to change the function name for the semantics doesn't > change at all. Just add a comment to note that MainThread() returns an > AbstractThread that is not associated with any doc group. Sure, I'll keep the original name since we both agree that a more comprehensive comment is good enough. :)
Comment on attachment 8817397 [details] [diff] [review] (v3) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8817397 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/AbstractThread.cpp @@ +121,5 @@ > > private: > + // This is set in constructor and is only used for pointer-comparison with > + // current thread, so we don't ref-count it. > + nsIThread* mRunningThread; The comparison will be invalid if mRunningThread becomes a dangling pointer. ::: xpcom/threads/AbstractThread.h @@ +109,5 @@ > > + // XXX: For MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate() and > + // MediaStreamGraphImpl::RunInStableState() to drain the direct tasks properly > + // after DocGroup::AbstractThreadFor() is introduced. > + virtual already_AddRefed<AbstractThreadRunner> Can you just return already_AddRefed<nsIRunnable>? @@ +132,5 @@ > + */ > +class AbstractThreadRunner : public Runnable { > +public: > + NS_IMETHOD Run() override final { > + IsRunInCurrentThread = false; Why setting it again when it is already set in the constructor? @@ +155,5 @@ > + } > + > +private: > + RefPtr<AbstractThread> mThread; > + bool IsRunInCurrentThread; mIsRunInCurrentThread;
Comment on attachment 8817400 [details] [diff] [review] (v3) Part 2.2: Drain Direct Tasks using AbstractThreadRunner in Media Playback. Review of attachment 8817400 [details] [diff] [review]: ----------------------------------------------------------------- I prefer the original code where MSG will drain direct tasks after running each stream udpate runnables. This patch forces every user of MSG to be aware of the details of direct tasks.
Attachment #8817400 - Flags: review?(jwwang) → review-
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #49) > I prefer the original code where MSG will drain direct tasks after running > each stream udpate runnables. This patch forces every user of MSG to be > aware of the details of direct tasks. Yes, this is bad but this is the only chance to associate the dispatched MSG runnable to its AbstractThread by my current understanding. :-( I'll look into the MediaStreamGraph implementation to see if there is other possibility not to expose these detail to the users of MSG. Any better suggestion or whom familiar with MSG I should reach for more input are welcome, thanks! :)
Comment on attachment 8819713 [details] [diff] [review] (v3) Part 1.2: Define DocGroup::AbstractThreadFor(TaskCategory aCategory). Review of attachment 8819713 [details] [diff] [review]: ----------------------------------------------------------------- It seems better to provide AbstractMainThreadFor() in DispatcherTrait as well for user to retrieve a AbstractThread instance easier from nsIDocument or nsIGlobalWindow. Cancel the review and update a new patch to address this.
Attachment #8819713 - Flags: review?(wmccloskey)
1. Addressed suggestions in comment 48. 2. Make ::CreateTaskRunner() as private and returns an instance of nsIRunnable instead of AbstractThreadRunner. 3. make MediaStreamGraph as a friend class of AbstractThread to utilize AbstractThread::CreateTaskRunner(). Note: CreateTaskRunner() is now invoked inside MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate() instead in the upcoming patch part 2.2 (v4) This shall resolve the concern about exposing too much detail of the internal implementation of AbstractThread to the user. The only parameter requires from caller additional is the AbstractThread instance that the caller is associated with for MSG to drain the generated directed tasks properly.
Attachment #8817397 - Attachment is obsolete: true
Attachment #8817397 - Flags: review?(jwwang)
Attachment #8822006 - Flags: review?(jwwang)
Add AbstractThreadFor() API to Dispatcher & DispatcherTrait to make the access of AbstractThread easier from nsDocument & nsGlobalWindow.
Attachment #8819713 - Attachment is obsolete: true
1. Revise the parameter of DispatchToMainThreadAfterStreamStateUpdate from (already_AddRefed<AbstractThreadRunner> aRunnable) to (AbstractThread*, already_AddRefed<AbstractThreadRunner> aRunnable) 2. The runnable will be wrapped internally in MediaStreamGraph by calling AbstractThread::CreateTaskRunner(). 3. The only thing that MediaStream and its sub-class has to do is to provide the AbstractThread instance associated with when calling DispatchToMainThreadAfterStreamStateUpdate().
Attachment #8817400 - Attachment is obsolete: true
Attachment #8822013 - Flags: review?(jwwang)
rebased.
Attachment #8817402 - Attachment is obsolete: true
Attachment #8822015 - Flags: review+
Attachment #8822007 - Attachment description: (v4) Part 1.2: Add AbstractThreadFor() API to Dispatcher & DispatcherTrait. → (v4) Part 1.2: Add AbstractMainThreadFor() API to Dispatcher & DispatcherTrait.
Attachment #8822007 - Flags: review?(wmccloskey)
Comment on attachment 8822007 [details] [diff] [review] (v4) Part 1.2: Add AbstractMainThreadFor() API to Dispatcher & DispatcherTrait. Review of attachment 8822007 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Dispatcher.h @@ +61,5 @@ > + // off the main thread. > + virtual AbstractThread* > + AbstractMainThreadFor(TaskCategory aCategory) const; > +protected: > + virtual ~DispatcherTrait() {} Are you getting compiler warnings somewhere? I don't think the non-virtual destructor is actually an issue here since we'll never delete a DispatcherTrait*. In any case, please put a line break before protected: if you want to keep it. @@ +85,2 @@ > protected: > + virtual ~Dispatcher() {} Same here. Deletion will always go through Release, which is virtual. We'll never delete a Dispatcher directly. ::: dom/base/TabGroup.cpp @@ +220,5 @@ > + MOZ_RELEASE_ASSERT(NS_IsMainThread()); > + MOZ_RELEASE_ASSERT(!mLastWindowLeft); > + MOZ_ASSERT(aCategory != TaskCategory::Count); > + > + RefPtr<AbstractThread>& thread = mAbstractThreads[size_t(aCategory)]; The reference makes this code a bit confusing. Can you just update mAbstractThreads directly in the branch below? @@ +221,5 @@ > + MOZ_RELEASE_ASSERT(!mLastWindowLeft); > + MOZ_ASSERT(aCategory != TaskCategory::Count); > + > + RefPtr<AbstractThread>& thread = mAbstractThreads[size_t(aCategory)]; > + if(!thread) { Need a space after |if|. ::: dom/base/TabGroup.h @@ +124,5 @@ > nsTArray<nsPIDOMWindowOuter*> mWindows; > RefPtr<ThrottledEventQueue> mThrottledEventQueue; > nsCOMPtr<nsIEventTarget> mEventTargets[size_t(TaskCategory::Count)]; > + // |mutable| for the lazy getter of AbstractMainThreadFor() const. > + mutable RefPtr<AbstractThread> mAbstractThreads[size_t(TaskCategory::Count)]; Does AbstractMainThreadFor have to be const? This is kind of ugly. ::: dom/base/nsDocument.cpp @@ +2905,5 @@ > +AbstractThread* > +nsIDocument::AbstractMainThreadFor(mozilla::dom::TaskCategory aCategory) const > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if(mDocGroup) { Need a space after |if|.
Attachment #8822007 - Flags: review?(wmccloskey) → review+
Comment on attachment 8822006 [details] [diff] [review] (v4) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8822006 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/AbstractThread.h @@ +119,5 @@ > + MOZ_CRASH("Not support!"); > + } > + > +private: > + friend class MediaStreamGraph; It is bad for this XPCOM class to have dependency on a particular media class. You should just expose this function as others. For now MediaStreamGraph is the only consumer. Also the name CreateTaskRunner is too vague to show what is really done inside. IIUC, the caller of this function will always need to pass the return value to Dispatch() to get the job done. So it would be better to have a function to do these 2 things in one single call for they are highly coupled.
Attachment #8822006 - Flags: review?(jwwang) → review-
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #60) > It is bad for this XPCOM class to have dependency on a particular media > class. You should just expose this function as others. For now > MediaStreamGraph is the only consumer. Will do. > Also the name CreateTaskRunner is too vague to show what is really done > inside. How about renaming it to |CreateDirectTaskHelper| as follow: // Returns a runnable to drain the direct task generated by |aRunnable| virtual already_AddRefed<nsIRunnable> CreateTaskRunner(already_AddRefed<nsIRunnable> aRunnable); > IIUC, the caller of this function will always need to pass the return value > to Dispatch() to get the job done. So it would be better to have a function > to do these 2 things in one single call for they are highly coupled. No, as explained in comment 55, the returned runnable will be appended to MSG::mPendingUpdateRunnables[]. There is no direct call to the Dispatch() method when calling this function. So it shall be fine to just return a runnable from this function.
Flags: needinfo?(jwwang)
How about: // Create a runnable that will run |aRunnable| and drain the direct tasks generated by it. virtual already_AddRefed<nsIRunnable> CreateDirectTaskDrainer(already_AddRefed<nsIRunnable> aRunnable);
Flags: needinfo?(jwwang)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #62) > How about: > > // Create a runnable that will run |aRunnable| and drain the direct tasks > generated by it. > virtual already_AddRefed<nsIRunnable> > CreateDirectTaskDrainer(already_AddRefed<nsIRunnable> aRunnable); sounds good to me! (y)
address suggestion in comment 62 to rename |CreateTaskRunner| to |CreateDirectTaskDrainer| and make it as a public function.
Attachment #8822006 - Attachment is obsolete: true
Attachment #8822593 - Flags: review?(jwwang)
address comment 59 but keep the virtual destructor to suppress the compiler warning.
Attachment #8822007 - Attachment is obsolete: true
Attachment #8822594 - Flags: review+
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #65) > Created attachment 8822594 [details] [diff] [review] > (v5) Part 1.2: Add AbstractMainThreadFor() API to Dispatcher & > DispatcherTrait. r=billm > > address comment 59 but keep the virtual destructor to suppress the compiler > warning. What compiler gives this warning?
Comment on attachment 8822593 [details] [diff] [review] (v5) Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. Review of attachment 8822593 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/AbstractThread.h @@ +107,5 @@ > > + // Create a runnable that will run |aRunnable| and drain the direct tasks > + // generated by it. > + virtual already_AddRefed<nsIRunnable> > + CreateDirectTaskDrainer(already_AddRefed<nsIRunnable> aRunnable) Please merge this function with CreateDirectTaskDrainerInternal(). I don't see the point to have an additional function.
Attachment #8822593 - Flags: review?(jwwang) → review+
(In reply to Bill McCloskey (:billm) from comment #66) > What compiler gives this warning? It's from my eclipse on mac: "Class 'DispatcherTrait' has virtual method 'AbstractMainThreadFor' but non-virtual destructor" "Class 'Dispatcher' has virtual method 'Release' but non-virtual destructor"
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #68) > (In reply to Bill McCloskey (:billm) from comment #66) > > What compiler gives this warning? > It's from my eclipse on mac: > "Class 'DispatcherTrait' has virtual method 'AbstractMainThreadFor' but > non-virtual destructor" > "Class 'Dispatcher' has virtual method 'Release' but non-virtual destructor" So presumably clang prints the warning. Can you see what version of clang you're using? I tested on my machine and I don't get a warning with clang-3.8. I would rather avoid adding extra code only to hide a warning printed by an old compiler.
(In reply to Bill McCloskey (:billm) from comment #69) > So presumably clang prints the warning. Can you see what version of clang > you're using? I tested on my machine and I don't get a warning with > clang-3.8. > > I would rather avoid adding extra code only to hide a warning printed by an > old compiler. It's clang --version Apple LLVM version 8.0.0 (clang-800.0.42.1) Target: x86_64-apple-darwin16.3.0 Thread model: posix InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin Xcode version: 8.2.1 But I thought its always a good habit to have virtual destructor if we have virtual method declared then we won't forget to have it when implementation change.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #70) > But I thought its always a good habit to have virtual destructor if we have > virtual method declared then we won't forget to have it when implementation > change. It seems to be a configuration issue in my Eclipse's code analysis tool instead of clang because I can't see this warning from ./mach build result. If always having a virtual destructor when a base class contains virtual methods is not a coding convention we have to follow in gecko, I'll remove them in next update.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #71) > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #70) > > But I thought its always a good habit to have virtual destructor if we have > > virtual method declared then we won't forget to have it when implementation > > change. In this particular case (and, I think, for all refcounted classes), it's not likely that we'll accidentally introduce new warnings through code changes. > It seems to be a configuration issue in my Eclipse's code analysis tool > instead of clang because I can't see this warning from ./mach build result. > > If always having a virtual destructor when a base class contains virtual > methods is not a coding convention we have to follow in gecko, I'll remove > them in next update. Thanks.
(In reply to Bill McCloskey (:billm) from comment #72) > In this particular case (and, I think, for all refcounted classes), it's not > likely that we'll accidentally introduce new warnings through code changes. Agree, as you mentioned in comment 59, deletion will always be done in Release() which is virtual, it seems not necessary to have these virtual destructors. (Start to wonder why other reviewers asked me to correct this. :p) Thanks for your time for these explanation! :-)
Comment on attachment 8822013 [details] [diff] [review] (v4) Part 2.2: Drain Direct Tasks using AbstractThreadRunner in Media Playback. Review of attachment 8822013 [details] [diff] [review]: ----------------------------------------------------------------- There are significant changes to the MSG API. Please ask a MSG peer for review.
Attachment #8822013 - Flags: review?(jwwang)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #74) > There are significant changes to the MSG API. Please ask a MSG peer for > review. Okay, but I have not much information about who are MSG peers from https://wiki.mozilla.org/Modules/All#Media_Playback May I know whom is recommended for the review of this change? Thanks!
Flags: needinfo?(jwwang)
Hi Randell, May I have your review for these API change in MediaStreamGraph to support the DocGroup labeling (with *origin* info) for the runnables consumed by AbstractThread::MainThread()? The reasons for these API change is for the support of the 1st step in Quantum-DOM project: https://wiki.mozilla.org/Quantum/DOM to have better scheduling/preemption approach for runnables on the main thread by labeling and categorizing the runnables in main thread per doc-group. In this bug, we focus more on the use case of AbstractThread::MainThread(). In patch part 1.2, a new ::AbstractMainThreadFor() method was introduced to provide per docgroup version of AbstractThread::MainThread(). The corresponding change in media playback are also done in patch part2.1. However, There is a special use case of draining direct task in MSG http://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#1781 which executing the runnable appended from MSG::DispatchToMainThreadAfterStreamStateUpdate() and drain the generated direct tasks by AbstractThread::MainThread()->TailDispatcher().DrainDirectTasks(); This is no longer workable after per-docgroup AbstractThread is applied in patch 2.1. To resolve this problem, in this patch, we have to 1. Bind the correct AbstractThread instance to a media stream. 2. Specify correct AbstractThread when calling DispatchToMainThreadAfterStreamStateUpdate(). 3. Utilize AbstractThread::CreateDirectTaskDrainer() as helper in MSG to drain the direct tasks generated by the dispatched runnables. Note: AbstractThread::MainThread() is still available for the corner cases that has no impact for the task scheduling such as telemetry runnables or tasks in parent process.
Attachment #8822013 - Attachment is obsolete: true
Flags: needinfo?(jwwang)
Attachment #8823520 - Flags: review?(rjesup)
Refinement to move ::mAbstractMainThread from subclasses to base class of AudioNodeEngine. The detailed explanation for the change of this patch is at comment 76 to bind a proper AbstractMainThread instance to MediaStream, AudioNodeEngine for the API change of MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate().
Attachment #8823520 - Attachment is obsolete: true
Attachment #8823520 - Flags: review?(rjesup)
Attachment #8823558 - Flags: review?(rjesup)
Comment on attachment 8823558 [details] [diff] [review] (v5) Part 2.2: Use AbstractThread::CreateDirectTaskDrainer() to Drain Direct Tasks Dispatched to MediaStreamGraph. Review of attachment 8823558 [details] [diff] [review]: ----------------------------------------------------------------- Ignoring the 'explicit' nits.... There are a bunch of changes here. While most are straightforward (though mildly annoying), a) I think there should be more commentary in comments in MediaStreamGraph.h and/or elsewhere are to why these are needed and what they do; b) I'm concerned that this may violate assumptions for in-order execution of MSG runnables -- perhaps it doesn't, but the details of what the *result* of this is aren't clear. I'm guessing from the overall comments and quantum doc that different docgroups could execute without any cross-group ordering. The problem is that MSG interacts with many mediastreams from many docgroups, and the tests in treeherder aren't designed to test media in multiple windows/origins, so green there doesn't tell you a great deal about such issue. I think these changes will require media-specific tests to verify that nothing breaks when multiple tabs are hammering MSG/WebAudio/<video>/etc, so if we break anything it will fail and warn us. Some assertions or even MOZ_RELEASE_ASSERTs might be needed as well. Also there's plenty of other code that interacts with MSG. c) We're going to have multiple MSGs soon running different outputs, and we can have offline MSGs today plus the main MSG (IIRC). Not sure if this will impact things. d) We'll need more MSG-peer eyes on this, in particular padenot, and maybe karlt or jya. I'm going to give feedback+ at this point until I understand more ::: dom/html/HTMLMediaElement.cpp @@ +4651,2 @@ > public: > explicit StreamListener(HTMLMediaElement* aElement, const char* aName) : This explicit should go... ::: dom/media/AudioCaptureStream.h @@ +23,5 @@ > class AudioCaptureStream : public ProcessedMediaStream, > public MixerCallbackReceiver > { > public: > + explicit AudioCaptureStream(TrackID aTrackId, AbstractThread* aMainThread); remove explicit ::: dom/media/DOMMediaStream.h @@ +589,5 @@ > // UnregisterTrackListener before being destroyed, so we don't hold on to > // a dead pointer. Main thread only. > void UnregisterTrackListener(TrackListener* aListener); > > + AbstractThread* AbstractMainThread() { return mAbstractMainThread; } const? ::: dom/media/MediaStreamGraph.cpp @@ +2500,5 @@ > class Message : public ControlMessage { > public: > explicit Message(MediaStream* aStream, > + already_AddRefed<nsIRunnable> aRunnable, > + AbstractThread* aMainThread) remove explicit @@ +3221,5 @@ > MediaInputPort::BlockSourceTrackId(TrackID aTrackId, BlockingMode aBlockingMode) > { > class Message : public ControlMessage { > public: > explicit Message(MediaInputPort* aPort, remove explicit ::: dom/media/webspeech/synth/nsSpeechTask.cpp @@ +32,5 @@ > { > public: > explicit SynthStreamListener(nsSpeechTask* aSpeechTask, > + MediaStream* aStream, > + AbstractThread* aMainThread) remove explicit
Attachment #8823558 - Flags: review?(padenot)
Attachment #8823558 - Flags: feedback+
(In reply to Randell Jesup [:jesup] from comment #78) > b) I'm concerned that this may violate assumptions for in-order execution of > MSG runnables -- perhaps it doesn't, but the details of what the *result* of > this is aren't clear. I'm guessing from the overall comments and quantum > doc that different docgroups could execute without any cross-group > ordering. The problem is that MSG interacts with many mediastreams from > many docgroups, and the tests in treeherder aren't designed to test media in > multiple windows/origins, so green there doesn't tell you a great deal about > such issue. I think these changes will require media-specific tests to > verify that nothing breaks when multiple tabs are hammering > MSG/WebAudio/<video>/etc, so if we break anything it will fail and warn us. > Some assertions or even MOZ_RELEASE_ASSERTs might be needed as well. Also > there's plenty of other code that interacts with MSG. Yes, the ordering of runnables from different DocGroups is not defined. Ideally, a runnable from one DocGroup should never touch resources from another DocGroup. If that's true, then I don't think the ordering issue is a problem. We would like to add assertions to ensure that a runnable labeled with a given DocGroup doesn't affect other DocGroups. Currently those assertions are pretty JS- and DOM-specific. Is there anything media-related that we could assert? It would be something of the form "If we're running in the context of DocGroup A, and we try to use a MediaStream B, assert that B's docgroup == A." The tricky part is defining things like "try to use" and "B's docgroup", so maybe you guys could help with that?
(In reply to Bill McCloskey (:billm) from comment #79) > (In reply to Randell Jesup [:jesup] from comment #78) > > b) I'm concerned that this may violate assumptions for in-order execution of > > MSG runnables -- perhaps it doesn't, but the details of what the *result* of > > this is aren't clear. > Yes, the ordering of runnables from different DocGroups is not defined. > Ideally, a runnable from one DocGroup should never touch resources from > another DocGroup. If that's true, then I don't think the ordering issue is a > problem. We would like to add assertions to ensure that a runnable labeled > with a given DocGroup doesn't affect other DocGroups. Currently those > assertions are pretty JS- and DOM-specific. Is there anything media-related > that we could assert? It would be something of the form "If we're running in > the context of DocGroup A, and we try to use a MediaStream B, assert that > B's docgroup == A." The tricky part is defining things like "try to use" and > "B's docgroup", so maybe you guys could help with that? This out-of-order may be fine, and may not be. MSG itself processes data for all MediaStreams in the process (well, modulo disconnected WebAudio graphs or future output selection yielding multiple graphs). This means it's processing the streams for page A, page B, etc - but page A and page B's streams are all separate. Commands from DOM go over a message-passing interface and are grouped for execution; results tend to go back as runnables -- probably per-docgroup consistency here is fine, but it needs some careful thought and code-checking. There are also some mainthread interactions for shutdown. Also: when we do update mainthread state (PrepareUpdatesToMainthreadState()), we bundle up a bunch of updates from *all* streams, and send them the MainThread via DispatchToMainThread(). In general, two MSG users can't observe each other (except maybe very indirectly). I'm betting on it being ok. I could be wrong. This does relax some fundamental assumptions code may have made about consistency of execution.
This is _probably_ ok to do. However, it could lead to a bunch of very subtle bug. I've concerned, for example, about not running script for a background tab that is currently rendering audio, using JavScript to schedule what is going to happen with audio in the future. I'm going to write a patch that makes it so that we run different MSGs for different document or something. I say "or something" because I don't know if it's going to be by document or by other ID, but the idea is more or less that. Maybe using the new DocGroup mechanism would be good here. This will allow us to isolate potential issues ahead of landing your patches, so we can reason better about ordering, and provide a better assessment of whether your patch will break us. I've been running with a lot of content processes for a very long time, and I haven't seen a single MSG issue, so I think writing the above will be easy, because we don't have dependencies, within the MSG, about objects that are across documents. Finally, when we are done writing our patch, we need to investigate the performance of having multiple MSGs. MSGs often run on thread that have the highest priority on the system, having more than one of those thread might or might not be a problem. I'm going to check CPU usage, memory usage and battery consumption. In general, the performance of the MSG is rather important and touchy. I hope to have an answer to give this week to unblock this. Bill, Bevis, does that sound good ?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(btseng)
I don't know anything about MediaStreamGraphs, so I can't really offer any feedback on the plan. Thanks for helping though. > This is _probably_ ok to do. However, it could lead to a bunch of very subtle bug. I've concerned, for > example, about not running script for a background tab that is currently rendering audio, using > JavScript to schedule what is going to happen with audio in the future. I'm not sure what you mean here. This work in this bug is only about reordering events, not about delaying them even when nothing is happening. Yes, if the foreground tab is hogging the main thread, then the background tab might not run for a while. But that could happen even without any Quantum stuff. It just becomes a little more likely with Quantum DOM. (It also becomes a little less likely that a background tab will cause media-related JS in a foreground tab to be delayed.)
Flags: needinfo?(wmccloskey)
It's great help to clarify more before having this patch landing if we have more concern on this change. Besides, as Bill mentioned, these patches only reorders the events per doc-group without suspending any events in background tab.
Flags: needinfo?(btseng)
Any updates Paul?
Flags: needinfo?(padenot)
Update my patch in treeherder rebased to current m-c. If someone would like to do more test in latest m-c with my patches(there are 6 in total), here is the link for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2d877be485d31601035f834cc745256b4e157be&group_state=expanded
Comment on attachment 8823558 [details] [diff] [review] (v5) Part 2.2: Use AbstractThread::CreateDirectTaskDrainer() to Drain Direct Tasks Dispatched to MediaStreamGraph. Review of attachment 8823558 [details] [diff] [review]: ----------------------------------------------------------------- I think we can land this for now.
Attachment #8823558 - Flags: review?(padenot) → review+
Update final patch. Return AbstractThread::MainThread() for the chrome tabgroup according to the change in bug 1332095 when the EventTarget is always be the main thread.
Attachment #8822594 - Attachment is obsolete: true
Attachment #8828719 - Flags: review+
Address the suggestion from :jesup in comment 78 and cancel the ni? flag after the review is granted from :padenot.
Attachment #8823558 - Attachment is obsolete: true
Attachment #8823558 - Flags: review?(rjesup)
Flags: needinfo?(padenot)
Attachment #8828723 - Flags: review+
Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab1b5ce5bbc Part 1.1: Refator XPCOMThreadWrapper as EventTargetWrapper to allow EventTargets to be AbstractThreads. r=jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf74b47953a Part 1.2: Define AbstractThreadFor(TaskCategory aCategory) in Dispatcher and DispatcherTrait. r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5d3ec02dcd Part 2.1: Factor out AbstractThread::MainThread() used in Media Playback. r=billm,jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/02e5708a75fc Part 2.2: Use AbstractThread::CreateDirectTaskDrainer() to Drain Direct Tasks Dispatched to MediaStreamGraph. f=rjesup,r=padenot,jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cfaafedb43 Part 3: Factor out AbstractThread::MainThread() used in FlyWeb. r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/66e7178c3154 Part 4: Factor out AbstractThread::MainThread() used in U2F. r=billm
Depends on: 1333371
Depends on: 1335624
Depends on: 1412184
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: