-
Notifications
You must be signed in to change notification settings - Fork 25
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
chore: Release wasm strip #158
Conversation
@pinkforest to focus this on wasm builds, should we override just that package: https://doc.rust-lang.org/cargo/reference/profiles.html#overrides? It may not be what we want for the rust-only package: https://nnethercote.github.io/perf-book/build-configuration.html. |
The release profile would be here only for us in the workspace to test the .wasm codesize out - Given this when the library gets used people have their own release profiles thus we could maybe test them all given tradeoffs ? One for code size and one for the others e.g. compile time etc. - with the codesize could be well known default. Further to that - we could adopt cfg's for desired performance priorities - I will do as separate write-up on that and we can see what fits on that side but unrelated to this one. |
Hm. That doesn't seem to fail in CI, so should be ok. I wonder what's happening on your end.
Not only that - we prepare the I do wonder whether we should have separate "debug" and "release" About the benchmark being 2x slower: I really don't know what that's about unfortunately. I recently re-ran the benchmarking action on another PR where it also was 2x slower, and I think re-running didn't show the issue anymore, so it's flakey. I'll do the same on this PR 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right and a 👍 if the benchmarking just ended up being flaky
Awesome. Thanks for this fix. Right now the workspace level profile change affects wnfs as a rust dep. And since cargo does not support target-specific profile settings and simply moving the settings into We should also have it in mind that we are optimizing wasm for build size over perf as the default behaviour if that is what we really want. |
Reached similar file size with a config that targets just wasm32 arch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what @appcypher is writing makes sense to me, so I'd retract my approval.
Still, thanks for prompting this!
@appcypher can you create a PR from your branch? Would you recommend we use these rustflags?
Summary
wasm blobs are generally slow to load and it is preferably if these are lightweight
Stripping is usually not done by default nor by wasm-opt so
Test plan (required)
$ yarn playwright test ✔️ 56 passed, 1 fail
One test seems broken ?
Before:
After:
Sheds ~200 kB
1 Failing Test