Add no_std support specifically for wasm32v1-none - #4630
Conversation
2b8b9f8 to
f9526f6
Compare
no_std support specifically for wasm32v1-noneno_std support specifically for wasm32v1-none
ogoffart
left a comment
There was a problem hiding this comment.
Nice work.
I'm thinking it might not be wise to remove all the caching. One can use the once_cell::race that you use anyway.
Currently tested using cargo check -p winit --target wasm32v1-none --all-features.
Can we add that on the CI?
There was a problem hiding this comment.
Just a question: why do we need this module instead of always using libm directly?
There was a problem hiding this comment.
I expect Winit would want to avoid using libm on platforms which have std (since the std implementations are what are used right now and can have differences in performance and precision). This stub module makes it clear what methods are actually needed from libm and avoid polluting the use sites with conditional compilation logic.
| impl_dyn_casting!(IconProvider); | ||
|
|
||
| #[derive(Debug)] | ||
| #[cfg_attr(all(target_family = "wasm", target_os = "none"), non_exhaustive)] |
There was a problem hiding this comment.
Any reason not to make it non_exhaustive in all cases?
There was a problem hiding this comment.
Purely to preserve the API as-is for std equipped platforms. I think it would make sense to leave it permanently non_exhaustive. But, IO in core is already on nightly, so this may be able to be removed in a not-too-distant MSRV bump anyway.
| loop { | ||
| match receiver.try_recv() { | ||
| Ok(Some(value)) => break value, | ||
| _ => core::hint::spin_loop(), |
There was a problem hiding this comment.
This is quite suspicious.
Is there really no way to block here?
I think we can still have threads with Web Worker, and spinlock doesn't seem like a good idea.
There was a problem hiding this comment.
core and alloc provide no mechanism to block or yield execution, but web-sys might provide something we could use (I think a timeout causing a yield, but there might be something more direct). I'll look into this a bit more.
There was a problem hiding this comment.
Ok so I believe this is the best we can do on web. The std::sync::Mutex implementation internally using a futex with atomics enabled, or just a boolean otherwise (arguing that the platform has no threads and therefore cannot contest). The futex internally just calls spin_loop while waiting to unlock, and without atomics yield_now is an empty function (again, no threads, so nothing to yield to).
The only way to enter into this loop is if you're in a worker and call Dispatcher::queue. Once you call that function, the work must be performed on the main thread, and the function must return the value produced by the work done on the main thread. Since the worker is blocked on the main thread, there's nothing we can do other than spin. The use of Condvar doesn't change the equation here either, since that internally just calls try_unlock in a loop on Wasm without atomics.
I do agree that it's not ideal, but this is a natural consequence of the function signature of queue.
|
|
||
| #[derive(Debug)] | ||
| struct SharedSurfaceSize { | ||
| inner: PhysicalSize<AtomicU32>, |
There was a problem hiding this comment.
Nice that it works. But now it is two different atomic and so it is possible for one thread to read partial result (the width has been set from one thread, but not the height)
It might not matter if all the read and right are expected to be on the same thread, as i don't think in practice, SurfaceSizeWriter is often being sent to a different thread. But maybe then it should be changed to a RefCell and remove the Send bound.
Or use an AtomicU64 instead of a PhysicalSize
There was a problem hiding this comment.
I originally used a packed AtomicU64 for this, but there are quite a few targets which currently don't support 64-bit atomics but do support 32-bit. I summarised that for foldhash here if you'd like to see exactly what I'm referring to. Since winit-core currently only uses AtomicUsize, adding a use of AtomicU64 would break any of those targets (which may or may not be currently supported). I don't believe there's a risk of actually observing unmatched width and height values, since the current logic already assumes the call to window_event will cause the SurfaceSize to be correctly updated. Since there's no synchronisation logic that actually ensures that is the case, winit must already be resilient to the surface size potentially not being updated correctly after window_event returns. So the worst case scenario remains a two-step resize, with the new possibility of updating either the width or height early.
31af106 to
67ed1f7
Compare
I would like to, but because EDIT: Added |
0daa2d6 to
be2d802
Compare
|
|
c0376ca to
528308d
Compare
Certain `f64` methods are only available with `std`, so `libm` is required to fill the gaps.
Since the navigator is available and provides consistent results across the main thread and workers, a global value is appropriate. Using a `OnceBox` would be appropriate, but a `OnceRef` avoids the one-off allocation at the expense of a bit of verbosity.
`Document` is only available from the main thread, so this function can only be called in the context of the main thread. Therefore, the `thread_local` that was originally caching the result can be replaced with a global static instead.
Requires a patch to `web-time` at time of writing
Description
wasm32v1-none#4629Details
This PR demonstrates adding
no_stdsupport towinitspecifically forwasm32v1-none, affectingwinit-core,winit-web, andwinit.Note that this currently requires a patch for
web-time:The published version does not include support for
wasm32v1-none, but it is implemented on the repository's main branch. The changes in this PR are still usable regardless, but actual compilation onwasm32v1-nonerequires the above patch.Since this PR doesn't target general
no_stdsupport, no new feature gates are required.Notable Changes
winit-coreSurfaceSizeWriterhas been refactored to usePhysicalSize<AtomicU32>instead of aMutex, but that change is abstracted around a newSurfaceSizeWriterHandletype. I believe this simplifies the API for users while also makingno_stdsupport simpler.libmexclusively forall(target_family = "wasm", target_os = "none")(wasm32v1-none). It's technically required for allno_stdplatforms, but this is the onlyno_stdtarget that this PR unblocks, so no need to add a feature gate for it.wasm32v1-nonewill markBadIconas non-exhaustive to avoid dealing with theOsErrorvariant. This can be removed oncecore::iois stable.wasm32v1-nonemarksWindowEventasnon_exhaustiveas theDataTransferReceivedvariant requiresdyn TypedData, which requires IO, which is currently nightly-only forno_std.winit-webonce_cellas a dependency. This is acceptable in my opinion, asonce_cellwas already a transitive dependency throughwasm-bindgen(and others).async::channelhas been modified to useConcurrentQueuerather thanstd::mpsc::channel.async::Dispatcheruses a channel to return values rather than aMutex/Condvarcombo. Since Wasm has a stubbedstdI don't believe thread yielding is actually more performant that busy waiting.OnceLockhave been replaced withonce_cell::race::{OnceBox, OnceBool, OnceRef}, andOnceCellwhere appropriate.thread_localhave either been removed entirely where it is relatively cheap to just recompute the stored value or replaced with a global static where it is appropriate to share said value between threads.winitwinit'scfg_aliaseshave been updated to includewasm32v1-noneas aweb_platform.Testing
Currently tested using
cargo check -p winit --target wasm32v1-none --all-features.Checklist
changelogmodule if knowledge of this change could be valuable to usersSurfaceSizeWriteris really visible, and actual compilation onwasm32v1-nonestill requires a patch.Notes