-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AUDIO_WORKLET] Optimise the copy back from wasm's heap to JS #22753
base: main
Are you sure you want to change the base?
Conversation
38203f0
to
24a90e4
Compare
Nice idea! It looks like this PR carries the rename from the other PR. Rebase/merge should hide it? |
A shower thought, as all good ideas are!
Yes, this one was rebased off the earlier one so should have the same commit IDs. |
d186eb5
to
d4680ca
Compare
EDIT: every test I tried suggests no, the stack is only used once we hit Wasm in the callback. @juj or someone who knows more about the internals of this than me: on entering To rephrase: unless the audio worklet explicitly uses the stack functions from JS, nothing external from Wasm will before |
dcdcea1
to
5b65dcf
Compare
Some notes: lots of experiments with the stack allocations, minimum sizes, various flags ( Next is to benchmark it. |
c98adcf
to
2373dd2
Compare
Benchmarks of the main part of the audio copy done: https://wip.numfum.com/cw/2024-10-29/index.html Testing on my M2 Mac Studio in Chrome and Safari this PR is around 15x faster on the float copy, e.g. the original being 0.625µs per
@juj if we're in agreement that the simplified standalone JavaScript test code is doing the right thing (it's short and a copy and paste), I can gather numbers from regular hardware (we have a wall of Chromebooks at work). Next I'll need to create tests to show that this still works with various input and output configs. EDIT: a 7-12x speed-up seems typical on x64 Windows or Linux. |
|
069f7a4
to
ae0e8bf
Compare
f6153e9
to
cccece4
Compare
b3dc2ef
to
ac37140
Compare
The code has also been brought back closer to the original for comparison.
The initial stackAlloc() is overflowing, seeming to need more space so this is accounted for.
Tested with various stack sizes, output sizes, and generators.
The assertions should now cover all cases of changes in address and size of the output views.
(Off home!)
Rough implementation to see what needs doing in JS vs Wasm.
The tests pass the audio context in a void* for convenience, which needs shortening/widening for 64-bit pointers.
7d7820a
to
32a9d98
Compare
I'd really appreciate some eyes on this and getting it merged. I'm going to start the proposed API changes and further prep for the next version of the AW standard, and need to build it on these changes (and after that I want to do a single file PR like the workers, memory64 support, and others; we'll be shipping it so it needs fixing up and maintaining). |
This builds on #22741 just because that's where I was at, but it's not required. The interesting changes are in
audio_worklet.js
and I'd appreciate some feedback from @juj before tidying this up (with sanity checks and a fallback).Since we pass in the stack for the worklet from the caller's heap, its address shouldn't change. And since the (I'll make myself say it) render quantum size doesn't change after the audio worklet creation, the stack positions for the audio buffers should not change either. So, we can create one-time subarray views and replace the float-by-float copy with a simple
set()
per channel (per output).I've thrown simple tests at it at and it works, fulfilling the garbage-free requirement and theoretically having a nice performance boost (not measured, but looping over thousands of JS Number types and shuffling them to and from floats must come at a cost). If the
outputList
does change, then it should only change after changes to the audio chain, which would be expensive enough that changing the subarrays wouldn't make a difference.To be extra sure, we can move the output buffers to the first entries on the stack, then simple additional changes like input buffers won't change the address.
It wants sanity checks here and there but I'd like feedback for anything I'm missing or misunderstanding. Thanks!