Optimize the AudioNodeEngine.cpp routines added in bug 815643

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: dminor)

Tracking

(Blocks 1 bug, {perf})

Trunk
mozilla48
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(4 attachments)

Reporter

Description

6 years ago
Follow-up from bug 877662.
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?
Attachment #761247 - Flags: review?(tterribe) → review+
Reporter

Updated

6 years ago
Whiteboard: [blocking-webaudio-]
Whiteboard: [blocking-webaudio-] → [blocking-webaudio-][webaudio-perf]
Keywords: perf
Whiteboard: [blocking-webaudio-][webaudio-perf] → [blocking-webaudio-]
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.
Priority: -- → P1
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.
Assignee: paul → karlt
Per email discussion with Karl, lowering this to P2.
Priority: P1 → P2
Priority: P2 → P1
No longer blocks: webaudioperf
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.)
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.
Assignee: karlt → nobody
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.
Assignee

Updated

3 years ago
Assignee: nobody → dminor
Assignee

Updated

3 years ago
Status: NEW → ASSIGNED
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 !
Attachment #8741698 - Flags: review?(padenot) → review+

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e719cc5de7b7
https://hg.mozilla.org/mozilla-central/rev/b1da60432e41
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

3 years ago
Depends on: 1266772
You need to log in before you can comment on or make changes to this bug.