Integrate wasm-opt into the rust-app pipeline #115

dnaka91 · 2021-01-30T03:41:15Z

This PR adds integration with wasm-opt as mentioned in #7 and #114. Currently the wasm-opt binary must be installed on the system for trunk to work (or the the attribute set to wasm-opt="0").

I thought of just skipping the optimization step if the binary is missing but that may cause unexpected side effects. My plan is to add an auto-download feature for wasm-opt and wasm-bindgen-cli in the future so that trunk can install any missing external programs.

Checklist

Updated CHANGELOG.md describing pertinent changes.
Updated README.md with pertinent info (may not always apply).

Fixes #114.

(Dodd) post-merge: open an issue to track work on automatic download of wasm-opt. Maybe lump this in with the work on auto-download of wasm-bindgen-cli (we already have a ticket for the later), but at a minimum let's try to consider both use cases.

ranile · 2021-01-30T06:53:23Z

README.md

@@ -41,6 +41,18 @@ Next, we will need to install `wasm-bindgen-cli`. In the future Trunk will handl
 cargo install wasm-bindgen-cli
 ```

+In addition, we will need to install `wasm-opt` which is part of `binaryen`. On MacOS we can install


Can we bundle the binary with trunk or download on command invocation (and cache it)?

As I wrote in the description I thought of adding that as a separate PR because it would involve wasm-bindgen-cli as well so we don't need any binaries present.

And there is #105 which would require to re-write download part again as the HTTP client would change as well.

thedodd · 2021-02-01T03:40:15Z

Awesome! Really happy to see this PR! We are currently hacking on getting the 0.8.0 release prepped. After that, I will be moving over to finishing review on our Tokio PR and then on to this one! 🍻

dnaka91 · 2021-02-01T12:54:40Z

@thedodd great to hear. Once the Tokio PR is merged I'll start preparing changes to auto-download pre-compiled binaries of wasm-opt and wasm-bindgen-cli as a separate PR.

README.md

thedodd

Thanks for all of the work here @dnaka91! A few items here. Let me know what you think.

src/pipelines/rust_app.rs

thedodd · 2021-02-16T13:45:53Z

src/pipelines/rust_app.rs

@@ -57,6 +61,7 @@ impl RustApp {
            })
            .unwrap_or_else(|| html_dir.join("Cargo.toml"));
        let bin = el.attr("data-bin").map(|val| val.to_string());
+        let wasm_opt = el.attr("wasm-opt").map(|val| val.to_string());


Let's add a struct to wrap and validate this value. Something like (using https://jeltef.github.io/derive_more/derive_more/deref.html):

#[derive(Deref)] #[deref(forward)] struct WasmOptValue(String); impl WasmOptValue { pub fn validate(val: &str) -> Result<Self> { // validate the value then wrap it in self } }

Then we can update this line to be let wasm_opt = el.attr("wasm-opt").map(|val| WasmOptValue::validate(val)).transpose()?;.

Then, down in the code where it is actually used, we can simply do self.wasm_opt.as_deref().as_str() which is compatible with the AsRef<std::ffi::OsStr> constraint used below.

This will allow folks to see if they've supplied an incorrect value as soon as Trunk is invoked, instead of having to wait until the very end of the cargo build.

How about creating an enum for the possible values and implement FromStr and AsRef<str> on it?
That would be a more type safe approach.

Initially I thought it's okay to let the user just pass any value as wasm-opt would just throw an error and we wouldn't have to adjust whenever wasm-opt changes its options. But especially this option probably never changes (or extremely rarely) and can be adjusted quickly.

I dig it. I was going to suggest that initially, but I wasn't sure if it would work with structopt. But if it does, then I definitely agree that it is a better approach.

Just checked, FromStr is what structopt requires for the argument types.

https://docs.rs/structopt/0.3.21/structopt/#type-magic

So this should work just fine with enum and FromStr 👍

thedodd · 2021-02-16T13:51:45Z

src/pipelines/rust_app.rs

+        if self.wasm_opt.as_deref() == Some("0") {
+            return Ok(());
+        }


I would say that we should do:

let wasm_opt = match self.wasm_opt.as_deref().as_str() { Some(val) if val != "0" => val, _ => return Ok(()); };

Basically this will allow Trunk workflows to continue to work even without wasm-opt present on their machine. If they decide that they want to start using it, then they can set it to a non-zero value, and they are g2g.

Once we implement the work to actually download wasm-opt for the user (and probably download wasm-bindgen-cli for them as well), then we can make this default to opt level 4 or something like that, as it will be mostly transparent to the user and will not break existing workflows.

Thoughts?

Yeah that's probably a better approach and would not cause half the users to get errors when running Trunk after the update 😅.

I think "" (no opt level) should be the default as that already applies many optimizations (maybe even equivalent to 3 or 4) and wasm-pack does the same.

thedodd · 2021-02-16T13:54:22Z

README.md

@@ -41,6 +41,18 @@ Next, we will need to install `wasm-bindgen-cli`. In the future Trunk will handl
 cargo install wasm-bindgen-cli
 ```

+In addition, we will need to install `wasm-opt` which is part of `binaryen`. On MacOS we can install


Per my comments below, until we get to the point where we are actually downloading wasm-opt for the user, I would say that we should update this verbiage to something like: If using wasm-opt, we will need to install ....

Definitely. Have been a bit occopied this week but I'll continue on this soon and adjust to your comments.

dnaka91 · 2021-02-19T14:50:17Z

Okay just made a few changes according to our discussions. Hope I didn't miss anything 😰

dnaka91 · 2021-03-03T15:45:57Z

It's been a while, so just wanted to know whether there is anything holding this off that I missed? (besides obviously pulling the latest changed from master. Will do that tomorrow)

thedodd · 2021-03-03T17:49:29Z

@dnaka91 nope, you're good. I have a few changes in flight for the WebSockets/Proxy updates, but I should be able to circle back here very soon to finish up final review and merge.

Thanks again!

thedodd

A few small items, though I suspect I will need to do one more pass after this branch is rebased onto master.

Keep me posted! I'm hoping to release this as part of 0.9.0 along with the WebSocket proxy code.

thedodd · 2021-03-05T02:29:42Z

src/pipelines/rust_app.rs

+
+impl Default for WasmOptLevel {
+    fn default() -> Self {
+        // Current default is off until automatic download of wasm-opt is implemented.\


Suggested change

// Current default is off until automatic download of wasm-opt is implemented.\

// Current default is off until automatic download of wasm-opt is implemented.

Oups, how did that one slip in there :D

thedodd · 2021-03-05T02:31:46Z

src/pipelines/rust_app.rs

@@ -57,6 +123,7 @@ impl RustApp {
            })
            .unwrap_or_else(|| html_dir.join("Cargo.toml"));
        let bin = el.attr("data-bin").map(|val| val.to_string());
+        let wasm_opt = el.attr("wasm-opt").map(|val| val.parse()).transpose()?.unwrap_or_default();


After rebasing onto master, this will have to be updated as a different mechanism is being used to pass along attributes.

thedodd · 2021-03-05T02:35:19Z

src/pipelines/rust_app.rs

+        // Zero means no optimizations (valid arg for wasm-opt), so we can skip calling wasm-opt as
+        // it wouldn't have any effect.
+        if self.wasm_opt == WasmOptLevel::Off {


Suggested change

// Zero means no optimizations (valid arg for wasm-opt), so we can skip calling wasm-opt as

// it wouldn't have any effect.

if self.wasm_opt == WasmOptLevel::Off {

// If opt level is off, we skip calling wasm-opt as it wouldn't have any effect.

if matches!(self.wasm_opt, WasmOptLevel::Off) {

That's definitely a better description, especially now where I turned this into an enum 👍

dnaka91 · 2021-03-05T03:06:07Z

Okay, all updated. Hopefully I didn't miss anything.

dnaka91 · 2021-03-05T03:11:37Z

@thedodd I saw the tokio PR was closed in favor of async-std. So I guess I can start working on a basic version of the auto-download feature after this is merged?

thedodd · 2021-03-05T13:51:13Z

@dnaka91 yes sir. That would be awesome!

thedodd · 2021-03-05T13:59:43Z

@dnaka91 I've added this issue: #139 let me know what you think! And thanks for all of the great work on this front!

Integrate wasm-opt into the rust-app pipeline

cb8cfce

ranile reviewed Jan 30, 2021

View reviewed changes

dnaka91 added 2 commits February 1, 2021 22:03

Merge remote-tracking branch 'upstream/master' into wasm-opt

be213a2

Merge remote-tracking branch 'upstream/master' into wasm-opt

4b5fa45

thedodd reviewed Feb 16, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into wasm-opt

6bf6823

thedodd requested changes Feb 16, 2021

View reviewed changes

Adjust to PR comments

dc5a962

dnaka91 requested a review from thedodd February 19, 2021 14:49

thedodd requested changes Mar 5, 2021

View reviewed changes

dnaka91 added 4 commits March 5, 2021 11:56

Merge remote-tracking branch 'upstream/master' into wasm-opt

ac33951

Move changelog entry to 0.9.0

8c27b5e

Remove accidental character from comment

8ee52b4

Update comment about disabled wasm-opt

fec49b9

thedodd merged commit fd347f3 into trunk-rs:master Mar 5, 2021

thedodd mentioned this pull request Mar 5, 2021

Auto-download wasm-opt if not present #139

Closed

3 tasks

dnaka91 deleted the wasm-opt branch March 5, 2021 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate wasm-opt into the rust-app pipeline #115

Integrate wasm-opt into the rust-app pipeline #115

dnaka91 commented Jan 30, 2021 •

edited by thedodd

Loading

ranile Jan 30, 2021

dnaka91 Jan 30, 2021 •

edited

Loading

thedodd commented Feb 1, 2021

dnaka91 commented Feb 1, 2021

thedodd left a comment

thedodd Feb 16, 2021

dnaka91 Feb 18, 2021

thedodd Feb 18, 2021

dnaka91 Feb 19, 2021

thedodd Feb 16, 2021

dnaka91 Feb 18, 2021

thedodd Feb 16, 2021

dnaka91 Feb 18, 2021

dnaka91 commented Feb 19, 2021

dnaka91 commented Mar 3, 2021

thedodd commented Mar 3, 2021

thedodd left a comment

thedodd Mar 5, 2021

dnaka91 Mar 5, 2021

thedodd Mar 5, 2021

thedodd Mar 5, 2021

dnaka91 Mar 5, 2021

dnaka91 commented Mar 5, 2021

dnaka91 commented Mar 5, 2021

thedodd commented Mar 5, 2021

thedodd commented Mar 5, 2021

	// Current default is off until automatic download of wasm-opt is implemented.\
	// Current default is off until automatic download of wasm-opt is implemented.

Integrate wasm-opt into the rust-app pipeline #115

Integrate wasm-opt into the rust-app pipeline #115

Conversation

dnaka91 commented Jan 30, 2021 • edited by thedodd Loading

Choose a reason for hiding this comment

dnaka91 Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

thedodd commented Feb 1, 2021

dnaka91 commented Feb 1, 2021

thedodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnaka91 commented Feb 19, 2021

dnaka91 commented Mar 3, 2021

thedodd commented Mar 3, 2021

thedodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnaka91 commented Mar 5, 2021

dnaka91 commented Mar 5, 2021

thedodd commented Mar 5, 2021

thedodd commented Mar 5, 2021

dnaka91 commented Jan 30, 2021 •

edited by thedodd

Loading

dnaka91 Jan 30, 2021 •

edited

Loading