web: Build two WASM modules, with/without extensions, load the appropriate one #5834

torokati44 · 2021-12-10T00:45:57Z

This includes, "unblocks", and extends #5225.

Also adds dynamic fallback to an additional, non-extension-enabled WebAssembly module, mostly for compatibility with ~~poor~~ Safari.

~~The simd128 extension is not yet directly utilized by us (it possibly is by some dependencies), but I have a few ideas where to use it, improving video decoding performance significantly.~~
EDIT: The simd128 feature should help a lot with the color conversion of both H.263 and VP6 videos since ruffle-rs/h263-rs#15 got merged. The IDCT in the H.263 decoder could also potentially benefit from it with some tweaking, but I haven't found/made any speedups there yet.

This also roughly (but not quite) doubles both the time it takes to build the web version, and the size of all web-based release packages.

What I don't like is that completely blanking out RUSTFLAGS also disables the web_sys_unstable_apis config option, which is needed for WebGPU.
We could just brush over this, saying that if browser compatibility is the entire reason for this fallback module, then surely WebGPU is not important. (Even though, Safari does have experimental support for it.)
EDIT: RUSTFLAGS is now set locally and properly for both modules, always setting the web_sys_unstable_apis config option.

torokati44 · 2021-12-10T00:54:49Z

If someone more experienced in TS finds a way to avoid having to upgrade to es2022 and enabling the topLevelAwait experiment, please let me know!

EDIT: The topLevelAwait experiment is no longer needed, and the upgrade is only to es2020 now! Thanks a lot, TypeScript Discord! ☺️

torokati44 · 2021-12-10T01:06:00Z

As this also ~~negatively affects CI times~~ makes CI fail, it would be nice if this dual-module machinery could be easily disabled.
Then it could even be off by default perhaps, only enabled for the releases.
Just where to put this switch and how to make it work well?

~~EDIT: CI is now fixed, so maybe turning it off is not that critical.~~
EDIT: Only a single module is built now by default, both for development an CI, with a copy of it standing in for the other one as well.

torokati44 · 2021-12-11T21:04:48Z

I'm thinking about adding a custom cargo profile for either (or both) of the wasm builds, to not jeopardize the build cache every time. Although that's a fairly new feature. And for frequent builds during development, using sccache is advisable anyway.
What do you think?

Herschel · 2021-12-15T20:25:39Z

My opinion is that anything in Rust stable is fair game, so I think it's worth giving the custom profiles a try!

torokati44 · 2021-12-17T01:10:41Z

It looks like until the linked cargo issue is addressed, we're going to be stuck with setting the different RUSTFLAGS in package.json... :/
Not really tragic, but not that neat either...

torokati44 · 2021-12-17T22:30:47Z

Okay, I think I made up my mind about this for now.
As is now, the first config is all-in (at least with the 4 relevant extensions), and the fallback is pure vanilla.
This way, Chrome and Firefox users get the full benefits of their choice, while the other build is guaranteed to keep working everywhere it did until now. Safari users are not benefited in any way (not even on 15.2, because of SIMD), but they are not worse off either. (And of course, as a general rule, only Apple can know what's best for them, I surely don't.)

Later, we could raise the bar on the fallback module as well, starting with extensions that worked on Safari 14.1 already (which is just signExtensions, if it matters), then maybe continuing with those that came in with Safari 15 (so, bulkMemory and saturatedFloatToInt).

If you disagree, I'm all ears. Well, of course, I am, even if you agree...

EDIT: I also just confirmed that passing --enable-sign-ext --enable-nontrapping-float-to-int --enable-simd --enable-bulk-memory to wasm-opt as well has no influence on the output whatsoever.

torokati44 · 2021-12-20T23:47:26Z

Does any NPM or shell scripting whiz perhaps have any tips on how we could skip building the vanilla .wasm module on CI, while keeping cross-shell compatibility? :/
It should just quietly not run any of the ...-noext commands if the CI envvar is set.

danielhjacobs · 2021-12-21T17:38:02Z

You could try prepending the ...-noext commands with [[ -z $CI ]] &&

See https://stackoverflow.com/questions/39619956/short-way-to-run-command-if-variable-is-set, and test it yourself.

[[ -z $USERNAME ]] && echo "Not defined" should print nothing on linux, but [[ -z $USERNAM ]] && echo "Not defined" should print "Not defined" (unless you set USERNAM).

torokati44 · 2021-12-21T19:18:00Z

I don't think this works in CMD.EXE as well... :/ That's the hard part. But thanks anyway!

danielhjacobs · 2021-12-22T02:43:13Z

This works on any system with Python:

python -c "import os, sys;sys.exit(1) if 'CI' in os.environ else sys.exit(0)" && echo 'a'

But needing to install python for CI may be asking a bit much...

torokati44 · 2021-12-22T02:44:26Z

A Node equivalent of this did come up... But I tried to find a simpler solution.

torokati44 · 2022-01-09T22:33:46Z

Okay, I think I resolved all comments that I could, and brought it again to a presentable state.
So, this is ready for the next round!

relrelb · 2022-01-10T07:12:15Z

.github/workflows/release_nightly.yml

@@ -268,7 +268,7 @@ jobs:
        shell: bash -l {0}
        run: |
          npm run bootstrap
-          npm run build
+          npm run build:full-release


Quite petty, but maybe rename this to build:full?

Hmm, I'm not sure about this one... Feels less descriptive to me, while I'm not seeing how it's any better. What do you think, @adrian17?

Personally, I lean towards build:release. I understand and somewhat agree with shortening this, but build:release seems to match better with build:debug. As in "this is the debug version. this is the release version". Maybe wait for an opinion from Adrian too though. Calling this the full version, as you mention, feels less descriptive.

FWIW:

So, I am fine with build:release as well.

but build:release seems to match better with build:debug
As in "this is the debug version. this is the release version".

This is my reasoning:
The issue is that these days, "release" means "optimized", not "this is what we use to release the software".

For comparison, these are (very general) build configurations for Firefox:

max debug, unoptimized; you basically never want to actually run this

normal optimized build (what cargo or Visual Studio would call "release")

increasingly fancy configurations with LTO, PGO, branding etc - for actual software release or more thorough tests and benchmarks

And these more or less correspond to our configurations:

max debug, unoptimized; you basically never want to actually run this

optimized (current npm run build)

optimized with extra debug info/avm tracing (current npm run build:debug)

increasingly fancy configurations with special wasm features

Now, when a dev comes and wants to build it, they most likely want "normal optimized build". But for a Rust programmer, this would probably be cargo run --release. But if they apply this logic here and run build:release, they get "final-release" and not "development-release" :) Which is why I'd prefer to reserve some more explicit wording for "this is max optimized, not for everyday development".

That isn't perfect either: Even for the plain build command now, -O is passed to rustc and wasm-opt is also called on the output either way... :|

Yeah, I realized that and deleted my comment suggesting build:optimized. Maybe build:fully-featured, but if the purpose of renaming is to shorten the command, that sort of defeats the purpose.

I propose yet another variant: build:dual-wasm :| - This one doesn't have release it.
And build:dual-wasm-release is way too long again?

@relrelb Could you please elaborate on what you had against build:full-release originally?

Anyway, what do you and @danielhjacobs think about build:dual-wasm?

I'm fine with dual-wasm

I don't have a strong preference to any name (this is a classical example for bikeshedding), I just thought build:full is a little more concise and descriptive name. I'm fine with staying on build:full-release, changing to build:dual-wasm, or even changing to something else.

torokati44 · 2022-01-11T23:25:22Z

I get the feeling that the only thing holding this up now is the name of that script. And I am not sure how to resolve this distributed decision, with all options having pros and cons brought up by different contributors. Which is awkward.
Honestly, just having this finally in is more important to me now than this single final detail.
And I don't even know who should have the final say in this bikeshedesque question.

web/package.json

web/packages/core/tsconfig.json

…riate one

…for the other And enable the module that really uses WebAssembly extensions for the releases by running the new "npm run build:dual-wasm" command, which sets the ENABLE_WASM_EXTENSIONS=true environment variable.

torokati44 · 2022-01-12T09:30:37Z

So, if nobody has anything against npm run build:dual-wasm (vs. npm run build:full-release earlier, or any other option that came up), I think this is ready ... again! :D

relrelb

LGTM!

relrelb

LGTM!

Herschel

Let's do it! Thank you!

torokati44 · 2022-01-12T19:59:41Z

Woooh yeah, thanks a whole hecking bunch and a half to everyone involved! 🥳

torokati44 force-pushed the dual-wasm branch from 3c1d776 to 4d12931 Compare December 11, 2021 20:59

torokati44 force-pushed the dual-wasm branch 4 times, most recently from 35d2f9d to c6d16ea Compare December 11, 2021 23:18

torokati44 marked this pull request as draft December 12, 2021 00:35

torokati44 force-pushed the dual-wasm branch from 8de1c11 to cff36d0 Compare December 12, 2021 11:46

torokati44 marked this pull request as ready for review December 12, 2021 20:40

torokati44 force-pushed the dual-wasm branch from cff36d0 to 267c741 Compare December 13, 2021 20:58

torokati44 mentioned this pull request Dec 13, 2021

Allow specifying rustflags per profile rust-lang/cargo#7878

Closed

torokati44 force-pushed the dual-wasm branch from 267c741 to 3a2c48b Compare December 17, 2021 22:22

torokati44 force-pushed the dual-wasm branch from 3a2c48b to 3550155 Compare December 17, 2021 22:47

torokati44 mentioned this pull request Dec 17, 2021

yuv: Drop the LUTs, do integer-only (fixed-point), SIMD-capable arithmetic instead ruffle-rs/h263-rs#15

Merged

torokati44 force-pushed the dual-wasm branch 5 times, most recently from b593677 to 6737f70 Compare December 22, 2021 00:49

torokati44 force-pushed the dual-wasm branch from 6737f70 to 05e59d8 Compare December 30, 2021 19:22

torokati44 force-pushed the dual-wasm branch 6 times, most recently from 5904768 to dd00bc8 Compare January 9, 2022 22:03

relrelb reviewed Jan 10, 2022

View reviewed changes

torokati44 force-pushed the dual-wasm branch 3 times, most recently from 6cecc2f to 7696479 Compare January 11, 2022 22:26

torokati44 force-pushed the dual-wasm branch from 7696479 to 2e620bc Compare January 11, 2022 23:59

relrelb reviewed Jan 12, 2022

View reviewed changes

web/package.json Outdated Show resolved Hide resolved

relrelb reviewed Jan 12, 2022

View reviewed changes

web/packages/core/tsconfig.json Show resolved Hide resolved

torokati44 mentioned this pull request Jan 12, 2022

web: Migrate to npm workspaces #6025

Merged

torokati44 added 3 commits January 12, 2022 10:27

web: Build two WASM modules, with/without extensions, load the approp…

5d9dda6

…riate one

web: Detect and log whether WebAssembly extensions are actually used

3060999

torokati44 force-pushed the dual-wasm branch from 2e620bc to 3060999 Compare January 12, 2022 09:27

relrelb approved these changes Jan 12, 2022

View reviewed changes

Herschel approved these changes Jan 12, 2022

View reviewed changes

Herschel merged commit db6731b into ruffle-rs:master Jan 12, 2022

This was referenced Jan 12, 2022

web: Enable the bulk-memory WebAssembly extension #5225

Closed

web: Add custom profiles to separate the build cache for the two wasm modules #6043

Merged

Replacing memset and memcpy calls with memory.fill and memory.copy? WebAssembly/binaryen#4403

Open

danielhjacobs mentioned this pull request May 16, 2023

core: Implement handling of text control input #11059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: Build two WASM modules, with/without extensions, load the appropriate one #5834

web: Build two WASM modules, with/without extensions, load the appropriate one #5834

torokati44 commented Dec 10, 2021 •

edited

Loading

torokati44 commented Dec 10, 2021 •

edited

Loading

torokati44 commented Dec 10, 2021 •

edited

Loading

torokati44 commented Dec 11, 2021

Herschel commented Dec 15, 2021 •

edited

Loading

torokati44 commented Dec 17, 2021

torokati44 commented Dec 17, 2021 •

edited

Loading

torokati44 commented Dec 20, 2021

danielhjacobs commented Dec 21, 2021 •

edited

Loading

torokati44 commented Dec 21, 2021

danielhjacobs commented Dec 22, 2021 •

edited

Loading

torokati44 commented Dec 22, 2021

torokati44 commented Jan 9, 2022

relrelb Jan 10, 2022

torokati44 Jan 10, 2022

danielhjacobs Jan 11, 2022

torokati44 Jan 11, 2022

adrian17 Jan 11, 2022

danielhjacobs Jan 11, 2022

torokati44 Jan 11, 2022

torokati44 Jan 11, 2022

danielhjacobs Jan 11, 2022

relrelb Jan 12, 2022

torokati44 commented Jan 11, 2022 •

edited

Loading

torokati44 commented Jan 12, 2022

relrelb left a comment

relrelb left a comment

Herschel left a comment

torokati44 commented Jan 12, 2022

web: Build two WASM modules, with/without extensions, load the appropriate one #5834

web: Build two WASM modules, with/without extensions, load the appropriate one #5834

Conversation

torokati44 commented Dec 10, 2021 • edited Loading

torokati44 commented Dec 10, 2021 • edited Loading

torokati44 commented Dec 10, 2021 • edited Loading

torokati44 commented Dec 11, 2021

Herschel commented Dec 15, 2021 • edited Loading

torokati44 commented Dec 17, 2021

torokati44 commented Dec 17, 2021 • edited Loading

torokati44 commented Dec 20, 2021

danielhjacobs commented Dec 21, 2021 • edited Loading

torokati44 commented Dec 21, 2021

danielhjacobs commented Dec 22, 2021 • edited Loading

torokati44 commented Dec 22, 2021

torokati44 commented Jan 9, 2022

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

Choose a reason for hiding this comment

torokati44 commented Jan 11, 2022 • edited Loading

torokati44 commented Jan 12, 2022

relrelb left a comment

Choose a reason for hiding this comment

relrelb left a comment

Choose a reason for hiding this comment

Herschel left a comment

Choose a reason for hiding this comment

torokati44 commented Jan 12, 2022

torokati44 commented Dec 10, 2021 •

edited

Loading

torokati44 commented Dec 10, 2021 •

edited

Loading

torokati44 commented Dec 10, 2021 •

edited

Loading

Herschel commented Dec 15, 2021 •

edited

Loading

torokati44 commented Dec 17, 2021 •

edited

Loading

danielhjacobs commented Dec 21, 2021 •

edited

Loading

danielhjacobs commented Dec 22, 2021 •

edited

Loading

torokati44 commented Jan 11, 2022 •

edited

Loading