1151656 - Build a mechanism to coalesce runnable dispatch to the tail end of a task

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

This is a prerequisite to bug 1144481 to get the necessary atomicity when inspecting mirrored state. I'd originally rolled this work into that bug, but I think it's tricky enough that it's good milestone all on its own.

Comment 1

[Bobby Holley (:bholley)]

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbff16cd9779

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add some more assertions. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop doing sync dispatch of PlaybackEnded. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Do static initialization explicitly. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement the ability to get the currently running task queue. v1

I think this will generally be useful for asserting things. If we're concerned
about the TLS overhead we could make it debug-only, but my sense is that it's
negligible.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement TaskDispatcher.h. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Implement a post-run TaskDispatcher for MediaTaskQueue. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 7 - Implement the ability to assert tail dispatch. v1

In order to make sure that the MDSM properly dispatches everything via tail
dispatch, we want verification that is more robust than simple inspection.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 8 - Make MediaPromises operate with TaskDispatchers. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 9 - Use TailDispatch for the MDSM's task queue. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 9 - Use TailDispatch for the MDSM's task queue. v2

The green try push above didn't include a couple of last-minute modifications
to part 9. Another try push [1] did include them, and I noticed an occasional
assertion failure from failed dispatch on the audio thread. Annotated that
assertion not to fire, since presumably this has always been failing.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7667e9c0d13e

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8589996 [details] [diff] [review]
Part 4 - Implement the ability to get the currently running task queue. v1

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

::: dom/media/MediaTaskQueue.h
@@ +34,5 @@
> +  static void InitStatics();
> +
> +  // Returns the task queue that the caller is currently running in, or null
> +  // if the caller is not running in a MediaTaskQueue.
> +  static MediaTaskQueue* CurrentQueueOrNull() { return sCurrentQueueTLS.get(); }

Any particular reason for this naming? The normal convention is that GetFoo() can return nullptr, and Foo() is infallible.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Any particular reason for this naming? The normal convention is that
> GetFoo() can return nullptr, and Foo() is infallible.

It's a JS/XPConnect convention, used to indicate that the method cannot fail, but that one of the valid values is null. If you think it's too incongruous, I can switch it to GetCurrentQueue.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8589997 [details] [diff] [review]
Part 5 - Implement TaskDispatcher.h. v1

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

::: dom/media/TaskDispatcher.h
@@ +24,5 @@
> + * there are sometimes reasons why we might want to delay the actual dispatch of
> + * these tasks until a specified moment. At present, there are two reasons to do
> + * this:
> + *
> + * XXXbholley - Finish me!

Yes bobby, please do :)

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Bobby Holley (:bholley) from comment #13)
> 
> It's a JS/XPConnect convention, used to indicate that the method cannot
> fail, but that one of the valid values is null. If you think it's too
> incongruous, I can switch it to GetCurrentQueue.

Ok, I haven't seen that much in layout/media/gfx code. I'd prefer GetCurrentQueue(), but it's not a big deal.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Ok, I haven't seen that much in layout/media/gfx code. I'd prefer
> GetCurrentQueue(), but it's not a big deal.

Sure thing.

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Yes bobby, please do :)

Oops. ;-)

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8590006 [details] [diff] [review]
Part 9 - Use TailDispatch for the MDSM's task queue. v2

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

It's a little annoying that we have to change all the call sites (and thus rely on 100% code coverage during debug tests to be sure that we got them all), but I don't have any better ideas.

Comment 18

[Bobby Holley (:bholley)]

Thanks for the reviews matt!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe38ca71a1d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e72d7aa5f4e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/defb154dbaa8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94128d385667
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6ee34874d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd5e9c300a80
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b43eca6d969
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b257d977271
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c22476cabca

Comment 19

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

Comment on attachment 8589996 [details] [diff] [review]
Part 4 - Implement the ability to get the currently running task queue. v1

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

::: dom/media/AbstractThread.cpp
@@ +28,5 @@
>  bool
>  AbstractThreadImpl<nsIThread>::IsCurrentThreadIn()
>  {
> +  bool in = NS_GetCurrentThread() == mTarget;
> +  MOZ_ASSERT_IF(in, MediaTaskQueue::CurrentQueueOrNull() == nullptr);

How does MediaTaskQueue come into play when AbstractThread is concerned here?

::: dom/media/MediaTaskQueue.cpp
@@ +206,5 @@
>  MediaTaskQueue::IsCurrentThreadIn()
>  {
>    MonitorAutoLock mon(mQueueMonitor);
> +  bool in = NS_GetCurrentThread() == mRunningThread;
> +  MOZ_ASSERT_IF(in, CurrentQueueOrNull() == this);

This looks suspicious to me. If we exit the funciton at [1], next runnable (not those dispatched to MediaTaskQueue) in the same thread will see |NS_GetCurrentThread() == mRunningThread| while CurrentQueueOrNull() would return null.

[1] https://hg.mozilla.org/mozilla-central/file/2c92a7df87c9/dom/media/MediaTaskQueue.cpp#l214

Comment 20

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

Comment on attachment 8589997 [details] [diff] [review]
Part 5 - Implement TaskDispatcher.h. v1

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

::: dom/media/TaskDispatcher.h
@@ +132,5 @@
> +
> +  PerThreadTaskGroup& EnsureTaskGroup(AbstractThread* aThread)
> +  {
> +    for (size_t i = 0; i < mTaskGroups.Length(); ++i) {
> +      if (mTaskGroups[i]->mThread == aThread) {

This comparison is not reliable. Atomicy will be broken if we accidentially have 2 AbstractThread instances associated with the same nsIThread such as the main thread.

Comment 21

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

Comment on attachment 8590000 [details] [diff] [review]
Part 7 - Implement the ability to assert tail dispatch. v1

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

::: dom/media/AbstractThread.cpp
@@ +19,5 @@
>  template<>
>  nsresult
>  AbstractThreadImpl<nsIThread>::Dispatch(already_AddRefed<nsIRunnable> aRunnable)
>  {
> +  MediaTaskQueue::AssertInTailDispatchIfNeeded();

Why this check here? Shouldn't it be done in mTarget->Dispatch() if mTarget happens to be a MediaTaskQueue.

Comment 22

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

Comment on attachment 8590001 [details] [diff] [review]
Part 8 - Make MediaPromises operate with TaskDispatchers. v1

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

::: dom/media/MediaPromise.h
@@ +319,5 @@
> +    // target for race bugs. So we do an extra assertion here to make sure our
> +    // caller is using tail dispatch correctly no matter what, rather than
> +    // relying on the assertion in Dispatch(), which may be called extremely
> +    // infrequently.
> +    aDispatcher.AssertIsTailDispatcherIfRequired();

Can this be done outside the lock?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe38ca71a1d4
https://hg.mozilla.org/mozilla-central/rev/e72d7aa5f4e3
https://hg.mozilla.org/mozilla-central/rev/defb154dbaa8
https://hg.mozilla.org/mozilla-central/rev/94128d385667
https://hg.mozilla.org/mozilla-central/rev/1c6ee34874d8
https://hg.mozilla.org/mozilla-central/rev/fd5e9c300a80
https://hg.mozilla.org/mozilla-central/rev/9b43eca6d969
https://hg.mozilla.org/mozilla-central/rev/8b257d977271
https://hg.mozilla.org/mozilla-central/rev/8c22476cabca

Comment 24

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #19)
> Comment on attachment 8589996 [details] [diff] [review]
> Part 4 - Implement the ability to get the currently running task queue. v1
> 
> Review of attachment 8589996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/AbstractThread.cpp
> @@ +28,5 @@
> >  bool
> >  AbstractThreadImpl<nsIThread>::IsCurrentThreadIn()
> >  {
> > +  bool in = NS_GetCurrentThread() == mTarget;
> > +  MOZ_ASSERT_IF(in, MediaTaskQueue::CurrentQueueOrNull() == nullptr);
> 
> How does MediaTaskQueue come into play when AbstractThread is concerned here?

This is just a useful way to assert that MediaTaskQueue::CurrentQueueOrNull (which has since been renamed MediaTaskQueue::GetCurrentQueue) returns null when we're on the main thread.


> This looks suspicious to me. If we exit the funciton at [1], next runnable
> (not those dispatched to MediaTaskQueue) in the same thread will see
> |NS_GetCurrentThread() == mRunningThread| while CurrentQueueOrNull() would
> return null.
> 
> [1] https://hg.mozilla.org/mozilla-central/file/2c92a7df87c9/dom/media/MediaTaskQueue.cpp#l214

Yeah, seems like we should just make the management of mRunningThread safer.

(In reply to JW Wang [:jwwang] from comment #20) 
> This comparison is not reliable. Atomicy will be broken if we accidentially
> have 2 AbstractThread instances associated with the same nsIThread such as
> the main thread.

Good catch! I think this is enough of a footgun that we should just remove ::Create and allow == checks. We can revisit this problem if we ever need an nsIThread other than the main thread.

(In reply to JW Wang [:jwwang] from comment #21)
> Why this check here? Shouldn't it be done in mTarget->Dispatch() if mTarget
> happens to be a MediaTaskQueue.

No. This check is about the caller, not the dispatch target. We need to enforce this property whenever the MDSM task queue dispatches something, even if the dispatch target is the main thread.

(In reply to JW Wang [:jwwang] from comment #22)
> Can this be done outside the lock?

Good idea.

I'll file a followup bug to make these improvements.