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

chore: Release wasm strip #158

Closed
wants to merge 2 commits into from
Closed

Conversation

pinkforest
Copy link
Contributor

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 ?

  Slow test file: [safari] › private.spec.ts (10m)
  Slow test file: [firefox] › private.spec.ts (31s)
  Consider splitting slow test files to speed up parallel execution

  1 failed
    [safari] › private.spec.ts:194:7 › PrivateDirectory › basicMv can move content between directories 
  59 passed (11m)

Before:

release [optimized] target(s) in 18.08s
Done in 19.38s
740K	pkg/wnfs_wasm_bg.wasm`

After:

release [optimized] target(s) in 18.22s
Done in 19.08s
572K	pkg/wnfs_wasm_bg.wasm

Sheds ~200 kB

1 Failing Test

  1) [safari] › private.spec.ts:194:7 › PrivateDirectory › basicMv can move content between directories 

    Test timeout of 600000ms exceeded.

    page.evaluate: Target closed

      193 |
      194 |   test("basicMv can move content between directories", async ({ page }) => {
    > 195 |     const [imagesContent, picturesContent] = await page.evaluate(async () => {
          |                                                         ^
      196 |       const {
      197 |         wnfs: { PrivateDirectory, PrivateForest, Namefilter },
      198 |         mock: { MemoryBlockStore, Rng },

        at /home/foobar/cc/rs-wnfs/wnfs-wasm/tests/private.spec.ts:195:57

    Pending operations:
      - page.evaluate at tests/private.spec.ts:195:57

@pinkforest pinkforest requested a review from a team as a code owner January 30, 2023 20:14
@zeeshanlakhani
Copy link
Contributor

@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.

@pinkforest
Copy link
Contributor Author

pinkforest commented Jan 30, 2023

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.

@matheus23
Copy link
Member

1 Failing Test

Hm. That doesn't seem to fail in CI, so should be ok. I wonder what's happening on your end.

The release profile would be here only for us in the workspace to test the .wasm codesize out -

Not only that - we prepare the .wasm and ship it when we bundle the npm package. So it practically reduces our npm package size 🙌

I do wonder whether we should have separate "debug" and "release" .wasms though. That all depends on how well Wasm can be debugged in browsers. For now it's pretty bad, so let's optimize the size :P


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 🤞

Copy link
Member

@matheus23 matheus23 left a 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

@appcypher
Copy link
Member

appcypher commented Feb 17, 2023

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 wnfs-wasm doesn't work either, we should consider move these settings to .cargo/config.toml.

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.

@appcypher
Copy link
Member

Reached similar file size with a config that targets just wasm32 arch.
https://github.com/wnfs-wg/rs-wnfs/compare/appcypher/chore-strip-flags.

Copy link
Member

@matheus23 matheus23 left a 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?

@pinkforest pinkforest closed this Feb 20, 2023
@pinkforest pinkforest deleted the chore-strip branch February 20, 2023 21:16
@pinkforest pinkforest mentioned this pull request Feb 20, 2023
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.

4 participants