Closed Bug 1151656 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(9 files, 1 obsolete file)

5.43 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.00 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.16 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.87 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.67 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.96 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.94 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
19.04 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
36.61 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
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.
Summary: Build mechanism to coalesce runnable dispatch to the tail end of a task → Build a mechanism to coalesce runnable dispatch to the tail end of a task
Depends on: 1148571
Attachment #8589993 - Flags: review?(matt.woodrow)
Attachment #8589995 - Flags: review?(matt.woodrow)
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.
Attachment #8589996 - Flags: review?(matt.woodrow)
Attachment #8589997 - Flags: review?(matt.woodrow)
In order to make sure that the MDSM properly dispatches everything via tail
dispatch, we want verification that is more robust than simple inspection.
Attachment #8590000 - Flags: review?(matt.woodrow)
Attachment #8590002 - Flags: review?(matt.woodrow)
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
Attachment #8590002 - Attachment is obsolete: true
Attachment #8590002 - Flags: review?(matt.woodrow)
Attachment #8590006 - Flags: review?(matt.woodrow)
Attachment #8589993 - Flags: review?(matt.woodrow) → review+
Attachment #8589994 - Flags: review?(matt.woodrow) → review+
Attachment #8589995 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8589996 - Flags: review?(matt.woodrow) → review+
(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 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 :)
Attachment #8589997 - Flags: review?(matt.woodrow) → review+
(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.
(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. ;-)
Attachment #8589998 - Flags: review?(matt.woodrow) → review+
Attachment #8590000 - Flags: review?(matt.woodrow) → review+
Attachment #8590001 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8590006 - Flags: review?(matt.woodrow) → review+
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 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 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 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?
(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.
Blocks: 1153370
Depends on: 1153665
See Also: → 1342420
You need to log in before you can comment on or make changes to this bug.