Skip to content
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

Build regression: "STANDALONE_WASM does not support memory growth yet" #9587

Closed
zeux opened this issue Oct 7, 2019 · 8 comments · Fixed by #9588
Closed

Build regression: "STANDALONE_WASM does not support memory growth yet" #9587

zeux opened this issue Oct 7, 2019 · 8 comments · Fixed by #9588

Comments

@zeux
Copy link

zeux commented Oct 7, 2019

The following command line used to work a few months ago - I think this was with 1.38.40:

emcc src/vertexcodec.cpp src/indexcodec.cpp -O3 -DNDEBUG -s EXPORTED_FUNCTIONS='["_meshopt_decodeVertexBuffer", "_meshopt_decodeIndexBuffer"]' -s ALLOW_MEMORY_GROWTH=1 -s TOTAL_STACK=24576 -s TOTAL_MEMORY=65536 -o build/meshopt_decoder.wasm

After uprading to 1.38.47, I now get the following error:

shared:ERROR: STANDALONE_WASM does not support memory growth yet

Memory growth worked correctly before the update. Any pointers as to how to fix this would be appreciated.

@kripken
Copy link
Member

kripken commented Oct 7, 2019

This is because we are moving to a more robust approach for -o X.wasm. Before we just didn't emit the JS, and hoped for the best. After, we have the new STANDALONE_WASM mode which builds slightly different system libs etc., in order to get a working wasm file. And I added some assertions for things known to not work yet.

Did the output you got before actually work, including memory growth? How did you run it, and with what support code (specifically, what handled the growth)?

If there was stuff that worked before that the assertions now prevent, we should remove them, but would be good to understand what that was.

@zeux
Copy link
Author

zeux commented Oct 7, 2019

Did the output you got before actually work, including memory growth? How did you run it, and with what support code (specifically, what handled the growth)?

Yeah the output before worked without issues; the memory growth support code I used was simply resizing the Memory object - note that the C++ code in question doesn't do any memory allocations, so this function is called from JS:

https://github.com/zeux/meshoptimizer/blob/master/js/meshopt_decoder.js#L13-L21

Just removing this check in the new emscripten build isn't sufficient for getting this to work though - not sure why yet.

kripken added a commit that referenced this issue Oct 7, 2019
…le it isn't 100% standalone (there is an import to grow the memory), it is usable for people that implement that import, so don't break them. fixes #9587
@kripken
Copy link
Member

kripken commented Oct 7, 2019

I see, thanks!

My guess is it doesn't work now because you provide sbrk, but we changed that ABI. sbrk is now internal in the wasm module (to make it more standalone). Instead, it imports a simpler emscripten_resize_heap function that is called to resize. To see more, I'd look at the imports, with something like wasm-dis X.wasm | grep import (but the wasm VM should also error and tell you what's missing on the import object, when it instantiates).

Another difference is that the wasm will create the memory itself, you don't need to create it on the outside and import it (again, this makes the wasm more standalone).

Sorry for the confusion, but this is an internal ABI detail that we don't guarantee won't change, and we had to optimize now.

That assertion was definitely unnecessary though, I'll remove it in #9588.

@zeux
Copy link
Author

zeux commented Oct 7, 2019

Yeah - I figured out how to adjust my code to make it work.

It looks like before, memory was an import so my JS wrapper had to provide it when loading the wasm module.
Now it's an export so I need to get it from the instance after loading.

I understand that I'm playing fast & loose here and I'm happy to adjust my code a bit after Emscripten releases - I couldn't find another way to get a small compact binary.

@zeux
Copy link
Author

zeux commented Oct 7, 2019

Interestingly I no longer need to supply "emscripten_memcpy_big" anymore either - I am assuming that the standalone version of libc now handles the copy internally without calling out to JS?

@kripken
Copy link
Member

kripken commented Oct 7, 2019

Correct, in standalone mode we don't expect JS to be able to do a super-fast memcpy for us, so it's all internal in the wasm.

@zeux
Copy link
Author

zeux commented Oct 7, 2019

Ok - thanks! The .wasm file is a touch bigger with the new standalone mode, but it evens out with the reductions in the JS blob (.wasm is embedded into .js here):

-rwxrwxrwx 1 zeux zeux 8407 Oct 6 19:06 meshopt_decoder.js
-rwxrwxrwx 1 zeux zeux 3865 Oct 6 19:06 meshopt_decoder.js.gz
-rwxrwxrwx 1 zeux zeux 8346 Oct 6 19:04 meshopt_decoder_new.js
-rwxrwxrwx 1 zeux zeux 3839 Oct 6 19:04 meshopt_decoder_new.js.gz

My code did depend on memcpy but the sizes were below the old JS threshold so performance isn't affected here either, which is great.

@zeux
Copy link
Author

zeux commented Oct 7, 2019

... in fact, it looks like with the new runtime I can even export sbrk via -s EXPORTED_FUNCTIONS which means I no longer need a JS stub for heap management. A JS implementation of sbrk used to be valuable for code size reasons, since the C implementation of sbrk would require a lot of odd imports such as DYNAMICTOP_PTR, but now all of this is self-contained inside the .wasm - which is great because it simplifies the implementation of the JS wrapper. So as far as I can tell, memory growth for standalone mode "just works" with no need for any imports/exports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants