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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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