If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Optimize the AudioNodeEngine.cpp routines added in bug 815643

RESOLVED FIXED in Firefox 48

Status

()

Core
Web Audio
P1
normal
Rank:
15
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: Ehsan, Assigned: dminor)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla48
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [blocking-webaudio-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Follow-up from bug 877662.
Created attachment 761247 [details] [diff] [review]
Add SSE2 version of AudioNodeEngine.cpp routines added in bug 815643. r=
Attachment #761247 - Flags: review?(tterribe)
Created attachment 761248 [details]
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-]

Updated

4 years ago
Whiteboard: [blocking-webaudio-] → [blocking-webaudio-][webaudio-perf]

Updated

4 years ago
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
Depends on: 877662

Updated

2 years ago
Blocks: 1169293
Priority: P2 → P1
Rank: 15
Blocks: 1189514
No longer blocks: 1169293
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.
Blocks: 1205458
(Assignee)

Updated

2 years ago
Assignee: nobody → dminor
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

2 years ago
Created attachment 8741697 [details]
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/
Attachment #8741698 - Flags: review?(padenot)
(Assignee)

Comment 11

2 years ago
Created attachment 8741698 [details]
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/
(Assignee)

Comment 12

2 years ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e25e545520275421a40aef1314930772bb5dfbda
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 14

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

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e719cc5de7b7
https://hg.mozilla.org/mozilla-central/rev/b1da60432e41
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

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