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

Invoke wasm-opt only in release mode? #197

Closed
WorldSEnder opened this issue Jul 16, 2021 · 12 comments · Fixed by #217
Closed

Invoke wasm-opt only in release mode? #197

WorldSEnder opened this issue Jul 16, 2021 · 12 comments · Fixed by #217
Labels
discussion This item needs some discussion

Comments

@WorldSEnder
Copy link

I successfully installed wasm-opt from binaryen and can include data-wasm-opt="4" in my index.html, which brings the wasm size down by a factor of 60% for a simple hello world project in release mode.

But since wasm-opt takes quite a bit of time, I don't want to do that in debug mode. Is there a way to only use that option in --release mode? The very ugly way is to have a separate index-release.html and possibly Trunk.release.toml referencing that file.

I see several paths forward, e.g. looking for Trunk.release.toml (or variants) and treating that as an override to the general Trunk.toml, or preprocessing index.html in some way to allow for dynamic replacements.

@Follpvosten
Copy link

You also may not have wasm-opt on your development machine's PATH at all; I use a container for building the project's release, which does have it. Because of this, wasm-opt breaks dev builds for me unfortunately.

@thedodd
Copy link
Member

thedodd commented Jul 28, 2021

@WorldSEnder having to maintain multiple index.html files is definitely a PITA, and I want to avoid a situation where folks feel that they need to do so. However, having multiple Trunk.toml files (Trunk.release.toml, or Trunk.dev.toml) is definitely part of design intention behind having an option config file. You can specify the config file using trunk --config <pathToFile> ... or by setting the env var TRUNK_CONFIG=pathToFile which can be configured in a CI file or the like.

Thoughts? Also, @Follpvosten does the above info resolve your issue at all?

@thedodd thedodd added the discussion This item needs some discussion label Jul 28, 2021
@thedodd
Copy link
Member

thedodd commented Jul 28, 2021

So, I have overlooked what appears to be the deeper issue. Per-pipeline config can not be altered via Trunk.toml. We essentially need a mechanism to be able to state in the index.html's pipelines that wasm-opt should only be invoked for release and not debug builds.

I am tempted to say that the easiest path forward is to just make it such that wasm-opt is NEVER invoked for debug builds; however, as soon as we do that someone will come along with a legit use case for why they NEED it enabled for their debug builds.

Though this is not a generic pattern for all other pipeline types, perhaps this is an issue where we just need to provide a new option for the pipeline. Something like data-wasm-opt-debug & data-wasm-opt-release. The value for debug could be set to "off" and the value for release could be set to "4" (or whatever is needed). This would resolve the issue in this specific case.

There is probably a handful of good ways to do this. Thoughts?

@WorldSEnder
Copy link
Author

WorldSEnder commented Jul 28, 2021

Though this is not a generic pattern for all other pipeline types, perhaps this is an issue where we just need to provide a new option for the pipeline. Something like data-wasm-opt-debug & data-wasm-opt-release. The value for debug could be set to "off" and the value for release could be set to "4" (or whatever is needed). This would resolve the issue in this specific case.

There is probably a handful of good ways to do this. Thoughts?

This is drifting into the direction of index.html actually being a html template which is instantiated with some config-specific variables. Perhaps commit to that idea fully and search and implement for a suitable template language?

Otherwise, I'd be fine with this very specific workaround of having both data-wasm-opt-debug and data-wasm-opt-release if there's not too many concerns about adapting the same approach for other things in the future.

@Follpvosten
Copy link

Follpvosten commented Jul 28, 2021

Though this is not a generic pattern for all other pipeline types, perhaps this is an issue where we just need to provide a new option for the pipeline. Something like data-wasm-opt-debug & data-wasm-opt-release. The value for debug could be set to "off" and the value for release could be set to "4" (or whatever is needed). This would resolve the issue in this specific case.

There is probably a handful of good ways to do this. Thoughts?

I agree; I think it would be nice to have this as a generic secondary property, like a data-debug and data-release, which just makes the other profile ignore the entire asset definition, or maybe even data-profile="<debug|release>" for more flexibility. Especially for the "rust" asset type, I think this would be very useful, since for example you might also want to enable some features based on the target, and in my case, I don't actually need a rust asset in debug mode at all (I only need it for that wasm-opt setting).

However, I can see that this might be overkill, so for now, data-wasm-opt-release and data-wasm-opt-debug might be preferable. I would be happy either way, as anything is better than what I have currently (sed -i '15i \<link data\-trunk rel\=\"rust\" href\=\"\.\" data\-wasm\-opt\=\"z\" \/\>' index.html in my CD pipeline).

@thedodd
Copy link
Member

thedodd commented Jul 28, 2021

Hahaha, sed to the rescue.

@Follpvosten that's an interesting point about data-profile="<debug|release>". That would be a generic way to enable or disable pipeline/asset definitions based on debug vs release builds. I quite like that. The only downside is that there would be some apparent duplication of data with the only difference being the data-profile value. That might be ok though, especially given that most other asset types do not need specialized configuration per profile. EG, SASS is automatically minified in release mode.

PS
Before we hit a 1.0 release, I want to remove the data- prefix from our various Trunk attributes. I thought that I would care a lot more about maintaining a "semantic" index.html file ... however only the output index.html matters after processing, and all of those attributes are removed.

@thedodd
Copy link
Member

thedodd commented Jul 28, 2021

@WorldSEnder perhaps we could create a mechanism where the Trunk.toml may include a [template] section with arbitrary key/values. And when Trunk parses the index.html file, if it detects an attribute with a variable interpolation pattern in it (EG {{key}}) then Trunk will dynamically interpolate the requested value from the Trunk.toml's template section.

This would allow for the optional config system to drive more specialized cases, and CI could easily be updated to just point Trunk to the appropriate Trunk.toml file. This would also mean that we don't need to pull in an entire templating engine.

Thoughts?

@WorldSEnder
Copy link
Author

it detects an attribute with a variable interpolation pattern in it. [...]
This would also mean that we don't need to pull in an entire templating engine.

This boils down to rolling your own templating engine, in the end, which means documenting syntax and extending it?

Not an endorsement, e.g. tinytemplate seems also lightweight, doesn't pull in a lot of dependencies (besides serde, which trunk already needs) and would probably work for interpolating attributes, as would microtemplate. Thoughts?

to just point Trunk to the appropriate Trunk.toml file

Yeah, that'd work, preferably I could also somehow inherit properties from Trunk.toml in Trunk.release.toml but so far that file is small enough, but I'd usually just override build.release and build.public_url in release profile.

Maybe define the attributes in [template] in some kind of external .env file?

@thedodd
Copy link
Member

thedodd commented Jul 28, 2021

Nice, tinytemplate could definitely work. Especially given that it will serialize the given context object as json. This would allow us to feed it our [template] section from the Trunk.toml directly and things should just work.

TBH, I like this better than having to introduce a bunch of new asset specific attributes. As long as we keep this simple, then the templating logic won't become a maintenance burden. If folks start asking for jinja2/gotmpl like functionality, then we've got a problem.

@WorldSEnder as far as config file inheritance, #205 should help to address this.

@WorldSEnder
Copy link
Author

WorldSEnder commented Jul 28, 2021

If folks start asking for jinja2/gotmpl like functionality, then we've got a problem.

At that point, I'd vote for mirroring how build.rs communicates with cargo and ask for a way to read the index.html from external program output instead of a file and push the heavy templating on the user, tbh.

@thedodd
Copy link
Member

thedodd commented Aug 5, 2021

Thinking about this a bit more, I'm tempted to say that we should totally just ignore wasm-opt for debug builds (can anyone think of an actual use case for wasm-opt on a debug binary?), and just apply wasm-opt automatically to release builds. That way users would only have to define the wasm-opt attr once in their index.html, and wouldn't have to toggle between that value for debug vs release.

Thoughts? @udoprog would you be interested in adapting your PR to match this behavior?

@Follpvosten
Copy link

Makes absolute sense to me. The current default behavior in 0.12.1 is what I'd set manually, too (0 for debug, 3 for release) and I don't think I'd ever want to run wasm-opt for debug builds (which, by the way, takes muuuuuuch longer than for release builds because LLVM doesn't optimize many things out in debug mode, so wasm-opt has to go through a lot more code).

thedodd added a commit that referenced this issue Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
thedodd added a commit that referenced this issue Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
thedodd added a commit that referenced this issue Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This item needs some discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants