1300118 - consider adding a class to help throttle runnable dispatch to threads

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Over in bug 1003730 I ran into another case where we can flood the main thread with runnables.  I think we could relatively easily solve this by adding a "throttled dispatcher" class that could be used in place of NS_DispatchToMainThread().  This could also likely be used in places like postMessage(), setTimeout(), etc.

If we had full event queue bucketing we could probably do this in a more general way.  Until that comes, however, it seems like we could possibly mitigate flooding in some scenarios with a helper class.  Might be worth doing if we can keep the complexity down.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

AbstractThread's TailDispatch concept might already implement what I want here.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I think I can implement what I want with a new factory method like AbstractThread::MainThread().  I want a separate bucket of tail dispatching instead of the single static bucket MainThread() currently provides.  Should be straightforward.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Err... no. TailDispatch() is not what I was thinking of.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

After further investigation I think TaskQueue is almost what I want.  It just needs to be able to target runnables at a single thread instead of a thread pool.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1.patch

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p2.patch

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1.patch

Better patch using nsIEventTarget.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p2.patch

Expose a TaskQueue on both nsPIDOMWindow and WorkerPrivate.  In general we try to have a unique TaskQueue per top level window (ScriptableTop) or top-level detached worker (ServiceWorker).

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p2.patch

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p3.patch

This makes WorkerRunnable objects that dispatch to the main thread go through the WorkerPrivate->MainThreadTaskQueue().  This covers both the console and postMessage cases AFAICT.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-hax.patch

Some temporary hacks I had to make to get dom/workers/test/test_console.html to pass.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I'm having trouble with the patches on my window build at the moment, but in my linux VM this seems to keep the browser interactive while running bug 1003730 comment 0.  Flushing the built up messages after the window is closed might be tricky for shutdown, though.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I think we may want to make TaskQueue implement nsIEventTarget.  That would let us do things like pass the MainThreadTaskQueue to nsITimer::SetTarget(), etc.

Comment 15

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

See bug 1144487 comment 4.

Btw, there is a pitfall in the interface of nsITimer::SetTarget() where you have a footgun when passing an nsIThreadPool which doesn't guarantee events to run in sequence.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] from comment #15)
> See bug 1144487 comment 4.
> 
> Btw, there is a pitfall in the interface of nsITimer::SetTarget() where you
> have a footgun when passing an nsIThreadPool which doesn't guarantee events
> to run in sequence.

Not sure I follow.  That comment doesn't discuss nsITimer.  I'm not proposing to use a thread pool here.  Sorry if I'm confused.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1.patch

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1-1.patch

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1-2.patch

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1-3.patch

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p2.patch

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p3.patch

These are the patches from the last try build.  URL duplicated here for convenience:

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

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p1-1.patch

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p2.patch

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

The current set of patches allow us to handle the test case in bug 1003730 without losing interactivity on the main thread.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-p4.patch

This patch makes the window setTimeout() and setInterval() use the MainThreadTaskQueue() as well.  This allows us to keep the main thread interactive with test cases like this:

  function boom() { setTimeout(boom, 0); setTimeout(boom, 0); }
  boom();

If you run this in content window devtools it basically buries the main thread in the child process today.  It prevents any tab using that child process from doing anything.

With this patch the child process main thread remains responsive.  That tab with the problem code may have delays some times, but other tabs can be opened and used without impact.

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

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Worker throttling only:

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

Worker + window setTimeout throttling:

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

There is an intermittent in dom/workers/test/test_consoleAndBlobs.html that I still need to figure out.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0200402e2660
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fbf3785d3fa

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Focusing on the worker throttling for now since the window TaskQueue has a shutdown issue I need to investigate further:

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

Andrea helped me possibly figure out the test_consoleAndBlobs problem.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make TaskQueue implement nsIEventTarget. r=bholley

Bobbey, I'm not quite ready to flag for review, but I wanted to get your opinion on this one.  I want to be able to reference these TaskQueue objects as nsIEventTarget interface pointers.  This will let me tie them into the rest of our thread ecosystem.  For example, I can pass it to nsITimer::SetTarget(), etc.

Personally I believe AbstractThread should probably implement nsIEventTarget.  This seems natural and the code is leaning that way already.  See the API points that exist today that offer both AbstractThread and nsIEventTarget signatures that do the same thing.

I have the impression you don't want AbstractThread to implement nsIEventTarget, though.  So as a compromise I've made TaskQueue implement both AbstractThread and nsIEventTarget itself.

The alternatives seem to be (in order of my preference):

1) Make AbstractThread implement nsIEventTarget (what I would prefer)
2) This patch.
3) Create an nsIEventTarget wrapper around TaskQueue
4) Implement a completely separate queue mechanism using nsIEventTarget
5) roto-till the entire tree to use AbstractThread (god, no)

Personally I'm not sure I understand why it was necessary to create a parallel API using AbstractThread instead of building on nsIEventTarget.

What do you think?  Thanks!

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make TaskQueue allow nested runnables. r=bholley

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make nsThread expect NS_DISPATCH_TAIL similar to nsThreadPool. r=bholley

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Expose the TaskQueue backlog Length(). r=bholley

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 8788291 [details] [diff] [review]
P2 Make TaskQueue implement nsIEventTarget. r=bholley

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

So, all the AsAbstractThread() calls in this patch are definitely gross.

The main problems I see with inheriting nsIEventTarget are:
* Refcounting becomes virtual. This is probably ok.
* we get a name collision with the existing Dispatch() method, which has slightly different semantics.
* We can end up with the silly situation where we're using an XPCOMThreadWrapper as an nsIEventTarget rather than the underlying object.

I think the best solution is to have an AsEventTarget method on AbstractThread which returns an nsIEventTarget equivalent. For the XPCOM thread wrapper this unwraps, which brings us back to the question of what to do about TaskQueue. I think that this approach in patch is probably the right way to go, modulo the ambiguity stuff, which I think could be solved by using a private base class along with AsEventTarget (which could do the cast internally).

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788289 [details] [diff] [review]
P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley

That makes TaskQueue work with any nsIEventTarget instead of just SharedThreadPool.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788292 [details] [diff] [review]
P3 Make TaskQueue allow nested runnables. r=bholley

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

The assertions previously assumed that only a single TaskQueue or AbstractThread could be involved on the stack.  This is no longer true.  For example, we can chain TaskQueues together to create a hierarchy of buckets:

  TQ1 writes to main thread
  TQ2 writes to TQ1
  TQ3 writes to TQ1

So if TQ2 goes crazy it does not impact the responsiveness of the code using TQ3.

The code in this patch is necessary to allow the assertions and whatnot to work with this kind of nesting.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788293 [details] [diff] [review]
P4 Make nsThread expect NS_DISPATCH_TAIL similar to nsThreadPool. r=bholley

This basically updates nsThread to expect the new dispatch flag just like:

https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadPool.cpp#273

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788294 [details] [diff] [review]
P5 Expose the TaskQueue backlog Length(). r=bholley

This exposes the TaskQueue backlog as Length().  I use this in the Worker thread run loop to apply back-pressure.  Effectively:

  if (taskQueue->Length() > 5000) {
    taskQueue->AwaitIdle();
  }

This prevents the worker thread from racing ahead, filling the TaskQueue, and OOMing the process.

Comment 45

[Bobby Holley (:bholley)]

Comment on attachment 8788293 [details] [diff] [review]
P4 Make nsThread expect NS_DISPATCH_TAIL similar to nsThreadPool. r=bholley

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

r=me with the rename patch I requested on top of these patches.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make TaskQueue implement nsIEventTarget. r=bholley

Ok, this does the private inheritence we discussed in IRC.  I still had to do a little bit of ambiguity fixing in SyncRunnable since those APIs are often passed a RefPtr<> which still has problems automatically casting with the private inheritence.  The other callsites were addressed, though.

Comment 47

[Bobby Holley (:bholley)]

Comment on attachment 8788294 [details] [diff] [review]
P5 Expose the TaskQueue backlog Length(). r=bholley

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

::: xpcom/threads/TaskQueue.h
@@ +73,5 @@
>    // executing.
>    void AwaitShutdownAndIdle();
>  
>    bool IsEmpty();
> +  uint32_t Length();

I'm assuming you basically just want this for coarse-grained heuristics? Please call this RaceyLength() or ImpreciseLengthForHeuristics() or something similar to indicate that the value can be wrong by the time the caller gets it.

Comment 48

[Bobby Holley (:bholley)]

Comment on attachment 8788476 [details] [diff] [review]
P2 Make TaskQueue implement nsIEventTarget. r=bholley

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

r=me with those fixes, but FTLOG write that rename patch.

::: xpcom/threads/SyncRunnable.h
@@ +91,5 @@
>  
> +  void DispatchToThread(TaskQueue* aThread, bool aForceDispatch = false)
> +  {
> +    DispatchToThread(aThread->AsEventTarget());
> +  }

Hm, do you actually need this? Seems pretty wrong to throw the argument away...

::: xpcom/threads/TaskQueue.cpp
@@ +164,5 @@
> +TaskQueue::Dispatch(already_AddRefed<nsIRunnable> aEvent, uint32_t aFlags)
> +{
> +  nsCOMPtr<nsIRunnable> runnable = aEvent;
> +  DispatchReason reason = aFlags == DISPATCH_TAIL ? TailDispatch
> +                                                  : NormalDispatch;

This is a totally nonsensical mixing meanings of 'tail dispatch', and precisely the reason I asked for the rename. You should pass NormalDispatch and ignore aFlags.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788476 [details] [diff] [review]
P2 Make TaskQueue implement nsIEventTarget. r=bholley

Actually, I'll have to work on this a bit more.  It still has the previous ambiguity problems.  I was confused reading my compiler output.  Sorry.

Comment 50

[Bobby Holley (:bholley)]

Comment on attachment 8788292 [details] [diff] [review]
P3 Make TaskQueue allow nested runnables. r=bholley

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

This patch is too much to review while I'm tired. Bouncing to jww.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley

The rename patch you requested.  I used the name roc indicated he would use in that other bug.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley

Rebased on top of the s/NS_DISPATCH_TAIL/NS_DISPATCH_AT_END/g rename.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make TaskQueue implement nsIEventTarget. r=bholley

Bobby, I think the best solution for now is to just give TaskQueue a WrapAsEventTarget() method.  If we end up with too many wrappers we can refactor this to remove the wrappers.  This works for my immediate purposes and well let me proceed.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make nsThread expect NS_DISPATCH_AT_END similar to nsThreadPool. r=bholley

Rebased on NS_DISPATCH_TAIL rename.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #47)
> I'm assuming you basically just want this for coarse-grained heuristics?
> Please call this RaceyLength() or ImpreciseLengthForHeuristics() or
> something similar to indicate that the value can be wrong by the time the
> caller gets it.

Its no more racey than IsEmpty(), but I'll rename it.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Expose the TaskQueue backlog ImpreciseLengthForHeuristics(). r=bholley

Addressed review feedback.

I'll push a new try after I finish rebasing bug 1300658 on these changes.  Later tonight or tomorrow morning.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

These patches are included in this try:

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

Comment 58

[Bobby Holley (:bholley)]

Comment on attachment 8788625 [details] [diff] [review]
P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley

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

r=me with that. Thanks!

::: xpcom/threads/SharedThreadPool.h
@@ +57,5 @@
>  
>    // Call this when dispatching from an event on the same
>    // threadpool that is about to complete. We should not create a new thread
>    // in that case since a thread is about to become idle.
> +  nsresult TailDispatch(nsIRunnable *event) { return Dispatch(event, NS_DISPATCH_AT_END); }

Please rename this to DispatchFromEndOfTaskInThisPool or something like that.

Comment 59

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

(In reply to Ben Kelly [:bkelly] from comment #42)
> Comment on attachment 8788292 [details] [diff] [review]
> P3 Make TaskQueue allow nested runnables. r=bholley
> 
> Review of attachment 8788292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The assertions previously assumed that only a single TaskQueue or
> AbstractThread could be involved on the stack.  This is no longer true.  For
> example, we can chain TaskQueues together to create a hierarchy of buckets:
> 
>   TQ1 writes to main thread
>   TQ2 writes to TQ1
>   TQ3 writes to TQ1
> 
> So if TQ2 goes crazy it does not impact the responsiveness of the code using
> TQ3.
> 
> The code in this patch is necessary to allow the assertions and whatnot to
> work with this kind of nesting.

I don't understand the example here. Can you elaborate more details?

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #59)
> I don't understand the example here. Can you elaborate more details?

Sure.

In bug 1300658 and bug 1300659 I am using TaskQueue to throttle runnables to the main thread.  For example, consider this worker script:

  while(true) { console.log('foo') }

This will generate console runnable objects faster than the main thread can consume them.  The main thread becomes so clogged that the browser becomes unresponsive.

By putting a TaskQueue in the dispatch patch I ensure that the real main thread only sees a single Runnable at a time.  The TaskQueue doesn't dispatch the next runnable until the previous one completes.  This allows things like painting, JS timers, etc to run responsively even though the worker is going crazy.

The next step beyond this is to compose multiple TaskQueue objects together.  For example, consider a window with some setTimeout() callbacks running and a Worker.  We can aggregate these together under the window by chaining the TaskQueue objects.  So:

  Worker TaskQueue targets the Window TaskQueue
  Timer TaskQueue targets the Window TaskQueue
  Window TaskQueue targets the main thread

By "target" I mean we pass the object to the TaskQueue() constructor.  (Patch p1 lets us target nsIEventTarget instead of just SharedThreadPool.)

So with this example with 3 TaskQueues we could have the Worker spamming console.log() as above, but also have the window doing something evil with timers like this:

  function boom() { setTimeout(boom, 0); setTimeout(boom, 0); }
  boom();

And the window would still remain responsive.  We could then send things like postMessage() to the main window TaskQueue.  Or maybe later we would decide to add a new TaskQueue for postMessage as well.  The advantage of this design is that its flexible and we can change it easily over time.

The reason I need the P3 patch is that aggregating TaskQueue objects like this breaks some overly strict assertions in the current code.  Previously TaskQueue only ever wrote to a SharedThreadPool, so it could be locked down to only nsIThread oriented things.

I'll comment on the patch directly why each change is needed.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

(By the way, sorry for the very long description.  I figured I would err on the side of too much information due to the timezone difference.  I'm going to bed shortly.)

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788292 [details] [diff] [review]
P3 Make TaskQueue allow nested runnables. r=bholley

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

::: xpcom/threads/AbstractThread.cpp
@@ -69,5 @@
>      // NS_GetCurrentThread starts returning null.
>      PRThread* thread = nullptr;
>      mTarget->GetPRThread(&thread);
>      bool in = PR_GetCurrentThread() == thread;
> -    MOZ_ASSERT(in == (GetCurrent() == this));

This assert seems somewhat bogus today, but breaks faster with my proposed changes.

It works today if you do:

  AbstractThread::MainThread()->IsCurrentThreadIn();

If, however, you do this on the main thread it will assert:

  MOZ_ASSERT(NS_IsMainThread());
  nsCOMPtr<nsIThread> target = NS_GetCurrentThread();
  RefPtr<AbstractThread> at = AbstractThread::CreateXPCOMThreadWrapper(target, false);
  at->IsCurrentThreadIn();

This works in the first case because AbstractThread::InitStatics() does this:

  https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/AbstractThread.cpp#142

But that assumes you only ever touch the main thread from an AbstractThread using that sMainThread static.  The second case asserts because I've made an AbstractThread targeting the main thread thats not sMainThread.

I think that makes this assert pretty dubious.

Now that we are going to let TaskQueue target the main thread, this assertion is going to get broken a lot more.  I believe I hit it during local testing or in a try build.

::: xpcom/threads/TaskQueue.h
@@ +146,5 @@
>        // might require it.
>        MOZ_ASSERT(!mQueue->mTailDispatcher);
>        mQueue->mTailDispatcher = this;
>  
> +      mLastCurrentThread = sCurrentThreadTLS.get();

This change is necessary because the previous code always overwrote the current thread TLS value and simply restored nullptr.  If you have a nested set of AbstractThread objects like chain TaskQueues, then this will break.  We will lose the previous AbstractThread.

The changes I make here chain the current thread values.  So if TaskQueue A calls into TaskQueue B, B will save A as the mLastCurrentThread.  It will then set B as the current thread.  When B finishes with the runnable it will restore A as the current thread.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Do comment 60 and comment 61 help at all?

I think we should probably remove this assertion for similar reasons as well:

  https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TaskQueue.cpp#143

I didn't explicitly hit this assertion but I think it could be triggered with a nested TaskQueue.  Something like:

  RefPtr<TaskQueue> tq1 = new TaskQueue(targetThread);
  nsCOMPtr<nsIEventTarget> tq1Target = tq1->WrapAsEventTarget();
  RefPtr<TaskQueue> tq2 = new TaskQueue(tq1Target);

  // If runnable calls tq1->IsCurrentThreadIn() everything is fine
  // If runnable calls tq2->IsCurrentThreadIn() then it will assert
  tq2->Dispatch(runnable);

What do you think?

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang

Updated the patch description to better match what the code is doing.  I also removed the assertion I mentioned in the previous patch.

Comment 65

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

Comment on attachment 8788629 [details] [diff] [review]
P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley

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

::: xpcom/threads/TaskQueue.h
@@ +31,5 @@
>  // They may be executed on different threads, and a memory barrier is used
>  // to make this threadsafe for objects that aren't already threadsafe.
>  class TaskQueue : public AbstractThread {
>  public:
> +  explicit TaskQueue(already_AddRefed<nsIEventTarget> aPool,

s/aPool/aTarget/

Comment 66

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

I do something like:

  RefPtr<TaskQueue> tq1 = new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK));
  RefPtr<TaskQueue> tq2 = new TaskQueue(tq1->WrapAsEventTarget());
  tq2->Dispatch(NS_NewRunnableFunction([] () {
    printf("running some task\n");
  }));

And it fails the assertion at [1].

I wonder if we really want to allow TaskQueue chaining which makes things really complicated.

Or what we need is something like:

  RefPtr<TaskQueue> tq1 = new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK));
  RefPtr<TaskQueue> tq2 = new TaskQueue(tq1->GetUnderlyingEventTarget());

[1] https://hg.mozilla.org/mozilla-central/file/4ee5ddeeee281b9b6f564fc56efb1713da8d7eac/xpcom/threads/TaskQueue.h#l135

Comment 67

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

(In reply to Ben Kelly [:bkelly] from comment #60)
> In bug 1300658 and bug 1300659 I am using TaskQueue to throttle runnables to
> the main thread.  For example, consider this worker script:
> 
>   while(true) { console.log('foo') }
> 
> This will generate console runnable objects faster than the main thread can
> consume them.  The main thread becomes so clogged that the browser becomes
> unresponsive.
> 
> By putting a TaskQueue in the dispatch patch I ensure that the real main
> thread only sees a single Runnable at a time.  The TaskQueue doesn't
> dispatch the next runnable until the previous one completes.  This allows
> things like painting, JS timers, etc to run responsively even though the
> worker is going crazy.

The magic comes from the tail dispatching which aggregate multiple Dispatch() calls into one single runnable. If this is exactly what you need, then you don't need TaskQueue chaining provided by WrapAsEventTarget(). Instead, you just need to target multiple TaskQueues to the same nsIEventTarget like:
  
  RefPtr<TaskQueue> worker = new TaskQueue(mainThread);
  RefPtr<TaskQueue> timer = new TaskQueue(mainThread);

Comment 68

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

Btw there is some inconsistency in IsCurrentThreadIn() which needs to be fixed.

Consider:

  RefPtr<TaskQueue> tq1 = new TaskQueue(mainThread);
  RefPtr<TaskQueue> tq2 = new TaskQueue(mainThread);

  tq1.Dispatch(runnable1);
  tq2.Dispatch(runnable2);

  runnable1 sees:
  tq1.IsCurrentThreadIn() == true and tq2.IsCurrentThreadIn() == false

  runnable2 sees:
  tq1.IsCurrentThreadIn() == false and tq2.IsCurrentThreadIn() == true

It works as expected. However...

  RefPtr<AbstractThread> at1 = AbstractThread::CreateXPCOMThreadWrapper(mainThread, false);
  RefPtr<AbstractThread> at2 = AbstractThread::CreateXPCOMThreadWrapper(mainThread, false);

  at1.Dispatch(runnable1);
  at2.Dispatch(runnable2);

  runnable1 sees:
  at1.IsCurrentThreadIn() == true and at2.IsCurrentThreadIn() == true

  runnable2 sees:
  at1.IsCurrentThreadIn() == true and at2.IsCurrentThreadIn() == true

IsCurrentThreadIn() should return true only when the runnable was dispatched to the AbstractThread/TaskQueue.

Comment 69

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

(In reply to Ben Kelly [:bkelly] from comment #63)
> Do comment 60 and comment 61 help at all?
> 
> I think we should probably remove this assertion for similar reasons as well:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TaskQueue.
> cpp#143
> 
> I didn't explicitly hit this assertion but I think it could be triggered
> with a nested TaskQueue.  Something like:
> 
>   RefPtr<TaskQueue> tq1 = new TaskQueue(targetThread);
>   nsCOMPtr<nsIEventTarget> tq1Target = tq1->WrapAsEventTarget();
>   RefPtr<TaskQueue> tq2 = new TaskQueue(tq1Target);
> 
>   // If runnable calls tq1->IsCurrentThreadIn() everything is fine
>   // If runnable calls tq2->IsCurrentThreadIn() then it will assert
>   tq2->Dispatch(runnable);
> 
> What do you think?

No. tq2->IsCurrentThreadIn() is fine but tq1->IsCurrentThreadIn() fails the assertion.
However, both tq1->IsCurrentThreadIn() and tq2->IsCurrentThreadIn() return true when the assertion is removed. I wonder if that is what we expect.

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #66)
>   RefPtr<TaskQueue> tq1 = new
> TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK));
>   RefPtr<TaskQueue> tq2 = new TaskQueue(tq1->WrapAsEventTarget());
>   tq2->Dispatch(NS_NewRunnableFunction([] () {
>     printf("running some task\n");
>   }));
> 
> And it fails the assertion at [1].
> [1]
> https://hg.mozilla.org/mozilla-central/file/
> 4ee5ddeeee281b9b6f564fc56efb1713da8d7eac/xpcom/threads/TaskQueue.h#l135

Yes.  I tried to explain in comment 62 why I changed that assertion in my P3 patch.  Were you running with my P3 patch?  Because I changed that to chain the current TLS thread and restore the correct value.

That assertion would have blown up if you targeted the main thread even without nesting TaskQueues because the sMainThread sets its own sCurrentThread TLS value instead of nullptr.

> The magic comes from the tail dispatching which aggregate multiple
> Dispatch() calls into one single runnable. If this is exactly what you need,
> then you don't need TaskQueue chaining provided by WrapAsEventTarget().
> Instead, you just need to target multiple TaskQueues to the same
> nsIEventTarget like:
>   
>   RefPtr<TaskQueue> worker = new TaskQueue(mainThread);
>   RefPtr<TaskQueue> timer = new TaskQueue(mainThread);

No, I explicitly do not want to run multiple Dispatch calls in a single runnable.  I talked with bholley extensively on IRC about if TailDispatch is what I wanted and he told me its not.  See:

 http://logs.glob.uno/?c=mozilla%23content&s=2+Sep+2016&e=2+Sep+2016&h=TailDispatch#c392226

What I want is just the basic ability to queue events until the target nsIEventTarget has completed executing a single runnable, then dispatch the next runnable.  This is TaskQueue normal behavior and unrelated to its TailDispatch.

And targeting the underlying event target does not give me the aggregation that I want.  This:

  TQ1 -> Main Thread
  TQ2 -> TQ1
  TQ3 -> TQ1

Is not the same as:

  TQ1 -> Main Thread
  TQ2 -> Main Thread
  TQ3 -> Main Thread

The ordering of events is changed by no longer aggregating the queues.  In the first case I can Dispatch some runnables to TQ1 and I know they will effectively have higher priority than TQ2 and TQ3 runnables.  For example I could dispatch 10 high priority runnables to TQ1 and know they can run consecutively without interruption.

In the second case TQ1, TQ2, and TQ3 all have the same effective priority.  Those example 10 high priority runnables might get delayed by TQ2 and TQ3 runnables getting put into the main thread.

>   RefPtr<AbstractThread> at1 =
> AbstractThread::CreateXPCOMThreadWrapper(mainThread, false);
>   RefPtr<AbstractThread> at2 =
> AbstractThread::CreateXPCOMThreadWrapper(mainThread, false);
> 
>   at1.Dispatch(runnable1);
>   at2.Dispatch(runnable2);
> 
>   runnable1 sees:
>   at1.IsCurrentThreadIn() == true and at2.IsCurrentThreadIn() == true
> 
>   runnable2 sees:
>   at1.IsCurrentThreadIn() == true and at2.IsCurrentThreadIn() == true
> 
> IsCurrentThreadIn() should return true only when the runnable was dispatched
> to the AbstractThread/TaskQueue.

This is a bug in the XPCOMThreadWrapper.  It incorrectly assumes that each instances is the only instance that will ever target a particular thread.  Its IsCurrentThreadIn() should probably compare GetCurrent() in addition to the NSPR thread:

  virtual bool IsCurrentThreadIn() override
  {
    // Compare NSPR threads so that this works after shutdown when
    // NS_GetCurrentThread starts returning null.
    PRThread* thread = nullptr;
    mTarget->GetPRThread(&thread);
    bool in = PR_GetCurrentThread() == thread &&
              GetCurrent() == this;
    return in;
  }

But this is somewhat unrelated to TaskQueue.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #69)
> No. tq2->IsCurrentThreadIn() is fine but tq1->IsCurrentThreadIn() fails the
> assertion.
> However, both tq1->IsCurrentThreadIn() and tq2->IsCurrentThreadIn() return
> true when the assertion is removed. I wonder if that is what we expect.

I think those should probably return true.  The runnable is indeed running "in" both those queues.  It was dispatched through both of them.  Both TaskQueues are on the stack when it executes.

If you want to change the IsCurrentThreadIn() to be only the most recent AbstractThread on the stack then we could use the same logic I suggested above for XPCOMThreadWrapper:

  bool
  TaskQueue::IsCurrentThreadIn()
  {
    bool in = NS_GetCurrentThread() == mRunningThread &&
              GetCurrent() == this;
    return in;
  }

What do you think?

Comment 71

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

(In reply to Ben Kelly [:bkelly] from comment #70)
> And targeting the underlying event target does not give me the aggregation
> that I want.  This:
> 
>   TQ1 -> Main Thread
>   TQ2 -> TQ1
>   TQ3 -> TQ1
> 
> Is not the same as:
> 
>   TQ1 -> Main Thread
>   TQ2 -> Main Thread
>   TQ3 -> Main Thread
> 
> The ordering of events is changed by no longer aggregating the queues.  In
> the first case I can Dispatch some runnables to TQ1 and I know they will
> effectively have higher priority than TQ2 and TQ3 runnables.  For example I
> could dispatch 10 high priority runnables to TQ1 and know they can run
> consecutively without interruption.
> 
> In the second case TQ1, TQ2, and TQ3 all have the same effective priority. 
> Those example 10 high priority runnables might get delayed by TQ2 and TQ3
> runnables getting put into the main thread.

I don't understand how the 1st case gives a higher priority to tasks dispatched to TQ1.

This is what I understand so far. Given:

 TQ1 -> Main thread
 TQ2 -> Main thread

If there is an evil thread which keeps doing:
  for (int i = 0; i < 10000; ++i) {
    TQ1.Dispatch(...);
  }

All other runnables dispatched to TQ1 by other threads will starve. However, all runnables dispatched to TQ2 will not starve. Neither are those dispatched using NS_DispatchToMainThread(). Both TQ1 and TQ2 share the same thread under the hood. However, due to the mechanics of TaskQueue, thrashing TQ1 will not stave the runnables dispatched to TQ2. Is this the 'throttling' you want?

Comment 72

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

Btw, I think we should clarify the need for chaining TaskQueue before disambiguating the semantics of IsCurrentThreadIn().

Comment 73

[Ben Kelly [:bkelly, not reviewing]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #71)
> I don't understand how the 1st case gives a higher priority to tasks
> dispatched to TQ1.
> 
> This is what I understand so far. Given:
> 
>  TQ1 -> Main thread
>  TQ2 -> Main thread
> 
> If there is an evil thread which keeps doing:
>   for (int i = 0; i < 10000; ++i) {
>     TQ1.Dispatch(...);
>   }
> 
> All other runnables dispatched to TQ1 by other threads will starve. However,
> all runnables dispatched to TQ2 will not starve. Neither are those
> dispatched using NS_DispatchToMainThread(). Both TQ1 and TQ2 share the same
> thread under the hood. However, due to the mechanics of TaskQueue, thrashing
> TQ1 will not stave the runnables dispatched to TQ2. Is this the 'throttling'
> you want?

I'll try one more time.

  TQ1 dispatches to main thread
  TQ2 dispatches to TQ1
  TQ3 dispatches to TQ3

This ensures that:
  * only one runnable from any of these queues in the main thread at once
  * only one runnable from TQ2 can be in TQ1 at any time
  * only one runnable from TQ3 can be in TQ1 at any time

This means I can dispatch 10 runnables into TQ1 if that operation is 10 times more important than TQ2 or TQ3 work.

You're right, though, I could dispatch these 10 runnables to the main thread.  This would prioritize them above TQ1 and TQ2.  But it would also prioritize them above any other work that uses the main thread.  This is exactly what we are trying to avoid!  We do NOT want to spam large numbers of runnables to the main thread because it causes jank.

Your proposed solution where TaskQueue can write to a single nsIEventTarget that is not TaskQueue does not provide what I need.  I cannot prioritize between different types of work without spamming the main thread directly.

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

See comment 73.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

And please just let me know if you would prefer to keep TaskQueue specific to its current uses.  I can pretty easily go write an nsIEventTarget class that does the same thing without any of the bits I don't need (e.g. TailDispatch, etc).  I was just trying to avoid code duplication.

Comment 76

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

I see your point. I think the design of TaskQueue chaining is elegant but difficult to comprehend. It should be helpful to put this idea in the comments of TaskQueue.h or somewhere else so it can be usable to more code.

Comment 77

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

Comment on attachment 8788751 [details] [diff] [review]
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang

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

We should have another bug to review the semantics of IsCurrentThreadIn() and write some tests to verify that.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  Sorry if I was having difficulty describing its.

I filed bug 1301411 for the IsCurrentThreadIn() issue.  I'll add a comment to TaskQueue in the P2 patch that adds the nsIEventTarget wrapper thing since that enables the chaining.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley

Updated to address last review feedback item.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley

Rebase.

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7368370ff9
P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/61813035f930
P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/0393ff1b2645
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba3a9f229cf
P4 Make nsThread expect NS_DISPATCH_AT_END similar to nsThreadPool. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f40bffecd83
P5 Expose the TaskQueue backlog ImpreciseLengthForHeuristics(). r=bholley

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

Backed out because I messages up the bug number in P2 patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fd5c60f2b6

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e40df52925d5
P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/60e82c1780e5
P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/50882fb40551
P2 Make TaskQueue implement nsIEventTarget. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd18e29b64bf
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe61ed092e8
P4 Make nsThread expect NS_DISPATCH_AT_END similar to nsThreadPool. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/24f714ad248c
P5 Expose the TaskQueue backlog ImpreciseLengthForHeuristics(). r=bholley

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for frequent xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35719398&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d2f549c288

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

I'm working the failure in bug 1300658.

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3da6680c362
P0 Rename NS_DISPATCH_TAIL to NS_DISPATCH_AT_END to avoid confusion with AbstractThread::TailDispatch. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6f6dc4543
P1 Make TaskQueue deliver runnables to nsIEventTarget. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/4844015b5fb7
P2 Make TaskQueue implement nsIEventTarget. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72b06de40c0
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8e26a69690
P4 Make nsThread expect NS_DISPATCH_AT_END similar to nsThreadPool. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cd6f6bc89e
P5 Expose the TaskQueue backlog ImpreciseLengthForHeuristics(). r=bholley

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3da6680c362
https://hg.mozilla.org/mozilla-central/rev/90a6f6dc4543
https://hg.mozilla.org/mozilla-central/rev/4844015b5fb7
https://hg.mozilla.org/mozilla-central/rev/e72b06de40c0
https://hg.mozilla.org/mozilla-central/rev/8a8e26a69690
https://hg.mozilla.org/mozilla-central/rev/f5cd6f6bc89e