1173016 - Optimize PeriodicWave initialization

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

We should:
- Make the size of the iFFT depend on the sampling rate. For now, we're generating way too much partials, and that's wasteful.
- Consider caching the resulting tables. It's a common pattern create a lot of OscillatorNode, and they should ideally share their (internal) PeriodicWaves created for the basic waveforms
- Consider using only linear interpolation for the curves wave tables. That should be good enough.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Cache the basic waveform PeriodicWaves. r=

I need to find a good way to manager the lifetime of those, they can be pretty
big so there is no way I'm just letting them in a global. I'm thinking maybe I
can shove those PeriodicWaves cache on the AudioContext.

I also created an new benchmark for this. Without this patch, we are around 50x
realtime. With this patch, we are around 130x realtime, so that's a 260%
speedup. Chome clocks in at about 150x or so.

Note that this patch does not include the optimization where we tailor the size
of the FFT to the number of partials we want, depending on the context's
AudioContext. We are doing FFTs of size 4096 for now, we could do 2048 without
changing the sound (the second half of the partials are above nyquist, but we
still do a big FFT). I'm still doing the maths to make sure I'm not doing
something crazy, though.

Finally, this also does not include the bilinear -> linear change.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Cache the basic waveform PeriodicWaves. r=

This implements the real stuff. The cache is refcounted and on the AudioContext
and OscillatorNodeEngines. This is fine because they get created and destroyed
on the main thread.

The cache gets used on the MSG thread (both populating the cache and using the
entries).

I'll do the bit about computing only the right partials in another bug. Also, we
are doing a linear interpolation, I mis-read what our code was doing when I
wrote the other comment.

Comment 3

[Paul Adenot (:padenot)]

I also have written a benchmark that will go in our benchmark suite so we don't regress this.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8622534 [details] [diff] [review]
Cache the basic waveform PeriodicWaves. r=

>+BasicWaveFormCache* AudioContext::GetBasicWaveFormCache()

Function name at beginning of a new line please.

>+BasicWaveFormCache::~BasicWaveFormCache() {
>+  MOZ_ASSERT(NS_IsMainThread());
>+}

>+class BasicWaveFormCache
>+{
>+public:
>+  BasicWaveFormCache(uint32_t aSampleRate);
>+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BasicWaveFormCache)

>+  // Cache to avoid recomputing basic waveforms all the time. This is
>+  // addrefed/released by the OscillatorNodeEngine on the main thread and then
>+  // used from the MSG thread.
>+  nsRefPtr<BasicWaveFormCache> mBasicWaveFormCache;

This addrefed/release comment is probably better moved to the
BasicWaveFormCache class definition as it describes the use of the class in
general rather than describing access to this variable.  I would look first at the class declaration when looking for such documentation.

If only addrefed/released on the main thread, then BasicWaveFormCache() does
not need thread-safe ref-counting.  However, see next comment.

>+  ~OscillatorNodeEngine() {
>+    MOZ_ASSERT(NS_IsMainThread());
>   }

AudioNodeEngines are owned by AudioNodeStreams, the base class of
which also has NS_INLINE_DECL_THREADSAFE_REFCOUNTING().

How do you know the last reference will always be released on the main thread
and why is this important?

This release code expects streams to be destroyed on the graph thread:
https://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/dom/media/MediaStreamGraph.cpp#l150

>-      case TYPE:
>+      case TYPE: {

>+      }

Unnecessary new brace.

Comment 5

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #4)
> >+  ~OscillatorNodeEngine() {
> >+    MOZ_ASSERT(NS_IsMainThread());
> >   }
> 
> AudioNodeEngines are owned by AudioNodeStreams, the base class of
> which also has NS_INLINE_DECL_THREADSAFE_REFCOUNTING().
> 
> How do you know the last reference will always be released on the main thread
> and why is this important?

It's not very important since we ref-count in a thread-safe way. I think I just got confused by the other patch about destroying the OscillatorNode and AudioBufferSourceNode MediaStreams early.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Cache the basic waveform PeriodicWaves. r=

This should be better.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8625166 [details] [diff] [review]
Cache the basic waveform PeriodicWaves. r=

>+    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PeriodicWave);
>+
>     static PeriodicWave* createSine(float sampleRate);
>     static PeriodicWave* createSquare(float sampleRate);
>     static PeriodicWave* createSawtooth(float sampleRate);
>     static PeriodicWave* createTriangle(float sampleRate);
> 
>     // Creates an arbitrary periodic wave given the frequency components
>     // (Fourier coefficients).
>     static PeriodicWave* create(float sampleRate,

Can you make the create methods return already_AddRefed please, to make these leak-safe?

That can be a separate patch or follow-up.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Properly refcount the inner PeriodicWave object. r=

We need to rename the PeriodicWave object imported from Blink, othewise our leak
detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave
object are the same, and is confused about having a different size.

This is because the macros to implement AddRef and Release only exist in an
"inline" version, and only takes the name of the class, without caring about the
namespace.

I don't like PeriodicWaveInternal too much, though. Considering that we might
end up rewriting the files we borrowed from Blink at some point, to really have
a separate implementation, it might not be important.

This adds already_AddRefed, and MOZ_COUNT_{C,D}TOR.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8629341 [details] [diff] [review]
Properly refcount the inner PeriodicWave object. r=

>We need to rename the PeriodicWave object imported from Blink, othewise our leak
>detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave
>object are the same, and is confused about having a different size.
>
>This is because the macros to implement AddRef and Release only exist in an
>"inline" version, and only takes the name of the class, without caring about the
>namespace.

Have you tried adding the namespace explicitly:
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebCore::PeriodicWave)?
That's probably preferable to the renames if it works.

>-PeriodicWave::PeriodicWave(float sampleRate)
>-    : m_sampleRate(sampleRate)
>-    , m_periodicWaveSize(PeriodicWaveSize)
>-    , m_numberOfRanges(NumberOfRanges)
>-    , m_centsPerRange(CentsPerRange)
>+PeriodicWaveInternal::PeriodicWaveInternal(float sampleRate)
>+  : m_sampleRate(sampleRate)
>+  , m_periodicWaveSize(PeriodicWaveInternalSize)
>+  , m_numberOfRanges(NumberOfRanges)
>+  , m_centsPerRange(CentsPerRange)

This file has 4 space indentation, so the previous form was consistent.

>This adds already_AddRefed, and MOZ_COUNT_{C,D}TOR.

>+    MOZ_COUNT_CTOR(PeriodicWaveInternal);

MOZ_COUNT_CTOR is not usually used in ref-counted classes because it and
NS_INLINE_DECL_THREADSAFE_REFCOUNTING will each count so bloat counts and any
leak counts will be double.

Not having MOZ_COUNT_CTOR does risk an undetected leak if the object is never
AddRef()ed, but the private constructor and already_AddRefed factory methods
now protect against that.

I think it's probably better to stick with convention and drop MOZ_COUNT_CTOR.

See comment at
https://hg.mozilla.org/mozilla-central/annotate/136c41fca853/xpcom/glue/nsISupportsImpl.h#l167

Comment 10

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #9)
> Comment on attachment 8629341 [details] [diff] [review]
> Properly refcount the inner PeriodicWave object. r=
> 
> >We need to rename the PeriodicWave object imported from Blink, othewise our leak
> >detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave
> >object are the same, and is confused about having a different size.
> >
> >This is because the macros to implement AddRef and Release only exist in an
> >"inline" version, and only takes the name of the class, without caring about the
> >namespace.
> 
> Have you tried adding the namespace explicitly:
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebCore::PeriodicWave)?
> That's probably preferable to the renames if it works.

I'm not sure how I misread the macro in the first place, but just adding the namespace works.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/146b55675c93
https://hg.mozilla.org/integration/mozilla-inbound/rev/911c598a9ef2

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01d7e27c5833

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/146b55675c93
https://hg.mozilla.org/mozilla-central/rev/911c598a9ef2
https://hg.mozilla.org/mozilla-central/rev/01d7e27c5833