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

web: Enable the bulk-memory WebAssembly extension #5225

Closed
wants to merge 2 commits into from

Conversation

torokati44
Copy link
Member

@torokati44 torokati44 commented Sep 4, 2021

This should improve all around performance on web, to a varying degree.
I'll gladly provide benchmark numbers upon request; but so far I've seen that at least while decoding videos, this should save anywhere between 10% and 20% of processing time, "for free".

There are still quite a few calls to the naive memcpy and memset functions (explicit loop and load/store in WASM), mostly from core and std.
To get rid of these as well, the build-std cargo feature would be needed (which does something similar to what Xargo was for), but that is a nightly-only flag as of now, because there are still some issues with it.

This raises the minimum browser version requirements to:

I know basically nothing about the habits of 🍎 ecosystem users, but based on past data, Safari 15.0 is supposed to be the most used version by November, and is supposed to reach near its top market share by about January or February:

Source: https://gs.statcounter.com/browser-version-market-share

So, I don't think this should be merged for at least a couple weeks, but we can still talk about it here until then. :)

@adrian17
Copy link
Collaborator

adrian17 commented Sep 4, 2021

Can you mark it as either draft or smth like "[DO NOT MERGE YET]"? Just in case :)

Also, when the time comes, let's also test AVM-heavy code. I did find cases where extremely AVM-heavy code was slightly slower as bulk-memory has actually higher overhead than wasm memcpy for extremely small arrays. Since then rustc improved a bit and generates less memcpys, but let's see in several months.

@torokati44 torokati44 marked this pull request as draft September 4, 2021 22:21
@torokati44 torokati44 force-pushed the wasm-bulk-memory branch 2 times, most recently from 32c7891 to ebaa995 Compare September 5, 2021 00:46
@torokati44
Copy link
Member Author

I switched from setting RUSTFLAGS to adding .cargo/config.toml, both for the new flag, and for the static CRT flag on windows, for consistency's sake; and to make sure the value set for the envvar in the two separate places don't conflict.

Copy link
Contributor

@relrelb relrelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! LGTM

.cargo/config.toml Outdated Show resolved Hide resolved
@torokati44
Copy link
Member Author

I'd appreciate it if someone on Windows could check if the new condition for the crt-static target-feature indeed works as intended!

@Herschel
Copy link
Member

Herschel commented Sep 6, 2021

Unfortunately the windows setting in config.toml doesn't work because of rust-lang/cargo#6858. I think we'll have to stick with setting RUSTFLAGS on CI for that.

@torokati44
Copy link
Member Author

Oh... :/ Thanks for checking!

How about the original, less ideal version?

[target.i686-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

[target.x86_64-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

@torokati44
Copy link
Member Author

I'm just worried about the potential conflict where the CI setup (including the action building the nightly releases) sets the RUSTFLAGS envvar unconditionally (it's just empty on non-windows targets), and it always takes priority over what's in .cargo/config.toml.

@adrian17
Copy link
Collaborator

adrian17 commented Sep 6, 2021

Considering this is bound to wait for a couple more months, maybe for now we could just wait and see if toolchains figure out something nicer? I think long-term, rustc itself will be switching some of wasm features on by default.

@Herschel
Copy link
Member

Herschel commented Sep 6, 2021

[target.i686-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

[target.x86_64-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

This should be fine -- I also tried using desktop/build.rs but unfortunately error: Only -land-Lflags are allowed in build script ofruffle_desktop v0.1.0 (D:\projects\ruffle\desktop): -C target-feature=+crt-static``.

Should we think about shipping multiple WASM modules with features enabled/disabled, and loading the appropriate module based on browser capabilities? Maybe bulk extensions will be ubiquitous enough that it's not a big deal, but there will be other features that we may want to selectively enable as well (webgpu, SIMD, interface types, ...). This might allow us to keep running on slightly older browsers while these features roll out.

@torokati44
Copy link
Member Author

torokati44 commented Sep 7, 2021

Partly I'm also hoping for what @adrian17 wrote: That over (hopefully not too long) time, rustc/cargo itself will enable the "most important" extensions by default, so we won't have to deal with them, and can delegate the responsibility for the effects of this decision to the Rust team.

But I can also imagine building even just two WASM modules: one for the "happy path" (when support is detected for all the extensions we want), and one for compatibility (which enables no extensions at all, like now).

This would complicate the build process and the loader code, result in slightly larger releases and hosted package, and might even bring difficulties when debugging user issues... - for perhaps too slight a benefit. (Let it be some extra performance now, or legacy compatibility later...) Whether it's worth it or not, is of course not an easy question.

(Well, I, for one, am silently rooting that something like this can be made to work, but I understand that it's not trivial and not without downsides, so...)

@torokati44
Copy link
Member Author

Safari 15 is now out!!!! Everyone, update right away!!

@torokati44
Copy link
Member Author

torokati44 commented Sep 27, 2021

With Safari 14.1 already starting to slowly fade out in favor of 15.0, I'll leave the judgement to merge this at any time to someone else.

Here is the download link for the CSV data for these charts.

@torokati44 torokati44 marked this pull request as ready for review September 27, 2021 06:41
@kmeisthax
Copy link
Member

I'm going to point out that, unlike prior versions, Apple's decided to keep iOS 14 installable for an additional year. This may hamper adoption.

@torokati44
Copy link
Member Author

Oh no... :/
And can Safari not be updated to 15.0 on iOS 14?

@kmeisthax
Copy link
Member

Safari and iOS are one single thing - you can't update them independently. Also, Apple doesn't allow shipping third-party browser engines, so all alternate browsers on iOS are using the same version of Safari that ships with the OS.

@torokati44
Copy link
Member Author

Yeah, I knew about the web engine restriction, but not about last-gen iOS being stuck with the same version it came with... That's a bummer. Anyway, I can wait!

@Herschel
Copy link
Member

Herschel commented Oct 3, 2021

Getting a panic on VP6BR::parse_header in this file:
vp6.zip

@torokati44
Copy link
Member Author

I think the panic in VP6BR::parse_header is not related to this PR.
Just tested your file with this commit on top of the now-fixed #5369, and got no panic.

@torokati44
Copy link
Member Author

Gratuitous Safari market share update:
14.1 just dipped below 2%, and 15.0 is really close to 1%!

(Sorry for the transparent background...)

@Herschel
Copy link
Member

Herschel commented Oct 16, 2021

I guess we should decide when to pull the trigger on this -- possibly when 15.0 becomes majority, or wait even longer? Can iOS 15.0 run on every device that 14.0 can? We should also have a "you need bulk memory/update your browser" error message in place.

I think the panic in VP6BR::parse_header is not related to this PR.

Sorry, this comment was supposed to be on the VP6 PR!

@torokati44
Copy link
Member Author

torokati44 commented Oct 23, 2021

I guess we should decide when to pull the trigger on this -- possibly when 15.0 becomes majority, or wait even longer?

Here's yet another fancy graph, this time with linear extrapolations based on the last 1, 2, and 3 weeks.
Squinting at the lines hard enough, from far enough, I'd say 14.1 might dip below 1.0% right around Christmas. Based on this, my suggestion for an arbitrary point in time to merge this would be around New Year's Day, just to leave some slack.

Can iOS 15.0 run on every device that 14.0 can?

I, for one, have no idea. However, at least according to this and this list, looks like it should be the case. The latter includes the iPhone 13 variants, but no other difference.

We should also have a "you need bulk memory/update your browser" error message in place.

Would we want something like https://github.com/GoogleChromeLabs/wasm-feature-detect for this, or just another rung in the if-ladder in ruffle-player.ts that maps an error message to a PanicError?

@Herschel
Copy link
Member

If we can deduce this specific error from the panic, it'd be nice if we can avoid the dependency. But if this is difficult, we can rely on wasm-feature-detect.

@torokati44
Copy link
Member Author

torokati44 commented Nov 5, 2021

Oh no, what is happening?

EDIT:
So, it looks like, by the end of this year, Safari 15.0 is expected to have a -0.5% market share. :|

Are people reverting en masse, or was the initial bump just a little post-release hype?

@torokati44
Copy link
Member Author

torokati44 commented Nov 14, 2021

OOooohhh, Safari 15.1 has already been out for a while now, that's what's been happening!

Looks much better! :) How did I not think about this until now...

@torokati44
Copy link
Member Author

torokati44 commented Dec 10, 2021

BTW, just today, for the first time ever, Safari 15.x market share surpassed that of Safari 14.x!

EDIT: Hmm, but what about browsers on iWhatevers, whose engine is that of Safari? Do they advertise themselves as not-Safari, despite not being better than it in this regard? 🤔

@yakagami
Copy link

yakagami commented Dec 15, 2021

@torokati44 assuming the same website is correct, here is the iOS market share by version which also tells us the Safari version as they are updated together.
15.x seems to be sitting at 48% and is rising.

StatCounter-ios_version-ww-monthly-202112-202112-bar

Link:
https://gs.statcounter.com/ios-version-market-share/mobile-tablet/worldwide/#monthly-202112-202112-bar

And here is the last 3 months


StatCounter-ios_version-ww-monthly-202109-202112

Link:
https://gs.statcounter.com/ios-version-market-share/mobile-tablet/worldwide/#monthly-202109-202112

@torokati44 torokati44 marked this pull request as draft December 18, 2021 11:55
@torokati44
Copy link
Member Author

Superseded by #5834! Take that, Safari! 😃

@torokati44 torokati44 closed this Jan 12, 2022
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.

6 participants