-
Notifications
You must be signed in to change notification settings - Fork 432
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
Add impl code for wasm target #197
Conversation
I produced the following example using this patched version of The code is simple (using #[macro_use]
extern crate lazy_static;
extern crate rand;
extern crate pcg_rand;
use pcg_rand::Pcg32Basic;
use rand::{Rng, SeedableRng};
use std::os::raw::c_double;
use std::sync::Mutex;
lazy_static! {
static ref RNG: Mutex<Pcg32Basic> = Mutex::new(Pcg32Basic::from_seed([42, 42]));
}
fn main() {}
#[no_mangle]
pub extern "C" fn get_rand() -> c_double {
let rng = &mut RNG.lock().unwrap();
rng.next_f64()
} |
Is it random when the first value is always Seriously, it would be good to test seeding, as mentioned here. Could you test seeding from |
Just tried replacing
I saw the If you want to generate entropy from wasm, I think the best would be to use an extern function. The easiest primitive that comes to mind is passing a random double from Javascript to Rust (see below). This means that, if someone wants to use // From the Rust side
extern "C" fn random_double() -> c_double; // From the Javascript side
function random_double() {
Math.random();
} |
That's not very satisfactory, but not too surprising. Sounds like it's not possible to get nanosecond time in JS so JitterRng is not useful. Getting secure random numbers in JS may be more involved. |
Is using |
Regardless of the mechanism you use, relying on an Here is an example: // Functions that will be passed the wasm module
let imports = {
env: {
random_double: Math.random
}
}
// Code to load the wasm module In my opinion, this library is usable even in case there is no fallback rng. That means users are forced to seed rngs manually, but that is better than having no library at all. It fits my use case, anyway. Would it make sense to merge this PR and figure out later what to do about a fallback for OsRng? |
Yes, probably, though I'm a little wary of mis-use (of However you'll also need to patch #196, assuming that merges first. |
For the time being I'd personally go with one of two strategies: (a) the one in this PR or (b) fail the compile of the whole crate with a better error message. For now I think of those two this PR is the better alternative (just stubs out the "OS" rng for wasm). I'd probably recommend avoid requiring an import on the instantiation side of things (aka |
Yeah, so that's the tricky bit. People are experimenting with using procedural macros to help generate the right import stuff automatically, but that's also tricky... |
@alexcrichton I agree that requiring an import is undesirable and that this PR in its current form is a good solution for the time being. |
Sounds sensible to me though I haven't looked into WASM much. #196 is now merged which probably also needs fixing; @aochagavia can you add a patch to disable that too? (Alternatively it could be given a different timer, but without nanosecond precision it will fail at runtime anyway.) |
@dhardy I rebased my branch so there are no conflicts anymore. I also provided an implementation of |
src/jitter.rs
Outdated
@@ -693,6 +693,11 @@ fn get_nstime() -> u64 { | |||
t as u64 | |||
} | |||
|
|||
#[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] | |||
fn get_nstime() -> u64 { | |||
panic!("Not supported") |
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 you modify test_timer
to return an appropriate TimerError
instead of panicking?
I guess this impl needs to stay in any case (for linking); shame but I guess we have to live with redundant error code. Actually, you should be able to use unreachable!()
here.
@dhardy Thanks for your feedback. Just amended the commit. |
Could we get a new version of rand pushed to crates.io that includes this PR? That would be wonderful. Thanks! |
Thinking about this some more, why choose to fail at run-time over selectively disabling unavailable functionality at compile time? The latter is how we've enabled It might make sense to change this before making a release — unless we plan to provide an alternative entropy source somehow? |
This is how the standard library is doing it as well. E.g. even though wasm has no way to manage/start threads of its own, the standard library has threading support for wasm, but if you try to create a thread you will get an error. Or open files, etc. I think the general advantage of this approach is that stuff still compiles and larger codebases are easier to port over. wasm can call into js which then has access to an entropy source, but the issue is that this involves manual involvement by the user... there is no way right now for a crate to present a js function and say "please add that to the imports so that rust code can call it". |
@dhardy could we get the current state out to crates.io? Right now there is a build failure of the crate with wasm, forcing you to use a git dep on latest master. Any release on crates.io doesn't mean we will have to commit to anything in the future, as long as rand is not 1.0 yet. Thanks! |
Add impl code for wasm target
still fail with yew |
@dhardy thanks! It did fixed the issue! |
No description provided.