1273009 - [Web Audio API] CPU consumption is gradually increased when issue many AudioParam automation.

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[gaito]

Attachment: 20160516automationfirefox.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160514030209

Steps to reproduce:

1. Issue many AudioParam automation method e.g. setValueAtTime()
   (test page : http://www.g200kg.com/demo/automationtest/automationtest.html , issues 100 setValueAtTime() / sec)
2. Check the Dev Console - Performance page

## attached file is screenshots of dev console


Actual results:

Automation issuing is gradually slow down.


Expected results:

Keep the speed same as just started.

Comment 1

[Nils Ohlmeier [:drno]]

:karlt as you seem to know what this is about I leave it to you to set the priority for this appropriately.

Comment 2

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

I'm guessing P2, please revise if I'm wrong.

Comment 3

[Paul Adenot (:padenot)]

I suppose we can just prune the events on the main thread as well, I'll have a look, but I want to profile first.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1273009 - Prune AudioParam events in the main thread when inserting new events.

Running the test-case in the bug, and profiling under OSX using Instruments'
time profiler, the time spent in `AudioEventTimeline::ValidateEvent` was the
highest Web Audio API-related function. This patch makes it disappear from the
profile. We already use the same technique on the MSG thread to keep the number
of events low.

Review commit: https://reviewboard.mozilla.org/r/61206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61206/

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8766213 [details]
Bug 1273009 - Prune AudioParam events in the main thread when inserting new events.

https://reviewboard.mozilla.org/r/61206/#review58936

::: dom/media/webaudio/AudioEventTimeline.h:339
(Diff revision 1)
>    uint32_t GetEventCount() const
>    {
>      return mEvents.Length();
>    }
>  
> -private:
> +protected:

It would also be nice to keep timeline manipulation private to AudioEventTimeline.  See below.

::: dom/media/webaudio/AudioParam.cpp:165
(Diff revision 1)
>    AudioNodeStream* stream = mNode->GetStream();
>    if (stream) {
>      stream->SendTimelineEvent(mIndex, aEvent);
>    }
> +
> +  CleanupOldEvents();

I would call this from EventInsertionHelper() because it is a little surprising that a "SendEvent" method would modify the timeline, and the other callers of SendEvent (SetValue, Cancel, Stream) are not adding events that will accumulate.

::: dom/media/webaudio/AudioParam.cpp:169
(Diff revision 1)
> +
> +  CleanupOldEvents();
> +}
> +
> +void
> +AudioParam::CleanupOldEvents()

Can you move the majority of this to a CleanupEventsOlderThan(TimeType) method on AudioEventTimeline, please?
(I don't mind whether you want to keep a separate AudioParam method to get the time and make the call or just do those inline.)

That way GetValuesAtTimeHelper() can use the same code.

Bug 128138 covers the fact that it's not correct to remove older events if there is a series of setTargetAtTime events and mLastComputedValue has not yet been set, but it would be nice to be able to fix that in one place.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7577ec0b2c
Prune AudioParam events in the main thread when inserting new events. r=karlt

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/3f7577ec0b2c