-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add support for automatically executing wasm-opt
#625
Conversation
i'd like to merge this with the refactor in #623, so gonna wait til that lands (which should be early next week, if not earlier) |
# Configuration can be set to `false` if you want to disable it (as is the | ||
# default for the dev profile), or it can be an array of strings which are | ||
# explicit arguments to pass to `wasm-opt`. For example `['-Os']` would optimize | ||
# for size while `['-O4']` would execute very expensive optimizations passes |
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 is missing "size" and "speed" meta options that we planned in the original design:
# This is the dev profile, but could also be the profiling or release profiles. [package.metadata.wasm-pack.profile.dev] # The `wasm-opt` key may be absent, in which case we choose a default # # or we can explicitly configure that we *don't* want to run it wasm-opt = false # or use our default alias to optimize for size wasm-opt = "size" # or use our default alias to optimize for speed wasm-opt = "speed" # or give your own custom set of CLI flags wasm-opt = ["--dce", "--duplicate-function-elimination", "--instrument-memory"]
Is the intention to add support for these in follow up PRs?
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.
True! I forgot to mention that here, but I actually first implemented it as a string being a space-separated list of arguments (like wasm-opt = "-Os -some-other-pass"
), and then I read the original issue and liked your idea better.
I figured though it may be best to do that as a separate PR as it should be pretty easy to add on top of this, but rather we'd just want to get the bare bones in here
This commit adds support for automatically executing the `wasm-opt` binary from the [Binaryen project][binaryen]. By default `wasm-pack` will now, in release and profiling modes, execute `wasm-opt -O` over the final binary. The goal here is to enable optimizations that further reduce binary size or improve runtime. In the long run it's expected that `wasm-opt`'s optimizations may mostly make their way into LLVM, but it's empirically true today that `wasm-opt` plus LLVM is the best combination for size and speed today. A configuration section for `wasm-opt` has been added as [previously proposed][fitzgen], namely: ```toml [package.metadata.wasm-pack.profile.release] wasm-opt = ['-Os'] ``` The `wasm-opt` binary is downloaded from Binaryen's [own releases](https://github.com/webassembly/binaryen/releases). They're available for the same platforms that we download predownloaded binaries for `wasm-bindgen` on. We'll also opportunistically use `wasm-opt` in `PATH` if it's available. If we're untable to run `wasm-opt`, though, a warning diagnostic is printed informing such. Closes rustwasm#159 [binaryen]: https://github.com/webassembly/binaryen [fitzgen]: rustwasm#159 (comment)
Ping for review on this? I'm not sure if there's anything still blocking this? |
taking on the merge for this- gonna rewrite a bit to use the general logic in #623 |
gonna merge this and then refactor |
this is failing on a cargo fmt, since i'm about to do #685 i think it's fine and gonna merge |
@alexcrichton @ashleygwilliams So i was working on a PR, and I think this is breaking functionality for the current master? I get the following error: Is there a way to skip the step? Or will this be fixed by #685 ? Thanks! 😄 |
@torch2424 you should be able to cofnigure: [package.metadata.wasm-pack.profile.dev]
wasm-opt = false
[package.metadata.wasm-pack.profile.release]
wasm-opt = false to turn off |
@alexcrichton Awesome, will try that later tonight, and update here 😄 I'll open the issue then too, since a newer version may have fixed this like you said (and I assume we will consume this newer version 👍 ) EDIT: It worked! Thanks for the suggestion! 🎉 |
Oh after looking at: EDIT: opened #696 |
@alexcrichton It works wrong with this configuration: [profile.release]
opt-level = 3
[package.metadata.wasm-pack.profile.release]
wasm-opt = ["-O3"] |
This commit adds support for automatically executing the
wasm-opt
binary from the Binaryen project. By default
wasm-pack
will now, in release and profiling modes, execute
wasm-opt -O
over thefinal binary. The goal here is to enable optimizations that further
reduce binary size or improve runtime. In the long run it's expected
that
wasm-opt
's optimizations may mostly make their way into LLVM, butit's empirically true today that
wasm-opt
plus LLVM is the bestcombination for size and speed today.
A configuration section for
wasm-opt
has been added as previouslyproposed, namely:
The
wasm-opt
binary is downloaded from Binaryen's ownreleases. They're
available for the same platforms that we download predownloaded binaries
for
wasm-bindgen
on. We'll also opportunistically usewasm-opt
inPATH
if it's available. If we're untable to runwasm-opt
, though, awarning diagnostic is printed informing such.
Closes #159