877662 - Optimize the AudioNodeEngine.cpp routines

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[(no longer active)]

We need SSE2 versions of those routines.  Also it would be nice to ensure that AllocateAudioBlock always allocates 16-byte aligned buffers where we have SSE2 available.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Microbenchmark

Here is a reimplementation in sse2 of all the functions we have in AudioNodeEngine.cpp (but not WriteZeroesToAudioBlock, which is essentially a memset), in the form of a independent microbenchmark with various flavor of C++, hand-unrolled C++, SSE2 intrinsics and hand unrolled SSE2 intrinsics, with some basic tests of the output.

Comment 2

[Paul Adenot (:padenot)]

Attachment: benchmark results

And some encouraging results.

Comment 3

[(no longer active)]

Nice!  BTW, I forgot to ask, do you also have NEON experience?  We should definitely optimize these routines for mobile as well...

Comment 4

[Paul Adenot (:padenot)]

No, but this is something I've been considering for some time. Now is the time look into it, I think.

Comment 5

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Align AudioBlock on 16 bytes when SSE2 is available. r=

Comment 7

[Paul Adenot (:padenot)]

Attachment: Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

derf, if you have a minute, could you have a look at the functions in
AudioNodeEngineSSE.cpp?

Comment 8

[Paul Adenot (:padenot)]

I'm also planning on adding some form of compiled test for those functions.

Comment 9

[(no longer active)]

Comment on attachment 758671 [details] [diff] [review]
Align AudioBlock on 16 bytes when SSE2 is available. r=

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

::: content/media/AudioNodeEngine.cpp
@@ +14,2 @@
>    nsRefPtr<SharedBuffer> buffer =
>      SharedBuffer::Create(WEBAUDIO_BLOCK_SIZE*aChannelCount*sizeof(float));

Please MOZ_ASSERT the alignment here.

::: content/media/SharedBuffer.h
@@ +28,5 @@
>   * Typically you would allocate one of these, fill it in, and then treat it as
>   * immutable while it's shared.
>   * This only guarantees 4-byte alignment of the data. For alignment we
>   * simply assume that the refcount is at least 4-byte aligned and its size
>   * is divisible by 4.

Please update this comment.

Comment 10

[(no longer active)]

Comment on attachment 758675 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

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

Ted, can you please do the honors for the build system stuff here?

Comment 11

[(no longer active)]

Comment on attachment 758675 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

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

::: content/media/AudioNodeEngineSSE2.cpp
@@ +38,5 @@
> +
> +    vscaled0 = _mm_mul_ps(vin0, vgain);
> +    vscaled1 = _mm_mul_ps(vin1, vgain);
> +    vscaled2 = _mm_mul_ps(vin2, vgain);
> +    vscaled3 = _mm_mul_ps(vin3, vgain);

It might make sense to have a separate path for the aScale==1.0 case, but it's fine if you want to do that in a follow-up.

::: content/media/AudioNodeStream.cpp
@@ +331,5 @@
>    AllocateAudioBlock(outputChannelCount, &aTmpChunk);
>    float silenceChannel[WEBAUDIO_BLOCK_SIZE] = {0.f};
>    // The static storage here should be 1KB, so it's fine
> +  nsAutoTArray<float, GUESS_AUDIO_CHANNELS*WEBAUDIO_BLOCK_SIZE + 15> downmixBuffer;
> +  float* alignedDownmixBuffer = ALIGN16(downmixBuffer.Elements());

Please declare this variable below, as this value here will be invalid if we end up doing a dynamic allocation.  Also, we don't need this value here anyway.

@@ +358,2 @@
>          for (uint32_t j = 0; j < outputChannelCount; ++j) {
> +          outputChannels[j] = &alignedDownmixBuffer[j * WEBAUDIO_BLOCK_SIZE];

So here we're assuming that WEBAUDIO_BLOCK_SIZE is a multiple of 16, which is fine, but please static assert that here to be explicit about it.

Comment 12

[Timothy B. Terriberry (:derf)]

Comment on attachment 758675 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

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

r=me with nits fixed.

::: content/media/AudioNodeEngineSSE2.cpp
@@ +8,5 @@
> +
> +#ifdef DEBUG
> +  #define ASSERT_ALIGNED(ptr)                                                  \
> +            MOZ_ASSERT((((uintptr_t)ptr + 15) & ~0x0F) == (uintptr_t)ptr,      \
> +                       #ptr " has to be aligned on 16 bytes.");

"16-byte aligned"

@@ +54,5 @@
> +    _mm_store_ps(&aOutput[i], vout0);
> +    _mm_store_ps(&aOutput[i + 4], vout1);
> +    _mm_store_ps(&aOutput[i + 8], vout2);
> +    _mm_store_ps(&aOutput[i + 12], vout3);
> +  }

Did you look at the compiler's output on x86-32? I worry that it will spill lots with this unrolled 4 times (since it can't prove the loads don't alias with the stores, it needs at least 9 registers, plus one because register allocation is NP-hard).

@@ +183,5 @@
> +    _mm_store_ps(in, vout0);
> +    _mm_store_ps(in+4, vout1);
> +    _mm_store_ps(in+8, vout2);
> +    _mm_store_ps(in+12, vout3);
> +    in += 16;

It's generally better for the compiler if you do array indexing (like your other functions) than if you do pointer arithmetic.

::: content/media/AudioNodeStream.cpp
@@ +10,5 @@
>  #include "ThreeDPoint.h"
>  
>  using namespace mozilla::dom;
>  
> +#define ALIGN16(ptr) (float*)(((uintptr_t)ptr + 15) & ~0x0F);

Parentheses around ptr, as well as the whole macro body, and remove the trailing ;

@@ +330,5 @@
>  
>    AllocateAudioBlock(outputChannelCount, &aTmpChunk);
>    float silenceChannel[WEBAUDIO_BLOCK_SIZE] = {0.f};
>    // The static storage here should be 1KB, so it's fine
> +  nsAutoTArray<float, GUESS_AUDIO_CHANNELS*WEBAUDIO_BLOCK_SIZE + 15> downmixBuffer;

You're allocating 15 extra floats when I think you want 15 extra bytes.

@@ +352,5 @@
>      } else if (channels.Length() > outputChannelCount) {
>        if (mChannelInterpretation == ChannelInterpretation::Speakers) {
>          nsAutoTArray<float*,GUESS_AUDIO_CHANNELS> outputChannels;
>          outputChannels.SetLength(outputChannelCount);
> +        downmixBuffer.SetLength(outputChannelCount * WEBAUDIO_BLOCK_SIZE + 15);

See above.

::: content/media/Makefile.in
@@ +26,5 @@
>  CFLAGS   += $(GSTREAMER_CFLAGS)
>  CXXFLAGS += $(GSTREAMER_CFLAGS)
> +
> +ifneq (,$(INTEL_ARCHITECTURE))
> +# gcc requires -msse2 on AudioNodeEngine.cpp since it uses SSE intrinsics.

The comment says -msse2 but the flag you're actually adding is -msse. Since you're using XMM registers, you want the former.

Also, shouldn't this be on AudioNodeEngineSSE2.$(OBJ_SUFFIX) instead?

@@ +28,5 @@
> +
> +ifneq (,$(INTEL_ARCHITECTURE))
> +# gcc requires -msse2 on AudioNodeEngine.cpp since it uses SSE intrinsics.
> +ifdef GNU_CC
> +AudioNodeEngine.$(OBJ_SUFFIX): CXXFLAGS+=-msse

The comment says -msse2 but the flag you're actually adding is -msse. Since you're using XMM registers, you want the former.

@@ +32,5 @@
> +AudioNodeEngine.$(OBJ_SUFFIX): CXXFLAGS+=-msse
> +endif
> +
> +ifdef SOLARIS_SUNPRO_CXX
> +AudioNodeEngine.$(OBJ_SUFFIX): CXXFLAGS+=-xarch=sse -xO4

Again, sse vs. sse2.

::: content/media/moz.build
@@ +43,5 @@
>  MODULE = 'content'
>  
> +# Are we targeting x86 or x64?  If so, build SSE2 files.
> +if CONFIG['INTEL_ARCHITECTURE']:
> +    DEFINES += ['-DUSE_SSE']

Unless you're planning to use gfx2d's Factory::HasSSE2() for some bizarre reason, I think it makes more sense to use xpcom's SSE.h and MOZILLA_MAY_SUPPORT_SSE2/MOZILLA_PRESUME_SSE2 (which you don't have to define yourself here).

Comment 13

[Timothy B. Terriberry (:derf)]

Comment on attachment 758675 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

Forgot to set the flag, too.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 758675 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

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

Makefile/moz.build bits look fine modulo derf's concerns.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

The previous patch did not include the SSE2 / scalar dispatch code, for some
reason. Here it is, in AudioNodeEngine.cpp. I basically followed the
recommendations at the top of SSE.h.

The unity gain fastpath is handled outside of SSE2/scalar specific code.

Comment 16

[Paul Adenot (:padenot)]

(In reply to Timothy B. Terriberry (:derf) from comment #12)
> @@ +54,5 @@
> > +    _mm_store_ps(&aOutput[i], vout0);
> > +    _mm_store_ps(&aOutput[i + 4], vout1);
> > +    _mm_store_ps(&aOutput[i + 8], vout2);
> > +    _mm_store_ps(&aOutput[i + 12], vout3);
> > +  }
> 
> Did you look at the compiler's output on x86-32? I worry that it will spill
> lots with this unrolled 4 times (since it can't prove the loads don't alias
> with the stores, it needs at least 9 registers, plus one because register
> allocation is NP-hard).

http://www.pastebin.mozilla.org/2492550, so it looks like it's OK.

Comment 17

[Paul Adenot (:padenot)]

Attachment: Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

This makes sure that we get aligned pointers even when we could borrow data from
the AudioBufferSourceNode buffer.

Comment 18

[(no longer active)]

Comment on attachment 760660 [details] [diff] [review]
Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=

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

Nice catch!

Note that I added a bunch of functions to AudioNodeEngine.cpp in bug 815643, sorry if it bitrots this patch.  I filed bug 881587 to optimize those.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 758671 [details] [diff] [review]
Align AudioBlock on 16 bytes when SSE2 is available. r=

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

::: content/media/SharedBuffer.h
@@ +35,5 @@
>  public:
> +  void* Data() {
> +    if (mozilla::supports_sse()) {
> +      // return a pointer aligned on 16 bytes.
> +      void * storage_start = (uint8_t*)(this + 1);

I think this should be unconditional. It's simpler that way, and there's pretty much no benefit to being less-aligned on non-SSE platforms.

Comment 20

[(no longer active)]

Paul, can you please address comment 19 and land these patches?

Comment 21

[Paul Adenot (:padenot)]

Some code has landed that allocate new buffers (like in the convolver node), so I need to make all that aligned, and make sure it works. But yes, I'll get to it next week.

Comment 22

[Maire Reavy [:mreavy]]

Making a P1 since early results showed it would help with perf.  If it's not going to have a big enough impact, we can drop it to P2.

Comment 23

[Maire Reavy [:mreavy]]

Karl -- Paul is clearing his plate to focus on bug 848954. Can you pick this up?   Also, see Comment 22.  If you think this should be lowered to a P2, I'm fine with that.  Thanks.

Comment 24

[Maire Reavy [:mreavy]]

Per email discussion with Karl, lowering this to P2.

Comment 25

[Paul Adenot (:padenot)]

If we want to refocus on this (and its sister bug), we really should spend some time profiling. Last time I checked, it was not clear all the processing functions were the bottlenecks, and the code has moved a lot since then.

Comment 26

[Karl Tomlinson (:karlt)]

Yes, the only method I've seen over 1% of a busy profile was BufferComplexMultiply()

Comment 27

[Maire Reavy [:mreavy]]

I talked with Karl today, and he and I agree that it'd be more efficient to find another person to work on this one.  Same with Bug 881587.

Comment 28

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 877662 - Align audio buffer allocations to 16 byte boundaries r?padenot

To be able to use SSE2 routines, we need to audio buffers to be allocated
on 16 byte boundaries.

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

Comment 29

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 877662 - Update SSE2 versions of AudioNodeEngine functions r?padenot

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

Comment 30

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 877662 - Use SSE2 versions of AudioNodeEngine functions r?padenot

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

Comment 31

[Dan Minor [:dminor]]

Recent try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=685abc5801f3edfe63b9b0e85ffa203e1fab62d5&selectedJob=19059201

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8738467 [details]
MozReview Request: Bug 877662 - Use SSE2 versions of AudioNodeEngine functions r?padenot

https://reviewboard.mozilla.org/r/44351/#review41969

::: dom/media/webaudio/blink/ReverbAccumulationBuffer.cpp:100
(Diff revision 1)
>      MOZ_ASSERT(isSafe);
>      if (!isSafe)
>          return 0;
>  
> -    AudioBufferAddWithScale(source, 1.0f, destination + writeIndex, numberOfFrames1);
> +#ifdef USE_SSE2
> +    // It is unlikely either the source is aligned or the number of values

This is not ideal, we should either:

- fall back to scalar when we have a number of items that is not a multiple of 16
- fall back to scalar if the input aligned, or process the first few elements in scalar and switch to vector and back for the last few items.

Can you file a followup for this?

Comment 33

[Paul Adenot (:padenot)]

Comment on attachment 8738465 [details]
MozReview Request: Bug 877662 - Align audio buffer allocations to 16 byte boundaries r?padenot

https://reviewboard.mozilla.org/r/44347/#review41965

::: dom/media/webaudio/AudioBlock.cpp
(Diff revision 1)
>   * buffer can reuse and modify its contents next iteration if other references
>   * are all downstream temporary references held by AudioBlock.
> - *
> - * This only guarantees 4-byte alignment of the data. For alignment we simply
> - * assume that the memory from malloc is at least 4-byte aligned and that
> - * AudioBlockBuffer's size is divisible by 4.

Can you leave a note about alignment here ?

::: dom/media/webaudio/AudioNodeExternalInputStream.cpp:91
(Diff revision 1)
>  {
>    NS_ASSERTION(aSegment->GetDuration() == WEBAUDIO_BLOCK_SIZE, "Bad segment duration");
>  
> +// To be safe, let's copy data if we're using SSE2 to ensure everything is
> +// aligned properly. Simply checking for channel alignment does not seem
> +// to be sufficient here.

Do we know why ?

::: dom/media/webaudio/BiquadFilterNode.cpp:141
(Diff revision 1)
>                      const AudioBlock& aInput,
>                      AudioBlock* aOutput,
>                      bool* aFinished) override
>    {
> -    float inputBuffer[WEBAUDIO_BLOCK_SIZE];
> +    float inputBuffer[WEBAUDIO_BLOCK_SIZE + 4];
> +    float* alignedInputBuffer = (float* )(((uintptr_t)inputBuffer + 15) & ~0x0F);

Maybe assert the alignment here ? Or maybe I'm just paranoid, do we have the guarantee that the stack is aligned in all the platforms we support ?

Comment 34

[Paul Adenot (:padenot)]

Comment on attachment 8738466 [details]
MozReview Request: Bug 877662 - Update SSE2 versions of AudioNodeEngine functions r?padenot

https://reviewboard.mozilla.org/r/44349/#review41967

::: dom/media/webaudio/AudioNodeEngineSSE2.cpp:31
(Diff revision 1)
> +void
> +AudioBufferAddWithScale_SSE(const float* aInput,
> +                            float aScale,
> +                            float* aOutput,
> +                            uint32_t aSize)
> +{

How are all those different from my initial patch ? It's a bit hard to review, can you make this sit on top of the old patches?

Comment 35

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/44347/#review41965

> Do we know why ?

I think I was accidentally only checking the alignment of the first channel. Works fine if I iterate over each channel.

Comment 36

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 877662 - Add AlignmentUtils.h r?padenot

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

Comment 37

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 877662 - Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=ehsan

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

Comment 38

[Dan Minor [:dminor]]

Comment on attachment 8738465 [details]
MozReview Request: Bug 877662 - Align audio buffer allocations to 16 byte boundaries r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44347/diff/1-2/

Comment 39

[Dan Minor [:dminor]]

Comment on attachment 8738466 [details]
MozReview Request: Bug 877662 - Update SSE2 versions of AudioNodeEngine functions r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44349/diff/1-2/

Comment 40

[Dan Minor [:dminor]]

Comment on attachment 8738467 [details]
MozReview Request: Bug 877662 - Use SSE2 versions of AudioNodeEngine functions r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44351/diff/1-2/

Comment 41

[Paul Adenot (:padenot)]

Comment on attachment 8741091 [details]
MozReview Request: Bug 877662 - Add AlignmentUtils.h r?padenot

https://reviewboard.mozilla.org/r/46183/#review42969

Comment 42

[Paul Adenot (:padenot)]

Comment on attachment 8741092 [details]
MozReview Request: Bug 877662 - Add an SSE2 implementation of AudioNodeEngine.cpp functions. r=ehsan

https://reviewboard.mozilla.org/r/46185/#review42971

I've written this, but I remember it being reviewed by someone, so I'm setting it as r+.

Comment 43

[Paul Adenot (:padenot)]

Comment on attachment 8738466 [details]
MozReview Request: Bug 877662 - Update SSE2 versions of AudioNodeEngine functions r?padenot

https://reviewboard.mozilla.org/r/44349/#review42973

Comment 44

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/46185/#review42971

ehsan did the review of your original patch.

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/826d16396107
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae01cbc5549
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e96c35c78bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1d898a440e
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f6d3815cac

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/826d16396107
https://hg.mozilla.org/mozilla-central/rev/5ae01cbc5549
https://hg.mozilla.org/mozilla-central/rev/6e96c35c78bd
https://hg.mozilla.org/mozilla-central/rev/5f1d898a440e
https://hg.mozilla.org/mozilla-central/rev/58f6d3815cac