Make tail dispatch automatic on threads that require it

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla40
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
When I originally implemented this setup, I rejected the idea of automatic tail dispatch for a couple of reasons, which I've outlined (and responded to) below.

After working on main-thread tail dispatch (bug 1154802), I've convinced myself that we should do this. If we want any hope of getting our dispatch semantics right, we already need all the relevant machinery to assert that we're doing it properly. And I've run into more and more cases that need something akin to MaybeTailDispatch.

Responses to my previous objections are as follows:

> (1) We couldn't do this for anything that uses XPCOM task dispatch (i.e. NS_DispatchToMainThread).

We can just forbid using XPCOM thread dispatch and require the use of AbstractThread in the code we care about.

> (2) For the main-thread case, we would never be able to tail dispatch everything.

Indeed. But we _can_ tail dispatch everything that we dispatch to an AbstractThread. The main benefit of tail dispatch is coherency from the perspective of a particular dispatch target - so all we really need to require is that dispatching to a MediaTaskQueue always uses tail dispatch.

> (3) Dealing with the pretzel case of avoiding an automatic tail-dispatch while tail-dispatching would be gross.

This turned out to be pretty straightforward. I have machinery to solve it.

> (4) We'd pay the cost of a TLS lookup on every dispatch.

I've convinced myself that this is probably insignificant.

> (5) Secretly converting regular dispatch into tail dispatch would be surprising for programmers - it's good to force programmers to be explicit.

Brendan once called this BDSM programming, and I'm inclined to agree in this case: if the only valid way to do such a dispatch is via tail dispatch, we should do it automatically.

There may still be snakes in the grass here that I haven't uncovered yet. Try will tell.
(Assignee)

Comment 2

3 years ago
Created attachment 8593038 [details] [diff] [review]
Part 1 - Hoist the GetCurrent TLS logic into AbstractThread. v1
Attachment #8593038 - Flags: review?(jwwang)
(Assignee)

Comment 3

3 years ago
Created attachment 8593039 [details] [diff] [review]
Part 2 - Rejigger tail dispatching assertions. v1
Attachment #8593039 - Flags: review?(jwwang)
(Assignee)

Comment 4

3 years ago
Created attachment 8593040 [details] [diff] [review]
Part 3 - Do tail dispatch automatically. v1
Attachment #8593040 - Flags: review?(jwwang)
(Assignee)

Comment 5

3 years ago
Created attachment 8593041 [details] [diff] [review]
Part 4 - Remove MaybeTailDispatch. v1

This is now the default behavior.
Attachment #8593041 - Flags: review?(jwwang)
(Assignee)

Comment 6

3 years ago
Created attachment 8593042 [details] [diff] [review]
Part 5 - Stop manually passing TaskDispatchers everywhere. v1
(Assignee)

Comment 7

3 years ago
Created attachment 8593048 [details] [diff] [review]
Part 6 - Remove unused AbstractThread::InTailDispatch. v1

This is unnecessary now that we pass this explicitly during dispatch.
Attachment #8593048 - Flags: review?(jwwang)
Comment on attachment 8593038 [details] [diff] [review]
Part 1 - Hoist the GetCurrent TLS logic into AbstractThread. v1

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

::: dom/media/AbstractThread.cpp
@@ +38,5 @@
>  
>    virtual bool IsCurrentThreadIn() override
>    {
>      bool in = NS_GetCurrentThread() == mTarget;
> +    MOZ_ASSERT_IF(in, GetCurrent() == this);

Do it hold that |NS_GetCurrentThread() == mTarget| iff |GetCurrent() == this|?

@@ +44,5 @@
>    }
>  
> +  virtual TaskDispatcher& TailDispatcher() override { MOZ_CRASH("Not implemented!"); }
> +
> +  virtual bool InTailDispatch() override { MOZ_CRASH("Not implemented!"); }

Isn't it a compile error to return nothing?

::: dom/media/AbstractThread.h
@@ +68,5 @@
> +  virtual bool InTailDispatch() = 0;
> +
> +  // Returns true if this task queue requires all dispatches performed by its
> +  // tasks to go through the tail dispatcher.
> +  bool RequiresTailDispatch() { return mRequireTailDispatch; }

const.

@@ +100,5 @@
> +  static ThreadLocal<AbstractThread*> sCurrentThreadTLS;
> +
> +  // True if we want to require that every task dispatched from tasks running in
> +  // this queue go through our queue's tail dispatcher.
> +  bool mRequireTailDispatch;

const.
Attachment #8593038 - Flags: review?(jwwang) → review+
Comment on attachment 8593039 [details] [diff] [review]
Part 2 - Rejigger tail dispatching assertions. v1

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

::: dom/media/TaskDispatcher.h
@@ +151,5 @@
>    nsTArray<UniquePtr<PerThreadTaskGroup>> mTaskGroups;
> +
> +  // True if this TaskDispatcher represents the tail dispatcher for the thread
> +  // upon which it runs.
> +  bool mIsTailDispatcher;

const.
Attachment #8593039 - Flags: review?(jwwang) → review+
Attachment #8593040 - Flags: review?(jwwang) → review+
Comment on attachment 8593041 [details] [diff] [review]
Part 4 - Remove MaybeTailDispatch. v1

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

Nice!
Attachment #8593041 - Flags: review?(jwwang) → review+
Comment on attachment 8593042 [details] [diff] [review]
Part 5 - Stop manually passing TaskDispatchers everywhere. v1

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

Great!
Attachment #8593042 - Flags: review+
Attachment #8593048 - Flags: review?(jwwang) → review+
Now I see TailDispatch resembles the stable state in Javascript.
(Assignee)

Comment 13

3 years ago
(In reply to JW Wang [:jwwang] from comment #8)
> >      bool in = NS_GetCurrentThread() == mTarget;
> > +    MOZ_ASSERT_IF(in, GetCurrent() == this);
> Do it hold that |NS_GetCurrentThread() == mTarget| iff |GetCurrent() ==
> this|?

Do you mean we should switch this to |MOZ_ASSERT(in == (GetCurrent() == this))|? That definitely seems reasonable. I'd like to check for weird edge-cases with another try push, so I'll bundle that fix into bug 1154802.
> Isn't it a compile error to return nothing?

MOZ_CRASH includes MOZ_NoReturn, which makes this ok.


> const.

> const.

Fixed.
(Assignee)

Comment 14

3 years ago
(In reply to JW Wang [:jwwang] from comment #12)
> Now I see TailDispatch resembles the stable state in Javascript.

Precisely! And that's exactly how I'm implementing tail dispatchers for the main thread in bug 1154802. :-)
(Assignee)

Updated

3 years ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 16

3 years ago
Followup bustage for a conflict on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d314c9da98f
(Assignee)

Updated

3 years ago
Blocks: 1155268
You need to log in before you can comment on or make changes to this bug.