dylanhart / ulid-rs

This is a Rust implementation of the ulid project
https://crates.io/crates/ulid
MIT License
389 stars 37 forks source link

allow sharing Generator across threads #21

Closed camerondavison closed 4 years ago

camerondavison commented 5 years ago

I created this example https://github.com/camerondavison/ulid-rs/blob/threads/examples/threads.rs because I was having trouble figuring out how to share the Generator across threads.

I think that the Box trait needs Send added to it, but ThreadRng is not Send compatible since it does all the init per thread. So that might mean that the Generator may need to optionally have a RngCore or use a ThreadRng instead of saving it, or use a different RngCore that has Send

Or maybe I am thinking about this the wrong way

dylanhart commented 5 years ago

I wrote a detailed reply and never clicked send... Will type it up again tonight.

dylanhart commented 5 years ago

The reason that I used Box<RngCore> is so that the inner rng would be type erased and would not need to be specified like Generator<ThreadRng>. Naturally since we don't know the type of the RngCore, we can't implement Send. (Not that we would be able to even if we knew the type because ThreadRng is ~Send) But as I was looking at this, I think that this is just the wrong approach. We shouldn't be forcing the rng across threads, because, ultimately, the rng used isn't important. The functionality we are looking for is "Can generate monotonic ulids across threads" and not "Can use the same generator + rng object across threads".

I'm thinking that this can best be done by having multiple generator objects that share the prev ulid, but have their own rng objects.

generator

The shared state is forced to be Sync, but individual generators don't have to be. Now in order to get the shared state to be Sync there needs to be some kind of wrapper for prev. The simple answer is Mutex but mutex accesses are very slow. I'm thinking that a spin lock of some kind would be better since the amount of time accessing shared state will be in the nanosecond range. I'm not sure how to implement this in Rust, but that can be looked up.

The shared state also has to be shared. Wrapping it in an Arc should be good for that.

I'm thinking of something like the following for the API, but really this needs a bit more thought.

struct ThreadGenerator {
    state: GeneratorState,
    source: Box<RngCore>,
}

impl ThreadGenerator {
    // same generate method as Generator
}

struct GeneratorState {
    prev: Arc<SpinLock<Ulid>>,
}

impl GeneratorState {
    fn new() -> Self { /* ... */ }
    fn new_generator(&mut self) -> ThreadGenerator { /* ... */ }
    fn new_generator_with_source(&mut self, /* ... */) -> ThreadGenerator { /* ... */ }
}
camerondavison commented 5 years ago

Thanks for taking the time to explain your thoughts on this. I really appreciate it and am learning more about rust through helping out on this project.

Would it make sense to do the same thing as the other ulid methods and just add a generate with source instead of managing the shared rng state. Then just take the rng out of the current state and leave everything how it is.

This would cause whomever is calling it to manage the rng but that is pretty easy with thread local rng.

dylanhart commented 5 years ago

That would work, but I don't think that it would be as nice to use. Makes sense for a first version though