-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: add wasm-bindgen-futures as an optional runtime for AsyncDB, target wasm32-unknown-unknown support #49
base: master
Are you sure you want to change the base?
Conversation
Add the `wasm-bindgen-futures` webtime, with support for `wasm32-unknown-known`. Tests can be run with `wasm-pack test --node -- --no-default-features --features asyncdb-wasm-bindgen-futures`
0904852
to
b783c4c
Compare
snapshot_counter: &mut usize, | ||
message: Message, | ||
) { | ||
match message.req { |
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.
can you refactor run_server()
to use match_message()
?
mut snapshots: HashMap<usize, Snapshot>, | ||
mut snapshot_counter: usize, | ||
) { | ||
if let Some(message) = recv.recv().await { |
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.
what's the plan/idea behind this? For AsyncDB
, it's clear: run the blocking I/O bound code on a background thread to keep the main loop responsive.
Why then implement the background thread using asynchronous I/O? If
- you don't have real threads in whatever runtime you're running your code, and run this in a background task on Tokio/Async-std/whatever, you can do all DB operations in-line in your tasks, with less overhead (no channel communication etc.);
- if you do have real threads in your runtime, you can just use the original implementation.
From what I can see here it look like we're dealing with case 1, that's how spawn_local()
is documented in the wasm_bindgen_futures
crate.
What am I missing?
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.
Yeah, it's a bit weird to be honest (and even weirder when you try an implement an Env that will work in WASM/the browser).
My goal was to get this to work with wasm-bindgen-futures, so that it can run in a WASM environment in the browser. Right now, that would mean in-memory only, although I was starting to poke at what a WASM-based Env would look like to run it e.g. in a browser.
The JS runtime is single-threaded, but uses Promises for asynchronous scheduling, so that the browser doesn't block on waiting for something. Though, working through the Env trait, I think it might be more challenging to implement (e.g. how do we sleep in WASM when we don't have time available), so any wasm32-unknown-unknown target would be in-memory only.
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.
(and ultimately it would be backed by IndexedDB, which is LevelDB at it's core so it's a bit of a :spider-man-pointing: situation -- maybe it's better to build what I'm building in IndexedDB directly for WASM and use this for the non-WASM Rust stuff)
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 agree with your last point: unless the IndexedDB API is so restrictive as to be useless (I don't know it myself), it is probably best to use it directly, and instead abstract LevelDB under a common database interface (instead of abstracting storage below LevelDB). It might save you some time and sweat.
This PR builds on #48 (and requires #48 as a base) to add additional support for
wasm-bindgen-futures
on theAsyncDB
implementation.It adds the additional feature flag
asyncdb-wasm-bindgen-futures
.