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 optimize sub-command using wasm-opt #236

Merged
merged 5 commits into from Oct 31, 2022
Merged

Add optimize sub-command using wasm-opt #236

merged 5 commits into from Oct 31, 2022

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Oct 27, 2022

What

Add optimize sub-command using wasm-opt.

Why

I saw @brson and @Aimeedeer's blog post about the wasm-opt crate today and after a quick test it appears to work very smoothly. Including optimization into the soroban-cli rather than instructing people to install other tools seems like it fits our philosophy of making writing effecient contracts the default.

This PR introduces an ultra simple sub-command (soroban optimize) that runs wasm-opt across the given .wasm file with the -Oz mode, which is the same mode we currently recommend.

I imagine we might also want to automatically run this across .wasm files as part of the deploy subcommand, although I thought doing that at this time, given that we are considering redesigning the CLI (#197), might be a bit premature.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Oct 27, 2022

I have a concern about merging this:

  • The wasm-opt crate requires a C compiler to also be installed. That might mean that cargo installing this crate might not work in all the cases that it works today. It is unlikely to be a big problem, but I'm not sure.

I am not attached to merging / adding this if folks think it isn't a good idea.

@leighmcculloch leighmcculloch marked this pull request as ready for review October 27, 2022 03:27
@leighmcculloch
Copy link
Member Author

leighmcculloch commented Oct 27, 2022

❯ soroban optimize --wasm target/wasm32-unknown-unknown/release/soroban_auth_contract.wasm
Reading: target/wasm32-unknown-unknown/release/soroban_auth_contract.wasm (4414 bytes)
Writing to: target/wasm32-unknown-unknown/release/soroban_auth_contract.wasm...
Optimized: target/wasm32-unknown-unknown/release/soroban_auth_contract.wasm (3522 bytes)

src/main.rs Outdated Show resolved Hide resolved
@dtolnay
Copy link

dtolnay commented Oct 27, 2022

  • The wasm-opt crate requires a C compiler to also be installed. That might mean that cargo installing this crate might not work in all the cases that it works today. It is unlikely to be a big problem, but I'm not sure.

You already depend on ring (via rustls, via jsonrpsee-http-client) which uses a C compiler.

src/opt.rs Outdated Show resolved Hide resolved
@brson
Copy link
Contributor

brson commented Oct 27, 2022

I have published version 0.110.2 of this crate that contains a single Binaryen backport that slightly reduces output size when the input module contains no "memories" section.

@leighmcculloch
Copy link
Member Author

I have published version 0.110.2 of this crate that contains a single Binaryen backport that slightly reduces output size when the input module contains no "memories" section.

Ah nice.

@brson Thank you!

@paulbellamy
Copy link
Contributor

Initially, I'm a bit on the fence about this.

Argument for including the soroban optimize command:

  • It is a generic wasm command which is applicable regardless of contract SDK used.
  • We may wish to integrate it into the deploy process automatically
  • We already have soroban inspect which is a wasm-specific command
  • Batteries-included approach to efficient contract development

Argument for excluding the soroban optimize command:

  • It is a build-step process, which should be dealt with as part of the build pipeline (rust/make/npm/whatever).
  • soroban-cli should not modify the wasm it is receiving (e.g. optimizing in deploy)
  • Reinforces the C compiler dependency
  • Automatically optimizing as part of deployment could make repeatable builds and source-code verification harder? (are repeated wasm-opt calls idempotent?)

But, overall, from writing that down, I think this SGTM.

src/opt.rs Outdated Show resolved Hide resolved
@leighmcculloch
Copy link
Member Author

  • It is a build-step process, which should be dealt with as part of the build pipeline (rust/make/npm/whatever).

For npm projects this makes sense. For Rust projects, there's no way to hook post-build steps into a project.

The reason to include wasm-opt into soroban isn't to take over the build pipeline (although #226 is probably getting into that territory), but to provide easy access to wasm-opt. Wasm-opt can be installed in a variety of ways that complicate system setup. Since devs already need to have soroban installed, we can say here's this default set of tools that is builtin that you can use to optimize.

But maybe it is being too opinionated.

  • Automatically optimizing as part of deployment could make repeatable builds and source-code verification harder? (are repeated wasm-opt calls idempotent?)

Good point. Your comment also got me thinking that automatically modifying contracts during deploy could be surprising, and if there was ever a bug there's no opportunity for a developer to test out the artifact before it deploys.

@leighmcculloch
Copy link
Member Author

  • are repeated wasm-opt calls idempotent?

I did some tests with this and it appears no. I managed to get a .wasm file even smaller by running it multiple times.

@leighmcculloch leighmcculloch merged commit 9593235 into main Oct 31, 2022
@leighmcculloch leighmcculloch deleted the optimize branch October 31, 2022 15:59
@willemneal
Copy link
Member

@leighmcculloch there is a --converge option on the CLI which should result in the same binary as manually rerunning it until it stops decreasing.

@leighmcculloch
Copy link
Member Author

Thanks @willemneal. I'm experimenting with that option in #238.

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 this pull request may close these issues.

None yet

6 participants