-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[AUDIO_WORKLET] Add support for MEMORY64 with 2GB and 4GB heap #24732
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
base: main
Are you sure you want to change the base?
Conversation
These failing size related tests are unrelated to the changes:
They don't fail for me with Emscripten 4.0.12 built from The failing |
To successfully rebaseline codesize tests on main you need to first run For pre-existing codesize changes we have a bot that automatically updates them.. I'll trigger it now so that main is up-to-date. |
What's the difference between |
The The easiest way to use any emscripten checkout without emsdk binaries is to do set |
I think it’s a throwback from LLVM fixes we were doing, and it’s stuck. I’ll switch to |
Unrelated fails seem Chrome related, same as #24750 for example. (Re-run and passed) |
@sbc100 Any chance of a review of this? It's a shorter replacement for the earlier one, discussed here way back when: |
var audioParams = [], | ||
processorName = UTF8ToString({{{ makeGetValue('options', C_STRUCTS.WebAudioWorkletProcessorCreateOptions.name, '*') }}}), | ||
numAudioParams = {{{ makeGetValue('options', C_STRUCTS.WebAudioWorkletProcessorCreateOptions.numAudioParams, 'i32') }}}, | ||
audioParamDescriptors = {{{ makeGetValue('options', C_STRUCTS.WebAudioWorkletProcessorCreateOptions.audioParamDescriptors, '*') }}}, | ||
i = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we convert to one-line-per-declaration here? I think it would help make this more readable, and should not effect minified code size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happily - I just followed the original style.
this.callback = getWasmTableEntry(opts.callback); | ||
this.userData = opts.userData; | ||
this.callback = getWasmTableEntry({{{ toIndexType("opts.callback") }}}); | ||
this.userData = {{{ toIndexType("opts.userData") }}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second line doesn't look quite right to me.
userData I assume is a pointer, which means it should be Number and not a bigint (it should be converted to a Number when it first arrived on the JS side).
If it needs to be converted back to a pointer (i64) when this.callback
then you should be using to64
at the callback call side, and not here.
toIndexType
should only be used for places where you are direclty accessing the table, or the memory bounds.
Can you test with -sMEMORY64=2
as well perhaps to catch these cases where there is difference between to64
and toIndexType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's to convert it back to a pointer. It's constant, so I removed the need to keep doing the conversion.
I can test with -sMEMORY64=2
and put the conversions near the call.
(Probably not worth adding a |
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from emscripten-core#24732
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from emscripten-core#24732
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from emscripten-core#24732
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from emscripten-core#24732
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from emscripten-core#24732
The convention for JS library code is to use var for better minification. The use of `void 0` instead of `undefined` I believe is a minor code size optimization, but its somewhat obscure and (as you can see from the fact that the code expectations didn't change) it an optimization that is already performed on the generated code anyway. Split out from #24732
This pulls the changes out of #23508 to add just the
MEMORY64
support.The already merged interactive tests (from #23659) can be run with:
The browser tests with wasm64 have been re-enabled.
Note: the intention is to add the performance improvements (from last October) once this is merged (which are still relevant and produce the same improvements).
Will need a once-over when #24734 lands.