974089 - Web Audio CPU usage climbs during long waits for garbage collection

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Karl Tomlinson (:karlt)]

1. Load https://dl.dropboxusercontent.com/u/40949268/Bugs/beep.html
2. Select sampling rate of 44100 or 48000, trying to guess native rate but it
   is not critical to reproducing.
   (Leave sample frame size at 2048.)
3. Wait for 10 or 20 seconds (for GCs to finish).
4. Click "start audio"
5. Observe CPU use of "MediaStreamGrph" thread over a minute
   (If on a platform supporting "top", type 'H' for threads.)

Desired results:
CPU use remains consistent (and low).

Actual results:
CPU use climbs for about a minute and then falls back to a stable level.
Here it may climb to over 60% before falling back to 2.6 to 3.6%.

Forcing CC from a preloaded about:memory in another window collects no objects
and has no effect on cpu.
Forcing GC causes CC to collect objects and the cpu usage falls.
Switching tabs also seems to cause objects to be collected and bring cpu down.

Comment 1

[Karl Tomlinson (:karlt)]

Each Web Audio node will usually consume cpu for as long as it lives.
Many pages create many nodes (many node types are designed as one-shot nodes.)
The test page is pretty much as simple as it gets.  More complex audio graphs
will run out of available cpu and stutter.

I assume GC is tuned for memory pressure, not cpu resources.
I think we need to stop depending on GC for cleaning up unnecessary streams.
For nodes that can be reconnected (not one-shot source nodes) this may involve
nodes recreating streams after they are destroyed.  Another option might be to
remove the stream from the graph and add later if required.

This is a variation on bug 897796 comment 30:

1. Passive streams without input are destroyed.
   This includes streams with a node.

2. Newly connected source nodes will propagate (re)creation of streams for
   their output nodes.

3. Streams without node or output are destroyed.
   This includes active streams.

4. Occasional cycle collection on the graph collects cycles without nodes or
   outputs to nodes (sinks), and cycles without inputs from active streams.

1 and 2 can be implemented separately in this bug and 3 and 4 in bug 897796.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: destroy stream when a source finishes

This is a proof of concept, which is effective only for one-shot source nodes.
More complex graphs will have other nodes that will contribute to cpu usage
but are not cleaned up here.

In the simple example here, it reduces MSG CPU usage to a stable 1.2%.  Even
if GC is running frequently, this is still a reduction to less than half.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: try LikelyShortLivingObjectCreated()

This was a failed attempt to trigger GC more frequently.

sLikelyShortLivingObjectsNeedingGC reached 13100 on 
http://www.cappel-nord.de/webaudio/acid-defender/
and 24200 on http://webaudiodemos.appspot.com/MIDIDrums/index.html
before GC, which was well after MSG CPU reached 100%.

At least, with this change, forcing a CC when
sLikelyShortLivingObjectsNeedingGC > 2500 leads to a GC, but there is nothing
to trigger a CC before we get the GC we need, at that seems to be on a 1 minute timer.

Comment 4

[Maire Reavy [:mreavy]]

Hey Karl -- We've gotten to a point where we really need this bug fixed for Web Audio perf.  It's affecting some important demos, and it's particularly noticeable now because we've solved so many other perf issues.  Will you able able to start back on this bug this week?

Comment 5

[Paul Adenot (:padenot)]

I'm running the first patch, rebased, on try because it implements what's we need for now.

It was deadlocking because we were running messages on the main thread during shut down, and then it was crashing because we would try to update a stream that was already destroyed.

Third try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17257d47548

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: destroy WebAudio MediaStream when a source finishes

as unbitrotted by padenot

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix deadlocks in destroying streams in MSG due to patch 1

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix MSG threading issues with stream destruction on finish

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=664fd91f7700

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8620801 [details] [diff] [review]
fix deadlocks in destroying streams in MSG due to patch 1

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

::: dom/media/MediaStreamGraph.cpp
@@ +128,5 @@
>    SetStreamOrderDirty();
>  }
>  
>  void
> +MediaStreamGraphImpl::RemoveStreamUnlocked(MediaStream* aStream)

Let's not clone the entire routine and just omit one line; that's a pain for maintenance.

Instead, pass a bool aLock to control the locking.  Autolocks are a pain when conditional so we have to duplicate the for() loop.

@@ +1298,5 @@
>      mStreamUpdates.SetCapacity(mStreamUpdates.Length() + mStreams.Length());
>      for (uint32_t i = 0; i < mStreams.Length(); ++i) {
>        MediaStream* stream = mStreams[i];
> +      if (!stream->MainThreadNeedsUpdates() ||
> +          stream->IsDestroyed()) {

Mainthread only (see follow-on patch; testing mGraph is a valid replacement to see if it's been destroyed on the MSG thread).

@@ +2125,5 @@
> +    {
> +      mStream->RemoveAllListenersImpl();
> +      auto graph = mStream->GraphImpl();
> +      mStream->DestroyImpl();
> +      graph->RemoveStreamUnlocked(mStream);

let's add a comment about why this is needed

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8620799 [details] [diff] [review]
destroy WebAudio MediaStream when a source finishes

Apply verbal r+ from padenot

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e511c79f2211
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6a7d5efe3a

Comment 12

[Maire Reavy [:mreavy]]

To test on spark, you can grab builds here once their done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67a26d625497

This fix should be in tomorrow's Nightly.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcfd15bb387

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing OSX/Windows test_bug867104.html permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=10668595&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/42c0f3d737f1

Comment 15

[Maire Reavy [:mreavy]]

Hi Harald - we got tripped up by a race condition in windows & mac and got backed out (working on fixing it), but you should be able to use the try build in comment 12 to verify that this will resolve the WebAudio game issues for you.  Can you test using that build and let us know how it works for you?

Thanks!

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8620801 [details] [diff] [review]
fix deadlocks in destroying streams in MSG due to patch 1

1dcfd15bb387 should be enough to fix the original deadlock from calling back
into the graph during NotifyMainThreadStreamFinished(), which was already on
the main thread.

I don't think this patch is necessary.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4c3379ecb3

Comment 18

[Karl Tomlinson (:karlt)]

I'll open a new bug about destroying and re-creating streams for processing (not just source) nodes.

Comment 19

[Maire Reavy [:mreavy]]

With Karl's landing last night (thanks to Karl, Padenot and Jesup -- a great team effort!), the improvements needed for the games demos are on inbound, and it looks they have stuck (i.e. they won't get backed out).  I expect a merge over to central very soon.

Harald -- Since it's Friday, can you verify today that the remaining issues with the demos are resolved?  Padenot tested the fixes, and it all looks good to him, but I know you were able to reliably repro the problems -- and some of the problems we had trouble repro'iing.  Please feel free to reach out to me or Padenot on irc (we're in #games) if you have any questions or concerns.  Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d4c3379ecb3

Comment 21

[:Harald Kirschner :digitarald]

Can't test currently, Angry Bots is getting OOM killed during loading in the latest dogfooding version on Aries.

Comment 22

[:Harald Kirschner :digitarald]

Still running crash, filed bug 1182600