-
Notifications
You must be signed in to change notification settings - Fork 291
Add WASM32 core platform abstractions #5079
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
Conversation
0823d50
to
ec94b8c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5079 +/- ##
==========================================
- Coverage 85.84% 85.75% -0.09%
==========================================
Files 325 327 +2
Lines 35906 35954 +48
==========================================
+ Hits 30824 30834 +10
- Misses 5082 5120 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of #[cfg(target_arch = "wasm32")]
s. How about doing it like this:
#[cfg(not(target_arch = "wasm32"))]
pub use tokio::sync::{ ... };
#[cfg(target_arch = "wasm32")]
pub use self::async_lock_wasm::*;
#[cfg(target_arch = "wasm32")]
mod async_lock_wasm {
// all the current cfg'd items
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this. A similar approach would be:
#[cfg(not(target_family = "wasm"))]
mod sys {
pub use tokio::sync::{…};
}
#[cfg(target_family = "wasm"))]
mod sys {
pub use …;
}
pub use sys::*;
I would prefer this choice to be honest.
Also, please use target_family = "wasm"
if possible (see target_family
) rather than target_arch = "wasm32"
. wasm64
will come soon, and it will be a maintenance burden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this thread is irrelevant given the other one on this file ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will make these corrections in these shim files. It seemed like the other was the norm but happy to adjust it.
#[cfg(target_arch = "wasm32")] | ||
{ | ||
WasmRuntimeHandle | ||
} | ||
|
||
#[cfg(not(target_arch = "wasm32"))] | ||
{ | ||
async_compat::get_runtime_handle() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(target_arch = "wasm32")] | |
{ | |
WasmRuntimeHandle | |
} | |
#[cfg(not(target_arch = "wasm32"))] | |
{ | |
async_compat::get_runtime_handle() | |
} | |
#[cfg(target_arch = "wasm32")] | |
return WasmRuntimeHandle; | |
#[cfg(not(target_arch = "wasm32"))] | |
return async_compat::get_runtime_handle(); |
/// Future for the [`next`](super::StreamExt::next) method. | ||
#[cfg(target_arch = "wasm32")] | ||
#[derive(Debug)] | ||
#[must_use = "futures do nothing unless you `.await` or poll them"] | ||
pub struct Next<'a, St: ?Sized> { | ||
stream: &'a mut St, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed? I think futures_util::stream::StreamExt's next should just work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is the call to .boxed() on the stream it returns, the StreamExt has has a Send
trait requirement that prevents .boxed() from being called when used in situations like this:
let (initial, mut subscriber): (RoomPaginationStatus, BoxStream<'_, RoomPaginationStatus>) =
match self.inner.live_back_pagination_status().await {
Some((status, stream)) => (status, stream.boxed()),
None => {
let (status, stream) = self.inner.focused_pagination_status().await.context(
"can't subscribe to the back-pagination status on a focused timeline",
)?;
(status, stream.boxed())
}
};
Here is the definition of .boxed() from the futures_util::stream::StreamExt
/// Wrap the stream in a Box, pinning it.
///
/// This method is only available when the `std` or `alloc` feature of this
/// library is activated, and it is activated by default.
#[cfg(feature = "alloc")]
fn boxed<'a>(self) -> BoxStream<'a, Self::Item>
where
Self: Sized + Send + 'a,
{
assert_stream::<Self::Item, _>(Box::pin(self))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would strongly recommend https://docs.rs/n0-future/latest/n0_future/ instead of making our own abstractions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this suggestion, I will update this in the split out PR to use this library for the shim instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
I've a couple of remarks though.
- I think it's better to split your single patch into one patch per features: one patch for
async_lock
, one patch forstream
etc. It would help the reviewer, - I don't understand why
async_lock
is necessary at all sincetokio::sync
is Wasm-compatible (seetokio
Wasm support), - I suggest to use the
#[cfg(target…)] mod sys
+pub use sys::*
pattern more broadly to reduce most of the#[cfg(target…)]
you have; it will also greatly clarify the code! - I suggest to use
target_family = "wasm"
to target Wasm instead oftarget_arch = "wasm32"
, - I don't understand the need for
WasmRuntime
becausetokio::runtime
is Wasm-compatible (see the same link fortokio::sync
), - I don't understand why you re-implement
Next
, it is by-design Wasm-compatible. I understand the need to “alias”LocalBoxStream
toBoxStream
though.
In general, please justify the reason of being of those types in their respective documentation please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this. A similar approach would be:
#[cfg(not(target_family = "wasm"))]
mod sys {
pub use tokio::sync::{…};
}
#[cfg(target_family = "wasm"))]
mod sys {
pub use …;
}
pub use sys::*;
I would prefer this choice to be honest.
Also, please use target_family = "wasm"
if possible (see target_family
) rather than target_arch = "wasm32"
. wasm64
will come soon, and it will be a maintenance burden.
#[cfg(target_arch = "wasm32")] | ||
#[derive(Debug)] | ||
/// A dummy guard that does nothing when dropped. | ||
/// This is used for the WASM implementation to match | ||
/// tokio::runtime::EnterGuard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the #[…]
after the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright is missing, along with a comment explaining why this is needed, what it does etc.
#[cfg(not(target_arch = "wasm32"))] | ||
pub type Handle = tokio::runtime::Handle; | ||
#[cfg(not(target_arch = "wasm32"))] | ||
pub type Runtime = tokio::runtime::Runtime; | ||
|
||
#[cfg(target_arch = "wasm32")] | ||
pub type Handle = WasmRuntimeHandle; | ||
#[cfg(target_arch = "wasm32")] | ||
pub type Runtime = WasmRuntimeHandle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could become:
#[cfg(not(target_family = "wasm"))]
mod sys {
pub use tokio::runtime::{Handle, Runtime};
}
#[cfg(target_family = "wasm")]
mod sys {
pub struct Handle;
pub struct Runtime;
}
pub use sys::*;
actually no, tokio rt on wasm doesn't integrate with the browser (for fetch() etc), many wasm compatible crates require the wasm-bindgen-futures runtime |
Instead of scattered conditional compilation directives throughout the code, this commit consolidates platform-specific code into sys modules with the pattern: mod sys { // native implementations } mod sys { // wasm implementations } pub use sys::*; Eliminate the definition of the unnecessary async_lock
bde0663
to
de2fcc0
Compare
|
I believe this PR can be closed in favor of the smaller ones, leaving it open just in case there is any back-and-forth on existing comments. |
Thank you for your work <3! |
This commit adds foundational WASM32 support to matrix-sdk-common by introducing platform-agnostic abstractions for async operations:
These abstractions enable matrix-rust-sdk to run on both WASM32 and native targets without conditional compilation in consuming code.
This is part of a larger effort by Filament to add wasm32 support to the matrix-rust-sdk and matrix-sdk-ffi bindings. We have these bindings working on our fork, and are now taking steps to cherry pick them incrementally back to main to make them more reviewable.
Signed-off-by: Daniel Salinas