Skip to content
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

matklad's dream fastrand #40

Open
matklad opened this issue Nov 8, 2022 · 2 comments
Open

matklad's dream fastrand #40

matklad opened this issue Nov 8, 2022 · 2 comments

Comments

@matklad
Copy link

matklad commented Nov 8, 2022

I love fastrand, it's the closest to the ideal prng crate for me, but it is annoyingly not quite there. As I've been thinking about this today, I decided to write my thoughts down. This is very explicitly not a proposal for a change -- I think there's just a difference in values between what fastrand is, and what I want from my idiosyncratic crate. Still, might be interesting for someone. Or maybe one day I'll fork more_opinionated_fastrand or something :)

The overall theme is "remove magic, it's ok if the user needs to write some code themselves if that leads to greater clarity".

So, the list of not-suggested changes:

  • remove interior mutability, the user can wrap in the Cell if they need two.
  • bias creation API towards requiring a seed
#[derive(Debug, PartialEq, Eq)]
pub struct Rng { s: u64 }

impl Rng {
    pub fn new(seed: u64) -> Rng {
        Rng { s: seed }
    }
    #[cfg(feature = "std")]
    pub fn from_entropy() -> Rng {
        Rng::new(random_seed())
    }
    pub fn seed(&self) -> u64 {
        self.s
    }
}

/// get me a random without getrandom
#[cfg(feature = "std")]
fn random_seed() -> u64 {
    std::hash::Hasher::finish(&std::hash::BuildHasher::build_hasher(
        &std::collections::hash_map::RandomState::new(),
    ))
}
  • remove magical Clone, it should either not exist, or create an identical Copy
  • remove Default
  • remove thread-local and global API, to bias towards using seed and passing rng explicitly. Again, the user can wrap the thing into a thread local if they need to.
  • fn u32(impl RangeBounds) -> fn u32(); fn u32_range(impl RangeBounds). .. just looks weird at the call-site
  • fn bool() for a unbiased coint flip, fn bool_ratio(num: u32, denom: u32) for num/denom biased flip
@notgull
Copy link
Member

notgull commented Nov 9, 2022

Hello!

Regarding your first and second points, having a version of fastrand that can be no_std and Sync has been on my to-do list for a while. I'm not a fan of the API changes; IMO we should value the case where we don't mind if a seed is generated for us over the other cases.

I agree with the magical Clone, see #38.

My general policy is that, if something can implement Default, it should implement Default. There are certain containers that assume this.

I'd be against removing the thread-local API. One of this crate's primary purposes is to serve as a global PRNG for some of our crates that need it, like futures-lite and async-executor (off the top of my head, there are probably others). I'd avoid pushing in the direction to make this harder. That being said, I wouldn't be opposed to moving this API to behind a feature flag.

I personally disagree with the separate u32_range function, as I don't really mind the .. at the call site.

I'd be for adding a bool function, not so sure about bool_ratio.

@fogti
Copy link
Member

fogti commented Feb 17, 2023

interior mutability and Clone are now fixed on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants