Remove last non-dev dependency on rand crate

[dekellum]

Motivation

Minimizing dependencies to reduce build size and time for typical tokio applications.

Background

In parking_lot 0.9.0 (upgraded here recently in #1298) the rand dependency was replaced entirely with its own private Xorshift PRNG, used for its fairness mechanism.

The tokio-threadpool also has its own Xorshift PRNG for "fair" work stealing, introduced in #466 as a performance improvement. However the rand dependency was kept, its use limited to producing the initial seeds for the thread local Xorshift PRNGs. This is currently the only existing non-dev dependency on rand, direct or transitive, in tokio. As mentioned, parking_lot no longer depends on rand, and tempfile (rand dep) is also only a dev dependency.

Solution

This PR changes the seed generation to piggyback on libstd's mechanism for obtaining system randomness for the default hasher (as used by HashMap for HashDoS resistance), and hashing on the current thread id. Based on some eprintln testing of seeds and Xorshift output, this strategy appears more than adequate for the use case of approximating fairness.

cc: @carllerche @stjepang @Amanieu @faern

[davidbarsky]

This is really great to see! Two questions, primarily out of curiosity:

• What was the impact, if any, on compile times on your personal computer?
• Does the overall number of dependencies significantly?

[dekellum]

Rand 0.7 is better than 0.6 was, but its still 9 crates total that can be avoided. To test this, I made a standalone project out of "tokio/examples/hello_world.rs" and patched in tokio master from local git tree, with all rand crates and other dependencies already downloaded:

before (patched to current master, d0bb161)

% time cargo build
real	0m28.327s
user	1m26.310s
sys	0m5.002s

% du -hs target/
253M	target/

% cargo clean

% time cargo build --release
real	0m58.074s
user	3m27.050s
sys	0m4.468s

% du -hs target/
85M	target/

% cargo clean

after (patch tree updated to this PR, 1b0e41a)

% cargo update
    Removing autocfg v0.1.5
    Removing c2-chacha v0.2.2
    Removing getrandom v0.1.6
    Removing ppv-lite86 v0.2.5
    Removing rand v0.7.0
    Removing rand_chacha v0.2.0
    Removing rand_core v0.5.0
    Removing rand_hc v0.2.0
    Removing spin v0.5.0

% time cargo build
real	0m25.559s
user	1m15.476s
sys	0m4.558s

% du -hs target/
236M	target/

% cargo clean

% time cargo build --release
real	0m54.466s
user	3m11.480s
sys	0m3.758s

% du -hs target/
77M	target/

[davidbarsky]

That's a really solid improvement! I can't speak to the code changes, but in principle, +1.

[faern]

It's probably really bad if crates here and there start inventing their own seeding and PRNG implementations. The reason we wanted to really get away from it in parking_lot was because it's going to be included in libstd, so it can't depend on crates from crates.io. It was not to cut down on build times or shrink the dependency tree.

As Amanieu points out, everyone else should depend on rand. See this comment: Amanieu/parking_lot#138 (comment)

[faern]

If all you need is some entropy to seed something, maybe don't use rand directly, but use getrandom which is what rand uses underneath: https://crates.io/crates/getrandom

[faern]

I agree it's a problem that rand is so large that people hesitate to include it. But if everyone start vendoring their own stuff it's eventually going to end really bad I think. A much better approach is to depend on something that exists and focuses on providing quality entropy, and if it's too big, push them to create smaller subset crates for more targeted use case. I agree depending on all of rand with a large number of PRNG algorithms in it feels bloated when all you need is some simple entropy.

Again, I can't stress enough how one should not use parking_lot as a guiding example here. We did not do it because rand was bloated, but because we will be included inside libstd and can't have dependencies really.

[dekellum]

It's probably really bad if crates here and there start inventing their own seeding and PRNG
implementations.

@faern, your concern would seem to apply firstly to #466 which has been merged for while. That author found custom Xorshift PRNG significanty faster then using rand s XorShiftRng for same purpose.

In this PR, I'm basically just trying to complete that process and reap the associated dependency minimization benefits. Yes getrandom is a possible alternative for seeding, which adds back one dep. I don't know of any upside to that alternative though.

[...] should not use parking_lot as a guiding example here. We did not do it because rand was bloated, but because we will be included inside libstd.

A fringe benefit then, IMO.

[faern]

A benefit of using getrandom is that you will get good entropy with a single function call. Without risking implementing something that might not yield good or any randomness. Does not have to be maintained and audited by this crates maintainers. And without introducing undocumented magic constants such as

        0x9b4e_6d25 // misc bits

getrandom only has two non-optional dependencies: libc and lazy_static which is already in the dependency tree of tokio. So it would be an extremely cheap dependency to add.

@faern, your concern would seem to apply firstly to #466 which has been merged for while

I'm not familiar with that PR since I'm not really associated with tokio. I just randomly landed on this PR just now thanks to a github notification. But yes, on first sight that PR does not sound optimal to me. Would have been better to try rand_pcg maybe. It's way faster than rand_xorshift in my experience.

I'm basically just trying to complete that process and reap the associated dependency minimization benefits.

This makes it sound like you are only removing stuff that is unused. But tokio does currently use rand for entropy. And this PR is inventing its own algorithm for doing that instead. So IMO a more fair description of this PR is "Replace rand with homegrown randomness generator"

[faern]

My personal opinion is that this project should try out some other low dependency PRNG using rand_core as base. For example rand_pcg. Then none of the seeding would need to be implemented, because the SeedableRng::from_entropy() would take care of that automatically.

[dekellum]

randomly landed

No, I friendly cc'd you because you were the author of the same Xorshift PRNG in parking_lot_core:

Amanieu/parking_lot@4d78285cef7

without introducing undocumented magic constants

Actually even with getrandom alternative, replacing a 0 seed with misc bits (or any non-zero value, would 1 be less bothersome to you?), given the very basic/fast nature of Xorshift PRNG of #466.

[faern]

replacing a 0 seed with misc bits (or any non-zero value, would 1 be less bothersome to you?)

Yes. Even if still not optimal it would better signal "Just not zero" IMO. Some other constant, like the one here at least directly makes me think about why this number was chosen. In cryptography in general there are a lot of magic constants chosen for some unique proven property about them. So when reading this code my first reaction is that this value is somehow important in itself.

I strongly believe that the best is to avoid magic numbers, and the second best is to use the least surprising ones if they are needed. 1 in definitely less surprising than 0x9b4e_6d25.

And I do realize we did not do very well in parking_lot on this area, but it would be good to document why it can't be zero. In order to not cause a future where people are afraid of touching code because it says it's important but they don't know why :)

I see the cc now. Missed that on the first glance, sorry about that.

[archseer]

I think rand_pcg could be a good choice. It only depends on rand_core and there are no other dependencies (apart from autocfg).

[dekellum]

Thanks for feedback to date. I've used it to improve the seed function and its comments. Stepping back slightly, please consider the following prior art using this same 32-bit Xorshift PRNG:

In the libstd, pattern-defeating quicksort, where the same 32-bit Xorshift PRNG is seeded with the sort slice buffer length (obviously predictable).

In Amanieu/parking_lot#144 where the same 32-bit Xorshift PRNG is used, here seeded by a 1-based incremented atomic (±predictable).

Here via #466, currently and as in this PR, with per-thread unpredictable seeds.

All of these use cases desire a very fast PRNG with minimal dependencies and can easily afford significant statistical bias in the PRNG output. As far as seeding, we can see in the above that much less care was given to it in other places. I don't think I've "invented" anything in this PR, but rather just found some appropriate if novel use of public libstd interfaces. Would anyone CC'd suggest I also offer this better, unpredictable seeding function to parking_lot or libstd for that matter?

Regarding PCG

To be clear, I'm not personally going to propose replacing Xorshift-32 with PCG-32, because I simply don't think its necessary for this use case. This is largely orthogonal to both the dependencies question/goal and the seeding question. If someone wanted to separately propose changing to PCG-32, I would recommend just swapping in the 4-lines of the PCG-32 with the 3-lines of Xorshift-32 here.

Alternative: Use getrandom crate for seeding

First, this would still need to be combined with some per-thread counter or other mechanism, so as to avoid the cost of system random bytes per-thread created. Also rand's getrandom crate, which was originally extracted from libstd, is returning to its origin, see rust-lang/rust#62079, rust-lang/rust#62082! So in a future rust stable release, the change of this PR will be using getrandom. Its better for users to not take on a second copy of getrandom as a dependency of tokio when it can be reasonable avoided.

[carllerche]

I'm OK w/ this strategy. The requirements for the random generator here are pretty minimal. Reducing the number of deps seems good 👍

[carllerche]

I probably wouldn't bother with a lazy_static. Instead, why not create one when the pool is created and pass the seed in to each worker when that is initialized?

[dekellum]

I actually tried that first, but get compile errors because &self is not constant (in fn rand_usize) for the thread local PRNG. It is an appropriate compiler lint, I think, because if its thread local then it needs to be the same across all pools... I mean, to the extent that the same thread could even be a part of multiple pools (a detail the compiler apparently doesn't know).

[carllerche]

Ok, not critical for merging this PR. We can follow up if we care.

[carllerche]

@faern I hear your concern, but RandomState seems sufficient for the needs here.