1154782 - Clean up AbstractThread dispatch semantics

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

My branch for bug 1144486 is getting pretty long, so I'm separating out pieces that can land on their own. The main goals of this bug are to automate the assertion of dispatch success and streamline the API entry points on MediaTaskQueue.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53ed28bfb5e

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Detemplatize nsIThread wrapper and move it into AbstractThread.cpp. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist dispatch failure handling into dispatch itself. v1

This is nicer than scattering the failure handling to each callsite. Moreover,
this is a necessary step on the road to automatic tail dispatch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove the concept of forced dispatch. v1

I added this several months ago to make MediaPromise dispatch reliable in the
presence of the god-awful Flush() feature of MediaTaskQueue. That feature has
now been restricted to a special subclass (which we should eventually eliminate),
so we can just assert that MediaPromises are never used on those task queues
and simplify the rest of the code.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Change the canonical internal smart pointer type in MediaTaskQueue to the XPCOM style. v1

We've been meaning to do this for a while, just haven't gotten around to it.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Align the failure handling of the TemporaryRef Dispatch overload with its companion. v1

Most of the callers ignore the return values. The MOZ_DIAGNOSTIC_ASSERTS will
tell us if any of these actually fail in practice.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8593034 [details] [diff] [review]
Part 5 - Align the failure handling of the TemporaryRef Dispatch overload with its companion. v1

Review of attachment 8593034 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/eme/EMEDecoderModule.cpp
@@ +35,5 @@
>    }
>  
>    virtual nsresult Init() override {
>      MOZ_ASSERT(!mIsShutdown);
> +    mTaskQueue->SyncDispatch(NS_NewRunnableMethod(mDecoder, &MediaDataDecoder::Init));

FYI, I removed the use of SyncDispatch() from EMEDecoderModule.cpp in bug 1154133, and alfredo is removing it from the Gonk PDM in bug 1154512. So soon we can remove MediaTaskQueue::SyncDispatch(). :)

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8593032 [details] [diff] [review]
Part 3 - Remove the concept of forced dispatch. v1

Review of attachment 8593032 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaPromise.h
@@ +320,5 @@
>      // infrequently.
>      aDispatcher.AssertIsTailDispatcherIfRequired();
>  
>      MutexAutoLock lock(mMutex);
> +    MOZ_ASSERT(aResponseThread->IsDispatchReliable());

Does this assertion mean MediaPromise should be used with reliable dispatcher only?

::: dom/media/MediaTaskQueue.h
@@ +126,5 @@
>  
>    // Monitor that protects the queue and mIsRunning;
>    Monitor mQueueMonitor;
>  
>    struct TaskQueueEntry {

We can just get rid of this class which has a single member.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8593033 [details] [diff] [review]
Part 4 - Change the canonical internal smart pointer type in MediaTaskQueue to the XPCOM style. v1

Review of attachment 8593033 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaTaskQueue.h
@@ +40,5 @@
>    static MediaTaskQueue* GetCurrentQueue() { return sCurrentQueueTLS.get(); }
>  
>    explicit MediaTaskQueue(TemporaryRef<SharedThreadPool> aPool, bool aRequireTailDispatch = false);
>  
> +  nsresult Dispatch(TemporaryRef<nsIRunnable> aRunnable)

Will this change to already_AddRefed<nsIRunnable>?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8593034 [details] [diff] [review]
Part 5 - Align the failure handling of the TemporaryRef Dispatch overload with its companion. v1

Review of attachment 8593034 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaTaskQueue.cpp
@@ +112,1 @@
>    return task->WaitUntilDone();

It looks strange to me to return a void type. Does the coding guidelines talk about it?

Comment 11

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #8)

> Does this assertion mean MediaPromise should be used with reliable
> dispatcher only?

Yes.

> We can just get rid of this class which has a single member.

Good catch.(In reply to JW Wang [:jwwang] from comment #9)

> > +  nsresult Dispatch(TemporaryRef<nsIRunnable> aRunnable)
> 
> Will this change to already_AddRefed<nsIRunnable>?

The point of this overload is to support all the legacy callers that pass RefPtrs rather than ns{COM,Ref}Ptrs. We should fix those at some point, but I wanted to keep this patch small-ish.

(In reply to JW Wang [:jwwang] from comment #10)
> ::: dom/media/MediaTaskQueue.cpp
> @@ +112,1 @@
> >    return task->WaitUntilDone();
> 
> It looks strange to me to return a void type. Does the coding guidelines
> talk about it?

Good catch - fixed.

Comment 12

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/86e83c1fbaea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9b6d32d83f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/25d4f90ff250
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/178e696b5691
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0651f3983932

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86e83c1fbaea
https://hg.mozilla.org/mozilla-central/rev/4e9b6d32d83f
https://hg.mozilla.org/mozilla-central/rev/25d4f90ff250
https://hg.mozilla.org/mozilla-central/rev/178e696b5691
https://hg.mozilla.org/mozilla-central/rev/0651f3983932