[AUDIO_WORKLET] Added support for MEMORY64 and 2GB heap (including tests) #23508
Conversation
We can remove the float-by-float JS copy and replace with this simple TypedArray set() calls.
Typed views are recreated if needed but otherwise are reused.
Lots of juggling with the various pointers, and next will be to reduce the code and move all of the output first to stop repeating some of the calculations. Some can also move to the constructor.
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.
All the AW tests are silently failing. I'll look at fixing them next. |
Done. |
This mostly adds an Audio Worklet parameter test, but also tidies a little the related tests and shared code.
|
Note for me to look at ( 
|
This mostly adds an Audio Worklet parameter test, but also tidies a little the related tests and shared code.
This mostly adds an Audio Worklet parameter test, but also tidies a little the related tests and shared code.
…tests) (#23659) This is splits out the test changes from #23508 adding an interactive parameter test: ``` test/runner interactive.test_audio_worklet_params_mixing ``` This test is also added to the browser tests now #23665 has merged, offering a single test that touches all the various structs and their offsets. When run as part of the `browser` tests it plays for 1 second, for interactive without the `TEST_AND_EXIT` macro it will play continuously. It also tidies a little the shared code (and related tests) in the process. Interactive tests had `_2gb` and `_4gb` options added (which will fail until #23508 lands, but they're not part of the CI). All features of these tests as interactive now work fully on Chrome, Firefox and Safari (Safari has issues with looping, but it doesn't affect the spirit of the test).
| @@ -193,14 +273,9 @@ class BootstrapMessages extends AudioWorkletProcessor { | |||
| // 'cb' the callback function | |||
| // 'ch' the context handle | |||
| // 'ud' the passed user data | |||
| p.postMessage({'_wsc': d['cb'], 'x': [d['ch'], 1/*EM_TRUE*/, d['ud']] }); | |||
| p.postMessage({'_wsc': {{{ toIndexType("d['cb']") }}}, 'x': [d['ch'], 1/*EM_TRUE*/, {{{ toIndexType("d['ud']") }}}] }); | |||
There was a problem hiding this comment.
Are the changes on this line necessary? I would hope that toIndexType would only be needed on lines that directly call wasm table operations. i.e. only for wasmTable.get, etc.
There was a problem hiding this comment.
Originally the type conversions were in the calling code, here for example:
emscripten/src/lib/libwebaudio.js
Line 269 in 0c3df92
But you suggested moving them to where they're needed. Without these it results in type errors, e.g.: TypeError: Cannot convert 0 to a BigInt.
| } | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This file sseems to have changed a lot more than I would expect just for the memory64 change. Am I missing something?
There was a problem hiding this comment.
It's built on #22753 from last October that was never merged, which contained, besides the performance improvements, the groundwork for the struct offsets which made 2GB and wasm64 support straightforward. The diff between the old PR and this isn't that large, with most of the work going into the tests (which went into their own #23659).
There was a problem hiding this comment.
I'll break this down into a series of smaller PRs on the current main, e.g. move to the C_STRUCTS offsets and makeGetValue, then introduce wasm64, then the performance changes. I couldn't review in its current state what I wrote myself.
I don't know when this will be though since we're shipping it and it's done what I set out to do (we have millions of users putting in hour long sessions per day, so a few millis per second saved is a big deal on low-end school hardware when juggling 3D content with audio, and if it didn't work we'd really know about it!).
There was a problem hiding this comment.
We appreciate all the work you have done here.
I'm excited to see these changes lands. If you can find time to spit split the PR that would be amazing. If not, we can circle back in a few weeks.
|
Note for me to come back to. These test look to have broken when merged with #23108: They break here with the bigint error: Line 477 in 2e14c6d Called from here: emscripten/src/lib/libwasm_worker.js Line 101 in 2e14c6d Probably the switch to The |
This is built on #22753 and uses the
_2gband_4gbtests (plus the parameter test) from #23659.It adds support for
MEMORY64to audio worklets and tests that:MEMORY64and >4GB heapThe tests already prove out various buffer combinations using the
INITIAL_MEMORYandGLOBAL_BASEflags.For the code changes, besides parameters expecting
BigIntit also needed any shifts removing from the offset calculations (signed shifts fail >2GB, unsigned >4GB).The interactive tests (from #23659) can be run with: