Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for running wasm-opt #159

Closed
fitzgen opened this issue Jun 7, 2018 · 10 comments · Fixed by #625
Closed

Add support for running wasm-opt #159

fitzgen opened this issue Jun 7, 2018 · 10 comments · Fixed by #625
Assignees
Labels
current release current todo items PR attached there's a PR open for this issue
Milestone

Comments

@fitzgen
Copy link
Member

fitzgen commented Jun 7, 2018

And configuring:

  • (most importantly) its optimization level (ie -Os vs -Oz vs -O3 etc)
  • what passes are enabled or disabled
  • maybe also installing it if necessary?

https://github.com/WebAssembly/binaryen

@ashleygwilliams ashleygwilliams added help wanted Extra attention is needed question Further information is requested to-do stuff that needs to happen, so plz do it k thx and removed question Further information is requested labels Jun 7, 2018
@ashleygwilliams
Copy link
Member

i think that we should fundamentally expose this through #153 - in line with our desire to have a more convention over configuration tool to expose pre-configured "build" options to the user. do those three build modes cover the behavior we'd want to see @fitzgen or is there more we would want to expose? i'm very cautious about overwhelming the use with options and configuration (tho #160 will certainly help with that)

@ashleygwilliams ashleygwilliams added the question Further information is requested label Jun 7, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Jun 7, 2018

I think we definitely need to expose control over optimization level. The other knobs I am "meh" on.

Regarding pre-configured builds: totally agree! I also don't think that is mutually exclusive with exposing knobs to change the default of some setting within each build. For example, cargo lets you do this with things like:

[profile.release]
# By default release doesn't build with debug info, but we are customizing that here.
debug = true
# Tell rustc to optimize for size, rather than its default of optizing for speed.
opt-level = "s"

I think we could do the same kind of default build configurations that do the 95% thing out of the box + knobs if you need them for the other 5% approach.

@ashleygwilliams ashleygwilliams added this to the 0.5.0 milestone Jun 11, 2018
@ashleygwilliams ashleygwilliams modified the milestones: 0.5.0, 0.6.0 Jul 26, 2018
@ashleygwilliams ashleygwilliams added PR attached there's a PR open for this issue help wanted Extra attention is needed and removed help wanted Extra attention is needed PR attached there's a PR open for this issue labels Jul 26, 2018
@csmoe
Copy link
Member

csmoe commented Jul 29, 2018

I'll take care of this.

@pepyakin
Copy link
Member

For what it worth, binaryen-rs has just gained support for setting optimization levels. This allows for achieving same functionality as with wasm-opt.

@ashleygwilliams ashleygwilliams removed this from the 0.6.0 milestone Dec 27, 2018
@ashleygwilliams
Copy link
Member

ashleygwilliams commented Jan 16, 2019

now that we've landed toml config in 0.6.0 i think it's time to look at this again and get it into 0.7.0.

decisions to make:

  1. how to run wasm-opt

    • should we run a child process with a wasm-opt binary release (from a fork of emscripten perhaps? since the release story there is complicated?)
    • should we leverage binaryen-rs

    personally if binaryen-rs is well-maintained this may be the best option.

  2. configuration
    i think we should leverage the build profiles. you should be able to set a key (wasm-opt or opt perhaps) to a direct set of wasm-opt args, or, some preset keywords (e.g. size, speed)

    i imagine this looking like

    [package.metadata.wasm-pack.profile.dev]
    opt = "size" #alias for some set of wasm-opt flags TBD
    
    [package.metadata.wasm-pack.profile.dev.wasm-bindgen]
    debug-js-glue = true
  3. alias defaults
    we'll need to define some defaults (if we opt for the aliasing which i think we should because i personally think the flags to wasm-opt are a bit inscrutable and not entirely clear how to use)

    i'd propose we have aliases for:

    • size
    • speed

    with potentially 2 more that are something like "super optimize for speed" and "super optimize for size" but i feel less strongly about these.

considerations:
i can't easily find docs on the wasm-opt interface (e.g. googling "wasm-opt docs" doesn't give me anyting) so we'll need to probbaly fully document wasm-opt here in addition to the aliases we'd be creating

@ashleygwilliams ashleygwilliams removed help wanted Extra attention is needed question Further information is requested labels Jan 16, 2019
@ashleygwilliams ashleygwilliams self-assigned this Jan 16, 2019
@ashleygwilliams ashleygwilliams added this to the 0.7.0 milestone Jan 16, 2019
@fitzgen
Copy link
Member Author

fitzgen commented Jan 16, 2019

  • I think we should run the wasm-opt binary instead of using binaryen-rs.

    • The downside is that we need to get upstream to publish windows and macos binaries (and/or publish them ourselves).
    • The advantage is avoiding the downsides of using binaryen-rs: that we would have to reimplement/reverse wasm-opt's CLI flag parsing (since there isn't a fn(argv) -> Config style function exposed AFAIK) or we would have to submit support for upstream to both binaryen and binaryen-rs.
    • Additionally, it is a whole bunch of C++ code, and having that in an isolated child process is nice for the inevitable crashes that will sneak in.
  • toml configuration

    • (mostly agreed here, just expanding a little bit)

    • The key should be wasm-opt not just opt because other tools have other optimization levels (e.g. rustc has its own opt-level)

    • Yes we should have a default alias for "optimize for size" and a default alias for "optimize for speed", and then also a third option that is an escape hatch for passing arbitrary CLI flags directly to wasm-opt.

    • Putting everything together, this means that users may choose one of the following:

      # 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"]
    • I suggest that if the wasm-opt key is missing, then we have the following defaults based on the profile:

      Profile Default wasm-opt key
      dev false (don't run wasm-opt)
      profiling "speed"
      release "speed"

      (I don't feel super strongly about defaulting to "speed" over "size", curious on others' opinions)

@ashleygwilliams
Copy link
Member

  • I think we should run the wasm-opt binary instead of using binaryen-rs.

yes - i think you're 100% right here, strong agree :) it keeps the internal logic of wasm-pack simpler too. we just spawn all the children, heh

@ashleygwilliams ashleygwilliams removed their assignment Mar 14, 2019
@ashleygwilliams ashleygwilliams modified the milestones: 0.8.0, 0.9.0 Mar 24, 2019
@alexcrichton
Copy link
Contributor

Ok as an update from the binaryen side of things they should now have binary releases configured for Windows/OSX as well as Linux platforms! A sample release looks like this and should be continuously happening for all future releases. It looks like tags are version_$N and the artifacts that we're interested in are:

  • binaryen-version_$N-x86_64-apple-darwin.tar.gz
  • binaryen-version_$N-x86_64-linux.tar.gz
  • binaryen-version_$N-x86_64-windows.tar.gz

All tarballs should have the directory structure $tarball_name/$binary.exe, where the binaries are just under the first folder.

I suspect we'll probably want to just hardcode a version number to download for now, but we can over time figure out a scheme for auto-updating binaryen downloads too

@alexcrichton
Copy link
Contributor

I've started work on this locally and hope to have a PR by tomorrow.

alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 12, 2019
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)
alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 12, 2019
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)
@alexcrichton
Copy link
Contributor

PR: #625

alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 12, 2019
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)
alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 12, 2019
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)
alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 12, 2019
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)
@ashleygwilliams ashleygwilliams added PR attached there's a PR open for this issue and removed PR welcome labels Apr 12, 2019
alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 15, 2019
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)
alexcrichton added a commit to alexcrichton/wasm-pack that referenced this issue Apr 16, 2019
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)
@ashleygwilliams ashleygwilliams removed the to-do stuff that needs to happen, so plz do it k thx label Jul 16, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
current release current todo items PR attached there's a PR open for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants