Skip to content

_ #4730

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

Closed
ghost opened this issue Jun 15, 2022 · 7 comments
Closed

_ #4730

ghost opened this issue Jun 15, 2022 · 7 comments

Comments

@ghost
Copy link

ghost commented Jun 15, 2022

_

@kripken
Copy link
Member

kripken commented Jun 15, 2022

There are binaries for the major platforms available here on github. If there are issues with the builds brew or some other package manager provides, perhaps try those?

https://github.com/WebAssembly/binaryen/releases

I can't think of a flag that changed that would prevent optimization. That sounds very strange. If you can't easily figure this out with those builds, please provide the full wasm-opt command you are running and the binary you are running it on, so we can help.

@kripken
Copy link
Member

kripken commented Jun 16, 2022

The only possible theory I could have is if the name section is invalid, binaryen might drop it rather than rewrite it. But you don't have -g so there is no name section relevant anyhow...

In any case, if you can't figure this out, please provide a full testcase we can reproduce this with: a wasm file + a wasm-opt command that optimizes to 1MB in v98 but turns into 21MB in v99. Such a huge regression should be very fast for us to figure out.

@kripken
Copy link
Member

kripken commented Jun 16, 2022

Oh, actually I had another idea: #3306 It fits as it landed in v99.

If that's it, it's a bugfix for behavior that was wrong before, so we had no choice but to fix it. But it can regress size. To confirm, try adding --zero-filled-memory. Looks like that's not in the changelog, which is bad - I'll add that now.

Btw, in general, it's safer to let emscripten run wasm-opt for you, as it will supply the right flags automatically, which avoids stuff like this.

@kripken
Copy link
Member

kripken commented Jun 16, 2022

Good, glad we figured this out. Btw, if you're building with wasi-sdk then another flag you might want is --low-memory-unused. That's the other main flag emscripten passes by default to wasm-opt, that you need to pass manually otherwise. It does require you to ensure low memory is actually unused, though, which I'm not sure how to do in wasi-sdk. But that flag is usually just a few % of improvement to size and speed.

@ghost ghost closed this as completed Jun 17, 2022
@tlively
Copy link
Member

tlively commented Jun 17, 2022

Smaller Wasm modules are generally better. Especially for the data section, there's no reason to prefer a larger one.

An integration test making sure that wasi-sdk + binaryen gets all the expected optimizations would be useful, but this would not be the right repo for it IMO. If there were more of an integration between wasi-sdk and Binaryen, the wasi-sdk repo would make more sense. But AFAIK there's not really an integration to test.

The zeroes you're seeing might be .bss, yes, but I thought that the linker skipped emitting .bss sections in most configurations. I don't remember exactly.

@kripken
Copy link
Member

kripken commented Jun 17, 2022

There should be a failed test for a program generated with lots of static memory and then wasm-opt not affecting the output.

I agree with @tlively that there isn't something to do in this repo. We already have tests for doing the right thing when that flag is passed (unit tests on the MemoryPacking pass). But we can't expect that to work without the flag, sadly (as mentioned before, it's just incorrect to assume memory is zeroed).

But I agree we can improve things in general. Another option might be adding some kind of docs on the wasi-sdk side, explaining how crucial this binaryen flag is? Hopefully other wasi-sdk users would see that and avoid the problem you ran into. I'm not sure exactly where such docs would go, however.

@tlively
Copy link
Member

tlively commented Jun 17, 2022

I would expect users to discover --zero-filled-memory by reading our --help output (and now by reading the release notes as well). Arguably there should be a wiki page or something with all the tool options, but it would mostly duplicate all the information already in --help.

@ghost ghost changed the title Output WASM file sizes _ Aug 15, 2022
This issue was closed.
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

No branches or pull requests

2 participants