Closed Bug 881587 Opened 7 years ago Closed 4 years ago

Optimize the AudioNodeEngine.cpp routines added in bug 815643

Categories

(Core :: Web Audio, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ehsan, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [blocking-webaudio-])

Attachments

(4 files)

Follow-up from bug 877662.
Attached image callgrind output
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+
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
Rank: 15
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: nobody → dminor
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+
https://hg.mozilla.org/mozilla-central/rev/e719cc5de7b7
https://hg.mozilla.org/mozilla-central/rev/b1da60432e41
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1266772
You need to log in before you can comment on or make changes to this bug.