seq WASM compatibility #33

celaus · 2018-07-03T07:47:06Z

Hi! So these are the (minor) changes to the seq module (incl. the update to the WASM compatible 0.5 rand crate).

I think there are a few things that are worth doing:

the iteration timing is useful for wherever Instant is available, WASM bindgen doesn't support it yet though.
the random thread_rng number generator doesn't work with WASM bindgen yet, a good solution would probably be to be able to pass in a custom RNG (like Math.random or XorShiftRng).

This PR should mostly show the required changes to seq. I am happy to use a completely different approach too!

Tracked also in #32

m-decoster

Thanks a lot for your pull request. I have left some comments on your code that I would like to address before we merge this. Unfortunately, I have little time this month, so I will probably not be able to respond quickly to changes / questions.

m-decoster · 2018-07-09T17:05:45Z

src/sim/seq.rs

@@ -185,7 +174,7 @@ where
    /// Kill off phenotypes using stochastic universal sampling.
    fn kill_off(&mut self, count: usize) {
        let ratio = self.population.len() / count;
-        let mut i = ::rand::thread_rng().gen_range::<usize>(0, self.population.len());
+        let mut i = XorShiftRng::new_unseeded().gen_range::<usize>(0, self.population.len());


It looks like new_unseeded is deprecated and you should use the FromEntropy or SeedableRng trait instead.

Yes, I know - fromEntropy won't work with WASM though, I was trying to find something that is sufficiently random and available, but I think it will come back to the user to take care of seeding it properly. This is an architecture issue, I'd say - because PRNGs will repeat after a while which means passing in a RNG is going to different than what it is now (I think).

But in any case, passing in a seed will need an interface, the question is where 😄

I would think that passing in the seed would go well in the SimBuilder. You could have a method that sets the seed, and have a default seed as well. That way you should be able to use Seedable, not break the interface, and remove this deprecated call 😃

m-decoster · 2018-07-09T17:08:16Z

src/sim/seq.rs

@@ -120,15 +118,6 @@ where
            }

            self.iter_limit.inc();
-            self.duration = match self.duration {


Rather than remove this code, I would refactor it to a free function (not exported) and have that function have a different implementation based on the target. The regular implementation would be this removed code, while the WASM implementation would simply return None.

Could you also tell me how you compiled this project for WASM (I tried building it and got over 90 errors for undefined identifiers, such as c_long)?

I'll get something up soon that demonstrates that

about the Instant stuff, it could be as easy as an if/else Option each time, I just thought that there might be a better idea 😆 I guess we can improve later on as well though

I'll get something up soon that demonstrates that

Thanks 👍. That will allow me to help you better.

about the Instant stuff [...]

I don't really understand what you mean here, but we can take a look at it once I get your PR to compile

I successfully build it like that:

cargo +nightly build --target wasm32-unknown-unknown wasm-bindgen target/wasm32-unknown-unknown/release/wasm_tspsolver.wasm --nodejs

m-decoster · 2018-07-09T17:09:16Z

src/sim/seq.rs

@@ -27,8 +27,10 @@ use super::*;
 use super::select::*;
 use super::iterlimit::*;
 use super::earlystopper::*;
-use std::time::Instant;
+use std::boxed::Box;


I'm not sure why you need this but it might be needed somewhere. Just make sure to remove unused use statements if this isn't needed 😄

True laziness on my side. will be gone soon 👍

celaus · 2018-07-10T20:39:06Z

@m-decoster oh no worries, I am unexpectedly busy too. I am happy with the pace 👍

celaus · 2018-08-07T11:11:16Z

Hi @m-decoster, I added something "new". In a word, it's now possible to collect various statistics from each step's population fitness. This should replace (and expand!) the ability to collect step timings but has the drawback of adding a type parameter that has to be specified for the default builder() method.

Take a look and let me know what you think!

Since we talked last there has also been a fix for rand to natively work with wasm-bindgen but it's SUPER slow unfortunately... wasm-bindgen is actually working on exposing the JS Math.random() (which is faster) by default, but as of now there is no common interface to use the same Random number generator. Hence I have put in a compilation switch for wasm :( and resorted to the XorShiftRng PRNG with a terrible seed. I'll update this in a next step.

From my view, the remaining things are:

Reduce code duplication with the compiler switch
Implement a step timer StatisticsCollector to replace step timings
update docs/Readme
tests 😅

celaus · 2018-08-25T14:02:18Z

@m-decoster friendly ping! 😄 Happy to discuss the changes whenever you have time

m-decoster · 2018-09-09T09:12:29Z

@celaus Sorry, I will look at it ASAP.

m-decoster · 2018-09-11T18:44:22Z

@celaus Unfortunately I am still unable to build the library with the wasm32-unknown-unknown target, unlike other Rust libraries. I used the command cargo +nightly build --target wasm32-unknown-unknown, by the way.

The issues originate in libc, and are all of the form cannot find type 'c_char' in this scope.

As far as I can tell the problem lies in the crossbeam-deque dependency. Running cargo tree here is my output:

├── rand v0.5.5
│   ├── libc v0.2.32
│   └── rand_core v0.2.1
└── rayon v1.0.2
    ├── crossbeam-deque v0.2.0
    │   ├── crossbeam-epoch v0.3.1
    │   │   ├── arrayvec v0.4.7
    │   │   │   └── nodrop v0.1.12
    │   │   ├── cfg-if v0.1.4
    │   │   ├── crossbeam-utils v0.2.2
    │   │   │   └── cfg-if v0.1.4 (*)
    │   │   ├── lazy_static v1.0.1
    │   │   ├── memoffset v0.2.1
    │   │   ├── nodrop v0.1.12 (*)
    │   │   └── scopeguard v0.3.3
    │   └── crossbeam-utils v0.2.2 (*)
    ├── either v1.2.0
    └── rayon-core v1.4.0
        ├── crossbeam-deque v0.2.0 (*)
        ├── lazy_static v1.0.1 (*)
        ├── libc v0.2.32 (*)
        ├── num_cpus v1.7.0
        │   └── libc v0.2.32 (*)
        └── rand v0.3.17
            └── libc v0.2.32 (*)

So the problem basically lies in the rayon dependency. By any chance did you remove this dependency locally? Or did you disable the max_unstable module that depends on rayon?

Since you are able to get it building, could you post detailed instructions?

UPDATE: I managed to get it to build but removing the references to rayon. While this allows me to review the code for now I would still like to have this module be fixed before merging this pull request.

m-decoster · 2018-09-11T18:59:46Z

I also took the liberty of cleaning up some code. Overall it looks quite good, if we can get a PoC working of RsGenetic running in the browser that would be cool.

Anyway, here's the commit: 411b068. I will create a PR on your repo as well so that you can integrate this change.

celaus · 2018-09-11T19:52:42Z

Hi @m-decoster - thanks for looking at this :)

I am not sure why this doesn't build for you, it does for me without any issue, rayon enabled and nothing changed that isn't in the PR. In fact, I was surprised that crossbeam and rayon simply compiled to be honest.

I have a version of this running on Azure Functions (i.e. Node 😄 ). I'll experiment and try to break my build, I'll let you know what I get.

10:54:57 ~/w/M/RsGenetic{master} > cargo tree
rsgenetic v1.7.14 (file:///home/cm/workspace/Mine/RsGenetic)
├── rand v0.5.5
│   ├── libc v0.2.43
│   └── rand_core v0.2.1
└── rayon v1.0.2
    ├── crossbeam-deque v0.2.0
    │   ├── crossbeam-epoch v0.3.1
    │   │   ├── arrayvec v0.4.7
    │   │   │   └── nodrop v0.1.12
    │   │   ├── cfg-if v0.1.5
    │   │   ├── crossbeam-utils v0.2.2
    │   │   │   └── cfg-if v0.1.5 (*)
    │   │   ├── lazy_static v1.1.0
    │   │   │   [build-dependencies]
    │   │   │   └── version_check v0.1.4
    │   │   ├── memoffset v0.2.1
    │   │   ├── nodrop v0.1.12 (*)
    │   │   └── scopeguard v0.3.3
    │   └── crossbeam-utils v0.2.2 (*)
    ├── either v1.5.0
    └── rayon-core v1.4.1
        ├── crossbeam-deque v0.2.0 (*)
        ├── lazy_static v1.1.0 (*)
        ├── libc v0.2.43 (*)
        └── num_cpus v1.8.0
            └── libc v0.2.43 (*)
10:54:59 ~/w/M/RsGenetic{master} > git st
?? wasm.build.sh
10:55:10 ~/w/M/RsGenetic{master} > git log
commit 6c7329c0b1f58ac4763a40bc752ba53756a89e12 (HEAD -> master, origin/master, origin/HEAD)

seq WASM compatibility

6a215ad

m-decoster requested changes Jul 9, 2018

View reviewed changes

Finished an interface for collecting statistics

1434f0b

celaus added 3 commits August 7, 2018 13:24

code deduplication

cac99d4

adjusted the statscollector trait

7663128

fixed conditional build

6c7329c

ravicious mentioned this pull request Aug 6, 2021

Implement a genetic algorithm that can handle an arbitrary number of images ravicious/collage#4

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

seq WASM compatibility #33

seq WASM compatibility #33

celaus commented Jul 3, 2018

m-decoster left a comment

m-decoster Jul 9, 2018

celaus Jul 10, 2018

m-decoster Jul 11, 2018

m-decoster Jul 9, 2018

celaus Jul 10, 2018

celaus Jul 10, 2018

m-decoster Jul 11, 2018 •

edited

celaus Jul 19, 2018

m-decoster Jul 9, 2018

celaus Jul 10, 2018

celaus commented Jul 10, 2018

celaus commented Aug 7, 2018 •

edited

celaus commented Aug 25, 2018

m-decoster commented Sep 9, 2018

m-decoster commented Sep 11, 2018 •

edited

m-decoster commented Sep 11, 2018

celaus commented Sep 11, 2018 •

edited

seq WASM compatibility #33

Are you sure you want to change the base?

seq WASM compatibility #33

Conversation

celaus commented Jul 3, 2018

m-decoster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-decoster Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

celaus commented Jul 10, 2018

celaus commented Aug 7, 2018 • edited

celaus commented Aug 25, 2018

m-decoster commented Sep 9, 2018

m-decoster commented Sep 11, 2018 • edited

m-decoster commented Sep 11, 2018

celaus commented Sep 11, 2018 • edited

m-decoster Jul 11, 2018 •

edited

celaus commented Aug 7, 2018 •

edited

m-decoster commented Sep 11, 2018 •

edited

celaus commented Sep 11, 2018 •

edited