881587 - Optimize the AudioNodeEngine.cpp routines added in bug 815643

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[(no longer active)]

Follow-up from bug 877662.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Add SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r=

Comment 2

[Paul Adenot (:padenot)]

Attachment: callgrind output

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 761247 [details] [diff] [review]
Add SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r=

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

r=me with the spec question clarified.

::: content/media/AudioNodeEngineSSE2.cpp
@@ +258,5 @@
> +  unsigned i;
> +  __m128 in0, in1, in2, in3,
> +         outreal0, outreal1, outreal2, outreal3,
> +         outimag0, outimag1, outimag2, outimag3;
> +

Can we please assert that aSize is a multiple of 8?

@@ +297,5 @@
> +
> +    _mm_store_ps(&aOutput[i], outreal0);
> +    _mm_store_ps(&aOutput[i + 4], outreal1);
> +    _mm_store_ps(&aOutput[i + 8], outreal2);
> +    _mm_store_ps(&aOutput[i + 12], outreal3);

What does the the Web Audio spec say about the expected result of the real-valued product (1 + 0*i)*(Inf + 0*i)? The above will return (Inf + NaN*i) instead of the real number result Inf + 0*i as one might expect (and the NaN will spread to the real values in subsequent operations).

@@ +313,5 @@
> +  acc0 = _mm_setzero_ps();
> +  acc1 = _mm_setzero_ps();
> +  acc2 = _mm_setzero_ps();
> +  acc3 = _mm_setzero_ps();
> +

Can we please assert that aLength is a multiple of 16?

Comment 4

[Maire Reavy [:mreavy]]

Making a P1 since early results (attached to the bug this came from: bug 877662) showed this could really help with perf.  We should reconsider the priority (drop to P2) if we think the perf win isn't going to be big enough.

Comment 5

[Maire Reavy [:mreavy]]

Karl -- Paul is clearing his plate to focus on bug 848954. Like with bug 877662, can you also pick this one up?   Plus, see Comment 4.  If you think this should be lowered to a P2, I'm fine with that.  Thanks.

Comment 6

[Maire Reavy [:mreavy]]

Per email discussion with Karl, lowering this to P2.

Comment 7

[Maire Reavy [:mreavy]]

Hi Karl, Just to be extra clear, I'd also love to have this fixed by end of Q3, but after talking with Paul, I (and Paul) feel it's less important than bug 1172997.  (That's what the rank is meant to indicate -- this is a 15, and 1172997 is a 10.)

Comment 8

[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 877662.

Comment 9

[Paul Adenot (:padenot)]

I was thinking of picking this up on the side. I've written a piece of code a while back that can help aligning the buffers, which is what was left to do here.

Comment 10

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 881587 - Add SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r=tterribe

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

Comment 11

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 881587 - Use SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r?padenot

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

Comment 12

[Dan Minor [:dminor]]

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

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8741698 [details]
MozReview Request: Bug 881587 - Use SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r?padenot

https://reviewboard.mozilla.org/r/46689/#review43291

Thanks !

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e719cc5de7b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1da60432e41

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/e719cc5de7b7
https://hg.mozilla.org/mozilla-central/rev/b1da60432e41