968016 - Use SharedThreadPools for media decode and state machine threads

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We can change to using SharedThreadPools for the decoder and state machine threads.

* This is the first major step towards running our decoders asynchronously, and reducing the number of threads we need.
* This means we won't spin up and shutdown decode threads every time a decoder fills its audio and video MediaQueues. The thread pool naturally keeps its thread alive for 60s after they become idle. Constantly creating and destroying decode threads showed up in a profile in bug 965761.
* With this, we don't need to use the StateMachineTracker to track decode threads, as this functionality is handled by the SharedThreadPool.
* We'll use a SharedThreadPool for the decode threads. It will have a thread limit of 25. This means we'll have the same basic behaviour as is currently provided by the StateMachineTracker's "request decode thread" functionality.

Comment 1

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

Attachment: Patch 1v1: Init state machine in MediaSourceDecoder

We'll need to setup the thread pools in MediaDecoderStateMachine::Init(), otherwise we won't have any threads to decode with. So MediaSourceDecoder will need to call MediaDecoderStateMachine::Init() on its startup path, else it won't have any threads to decode with.

So copy the regular MediaDecoder::Load() sequence and create and init the state machine in MediaSourceDecoder::Load().

Comment 2

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

Attachment: Patch 2v1: Specify the number of threads in a SharedThreadPool

Add the ability to specify the number of threads in a SharedThreadPool.

We want this so that we can use a SharedThreadPool of size 1 thread to use as the state machine thread. This means we can get the singletonness of the state machine thread automatically managed by the SharedThreadPool infrastructure.

Comment 3

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

Attachment: Patch 3v1: Implement nsIThreadPool::IsCurrentThreadOn()

The media code has assertions throughout to ensure code is running on the threads we think it's supposed to be running on. We'll want to keep this behaviour when we switch to using thread pools instead of hordes of individually managed threads.

The easy way is to implement nsIThreadPool::IsCurrentThreadOn(). I see there's a stern looking comment there saying we need to think carefully about what nsIThreadPool::IsCurrentThreadOn() is supposed to mean... Since we'll only be using this in assertions in debug builds, I'm happy to make the implementation #ifdef DEBUG if we don't want the implementation being exposed in release builds.

Comment 4

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

Attachment: Patch 4v1: Implement MediaTaskQueue::IsCurrentThreadIn() and expose nsIEventTarget::IsCurrentThreadOn() on SharedThreadPool

Implement MediaTaskQueue::IsCurrentThreadIn() and expose nsIEventTarget::IsCurrentThreadIn() on the SharedThreadPool, so the MediaDecoderStateMachine and friends can use these in MediaDecoderStateMachine::OnDecodeThread() etc.

Comment 5

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

Attachment: Patch 5v1: Use SharedThreadPool for decode threads

Use MediaTaskQueue, running on a SharedThreadPool for decode threads.

We basically just dispatch a runnable to call MediaDecoderStateMachine::DecodeThreadRun() on the task queue, instead of creating a new thread every time we want to run this function. Once DecodeThreadRun() returns (because our MediaQueues are full, or whatever) we'll free up a thread for the next decoder. This matches the existing behaviour provided by the StateMachineTracker.

Comment 6

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

Attachment: Patch 6v1: Use SharedThreadPool of size 1 for state machine thread

* Instead of having the StateMachineTracker enforce the singletoness of the state machine thread, use a SharedThreadPool of size 1. We now don't need custom threadsafe singleton code for the state machine thread.
* Change MediaDecoderStateMachine::GetStateMachineThread() to be non-static and return an nsIEventTarget from the state machine thread' SharedThreadPool.
* Can remove StateMachineTracker completely now.
* We don't need to track the state machine thread in the MediaShutdownManager. I will however add code in a later patch to enforce that all SharedThreadPools have been destroyed before our XPCOM shutdown listener exits however.

Comment 7

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

Attachment: Patch 7v1: Add threadStackSize attribute to nsIThreadPool

Add an attribute to nsIThreadPool to set the reserved stack size of its threads.

Media threads set their stack size, as we've had problems with running out of address space in the past, so we need this.

Comment 8

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

Attachment: Patch 8v1: Ensure all SharedThreadPools are shutdown on xpcom shutdown

Spin the event loop in our xpcom-shutdown observer until all the SharedThreadPools have completely shutdown. We can spin because we shutdown the hashtable of SharedThreadPools in an event dispatched to the main thread.

Comment 9

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

Greenish so far:
https://tbpl.mozilla.org/?tree=Try&rev=e2d0b269bcfc

Comment 10

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

Note these patches are based on my just-landed patches in bug 962385.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8370537 [details] [diff] [review]
Patch 5v1: Use SharedThreadPool for decode threads

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1184,5 @@
> +
> +  nsresult rv = mReader->Init(cloneReader);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;

The old code seemed more succinct.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8370535 [details] [diff] [review]
Patch 3v1: Implement nsIThreadPool::IsCurrentThreadOn()

I don't mind having the implementation in release builds, but because it's a lock and a loop it's going to be slow, so we should try not to actually use it in release builds.

Comment 13

[Benjamin Smedberg]

Comment on attachment 8370540 [details] [diff] [review]
Patch 7v1: Add threadStackSize attribute to nsIThreadPool

It seems a little strange to me that SetThreadStackSize is entering the monitor but GetThreadStackSize isn't and we're using mStackSize value outside the monitor in nsThreadPool::PutEvent.

I do think we have to synchronize it, unless we're going to make it an init-only property. And in that case we should either consistently use the monitor or use an Atomic. The monitor is better in this case.

Comment 14

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

Attachment: Patch 7v2: Add threadStackSize attribute to nsIThreadPool

With more synchronization, via the monitor as suggested.

Comment 15

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

Try:
https://tbpl.mozilla.org/?tree=Try&rev=1db91ce7fe2a

Comment 16

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

(In reply to Chris Pearce (:cpearce) from comment #15)
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=1db91ce7fe2a

B2G ICS Emulator timeout:
WARNING: waitpid failed pid:723 errno:10: file ../../../gecko/ipc/chromium/src/base/process_uB2GRunner TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_standalone.html | application timed out after 330.0 seconds with no output

Comment 17

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

There is two problems here:
1. I stopped calling MediaDecoderReader::OnDecodeThreadStart() and OnDecodeThreadEnd(). We should still do that for B2G's benefit.
2. I think I need to wait for the decode task queue to be idle before calling MediaDecoderReader::ReleaseMediaResources() in MediaDecoderStateMachine::RunStateMachine()'s DORMANT case. I'm already doing this when I call ReleaseMediaResources() in RunStateMachine()'s SHUTDOWN case, and all the crashes I've seen are crashing in the call to ReleaseMediaResources() in RunStateMachine()'s DORMANT case where I didn't do this.

Let's see:
https://tbpl.mozilla.org/?tree=Try&rev=b4b841419093

Comment 18

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

Attachment: Patch 9: Call OnDecodeThreadStart/Finish in DecodeLoop

We need to do this to ensure we save power on B2G devices. Not needed any more technically on Windows (except in the webaudio case) but won't hurt.

Comment 19

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

Attachment: Patch 10: Await decode task completion in DORMANT state

Await decode task completion when entering DORMANT state. This matches the behaviour of the SHUTDOWN state machine case, and prevents the failure on B2G emulator mochitest chunk 3 & 4.

Comment 20

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

Once more, because I'm paranoid: https://tbpl.mozilla.org/?tree=Try&rev=f899e2e2f9ee

Comment 21

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

(In reply to Chris Pearce (:cpearce) from comment #20)
> Once more, because I'm paranoid:
> https://tbpl.mozilla.org/?tree=Try&rev=f899e2e2f9ee

Turns out my paranoia is justified. There's still a fairly frequent failure in this push on Linux mtest-1:

8600 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug448534.html | Test timed out.
8605 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug463162.xhtml | Test timed out.
8611 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug495145.html | Test timed out.
8617 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug495300.html | Test timed out.

I have a patch to fix it in bug 973379. Once that lands, this can land.

Comment 22

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

The remaining orange should have been fixed by bug 973379 landing a few minutes ago.

Landed on m-i for Firefox 30.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f78e7bb808c
https://hg.mozilla.org/integration/mozilla-inbound/rev/406b7fde02e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71ac8b9f7de
https://hg.mozilla.org/integration/mozilla-inbound/rev/7102981f5c72
https://hg.mozilla.org/integration/mozilla-inbound/rev/891419fc6190
https://hg.mozilla.org/integration/mozilla-inbound/rev/654700db7152
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c760d0a1a3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba0ec65173f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c582be4d20
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d80d79b817

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f78e7bb808c
https://hg.mozilla.org/mozilla-central/rev/406b7fde02e2
https://hg.mozilla.org/mozilla-central/rev/d71ac8b9f7de
https://hg.mozilla.org/mozilla-central/rev/7102981f5c72
https://hg.mozilla.org/mozilla-central/rev/891419fc6190
https://hg.mozilla.org/mozilla-central/rev/654700db7152
https://hg.mozilla.org/mozilla-central/rev/2c760d0a1a3b
https://hg.mozilla.org/mozilla-central/rev/4ba0ec65173f
https://hg.mozilla.org/mozilla-central/rev/c1c582be4d20
https://hg.mozilla.org/mozilla-central/rev/c9d80d79b817