emilk
Allow usage with atomics as well as wgpu on wasm32
Is your feature request related to a problem? Please describe.
I'd like to move some single threaded heavy work that creates a lot of data into a background thread on wasm, and put a lot of time into preparing to do this with shared memory. At the same time I do render the results of that calculation, and may in the future move some parallelizable work, into webgpu shaders. So I use egui-wgpu. But egui-wgpu depends on wgpu with fragile-send-sync-non-atomic-wasm. This means that its fundamentally incompatible with wasm_thread, wasm-bindgen-rayon and wasm-mt, as well as any other project that wants to use Wasm-Workers with SharedArrayBuffer.
Describe the solution you'd like Ideally of course the removal of that feature. But some indication why this was required would be helpful already, and perhaps a guess about the difficulty of getting rid of this feature.
If no solution is reached, then a warning in some spot would be very helpful. Someone that uses webgpu could be expected to also want to have shared memory multithreading on the web. Had I known about this beforehand I had not put so much energy into solving other issues around having that kind of multithreading.
Describe alternatives you've considered Well, getting rid of WebGPU isn't gonna be easy at this point, and likely will also make performance much worse. So IDK.
Additional context This commit seems to have introduced this feature. Its a merge commit, in that merge, this commit seems to have introduced it. In that case all it did was remove a conditional import of either the concurrent or non-concurrent typemap. I don't know if the gained concurrency was used for something else somewhere else.
I've tested my application with a patched eframe etc, with the relevant changes simply reverted, and it seems to work without issue.
Note that that is on the commit just before switching to wgpu 22.0, as my application is currently stuck at 0.20.1 because of another dependency.
@emilk my suggestion would be to put this behind a default feature flag, to avoid breaking anyone depending on this (unless they use no-default-deatures, but IDK what to do about that). I've not got any issues with the application since I switched to my patched branch. The pipeline is broken in that project, but with it behind a feature flag, that is likely to be fixed. You would not actively test that flag that way, of course, but I guess that'd be my weight to carry.
Would you accept a PR that introduces such a feature and documents it?
@9SMTM6 I looked around a bit to familiarize myself with the story here again and I agree with your rationale & proposal! Putting send/sync egui-wgpu renderer/callbacks behind a feature flag makes perfectly sense me.
Personally, I'd be even leaning towards making it opt-in, but I know that a lot of people are struggling with the Send/Sync restriction and not a lot of folks are actually using atomics on the web so it's probably better to have it opt-out as you propose (i.e. all the same reasons wgpu has fragile-send-sync-non-atomic-wasm in the first place!)
You would not actively test that flag that way, of course, but I guess that'd be my weight to carry.
I think there should be a cargo check job on ci (as part of check_wasm in rust.yml) with the right feature flag combination to ensure that this code path doesn't break at random. I figure that should catch almost all issues that would ensue from the "web-atomics/no-web-atomics variants" :)
Would you accept a PR that introduces such a feature and documents it?
Emil pinged me to have a look at this issue, so you can see this as a yes 😄
Thank you so much for looking into this and willing to contribute your findings back upstream! :)
Personally, I'd be even leaning towards making it opt-in
@Wumpf I've thought a bit about this while creating the actual PR, but I decided to stick with it being on by default.
The original reason, not making a breaking change, doesn't really pull I guess. egui has plenty of breaking changes here and there, thankfully usually easy to solve, as would probably be the case here.
However it being enabled by default in here is better for discoverability, IMO. The default is to have the 'same behavior' on native and web, which makes things easier for normal users, and then when one wants to do multithreading, there is a nice compiler error in wgpu. That should point people to the wgpu flag, and hopefully they will find the respective egui-wgpu flag as well. If the feature is off by default, if people use frame, finding the corresponding flag in egui-wgpu isn't that easy, since you depended on it indirectly, and don't get a compiler error that points you to at least the start of the fix.
To that point, I'm not sure if it might be better to keep the same name as wgpu, or use the IMHO easier to understand name I gave it in my PR. For people that know their way around cargo.toml it should still be easy enough to spot, and I thought this makes it easier to understand for newcomers looking at these flags.
And thank you for the kind words:).