[Merged by Bors] - Remove rand crate from dependency tree#3992
[Merged by Bors] - Remove rand crate from dependency tree#3992BGR360 wants to merge 1 commit intobevyengine:mainfrom
rand crate from dependency tree#3992Conversation
This replaces `rand` with `fastrand` as the source of randomness for `HandleId::new()` in `bevy_asset`. This was the only crate with a dependency on `rand`, and now the dependency exists only as a dev-dependency. `fastrand` was already in the dependency tree, thanks to `futures-lite`, `async-executor`, and `tempfile` to name a few.
|
It's worth discussing if this potentially degrades the uniqueness of |
alice-i-cecile
added
A-Assets
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
LGTM. I'm idly curious about the performance difference, but I don't think it would change my mind; stripping out heavy dependencies is generally good.
|
Fastrand uses wyhash as prng and seeds with time and pid. Rand uses a csprng for |
|
As long as the IDs are unique, is more predictability actually something bad for asset handles? |
|
For asset ids no. However for sources of randomness directly used by the game more predictability is a bad thing. While we can move bevy_assets to fastrand, whatever crate will expose a source of randomness to the user will still need to use rand. |
For the record, there is currently no such crate in bevy, as far as i could see. |
|
It has been proposed but not (yet) accepted: #2504 |
|
I would like to understand what problem this PR solves as it's not covered in the description. Unless there's a reason, I am opposed to this change as |
|
The problem being solved is reducing unnecessary dependencies. I was under the impression that this was one of the core tenets of the Bevy philosophy. The |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
@alice-i-cecile A long time ago I actually did a benchmark between a few RNG crates. It might very well be wrong but for me fastrand indeed turned out to be the fastest: https://gist.github.com/r00ster91/817f4691b7d240c5908c64bde6bb1f7f (needs nightly)
[src/main.rs:59] std_rng_duration = 12.914µs
[src/main.rs:59] micro_rand_duration = 8.526µs
[src/main.rs:59] nanorand_duration = 5.6µs
[src/main.rs:59] oorandom_duration = 4.769µs
[src/main.rs:59] small_rng_duration = 2.034µs
[src/main.rs:59] fastrand_duration = 1.854µs
(Output manually sorted)
That's an improvements of >85%!
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
@BGR360 can you retitle this to "Remove And add the following to your PR description Once that's done I'm happy to merge this. |
|
@alice-i-cecile done! |
|
Blocking this for now: as @TheRawMeatball points out, can we just use a global atomic rather than a random UUID? That would be my preferred solution if possible. |
|
Sorry, where did @TheRawMeatball point this out? |
|
On Discord a couple minutes ago, in #engine-dev. I just wanted to give credit where it's due :) Ongoing discussion in this thread: https://discord.com/channels/691052431525675048/968230879317078116 |
alice-i-cecile
removed
the
S-Ready-For-Final-Review
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
This is clearly correct, and fixes #1376, which is in the milestone. Merging now, we can hammer out the broader questions on "should we be using global atomics" after. |
|
bors r+ |
This replaces `rand` with `fastrand` as the source of randomness for `HandleId::new()` in `bevy_asset`. This was the only crate with a dependency on `rand`, and now the dependency exists only as a dev-dependency. `fastrand` was already in the dependency tree, thanks to `futures-lite`, `async-executor`, and `tempfile` to name a few. ## Changelog Removed `rand` from dependencies in `bevy_asset` in favor of existing in-tree `fast-rand`
bors
bot
changed the title
rand crate from dependency treerand crate from dependency tree
This replaces `rand` with `fastrand` as the source of randomness for `HandleId::new()` in `bevy_asset`. This was the only crate with a dependency on `rand`, and now the dependency exists only as a dev-dependency. `fastrand` was already in the dependency tree, thanks to `futures-lite`, `async-executor`, and `tempfile` to name a few. ## Changelog Removed `rand` from dependencies in `bevy_asset` in favor of existing in-tree `fast-rand`
This replaces `rand` with `fastrand` as the source of randomness for `HandleId::new()` in `bevy_asset`. This was the only crate with a dependency on `rand`, and now the dependency exists only as a dev-dependency. `fastrand` was already in the dependency tree, thanks to `futures-lite`, `async-executor`, and `tempfile` to name a few. ## Changelog Removed `rand` from dependencies in `bevy_asset` in favor of existing in-tree `fast-rand`
This replaces `rand` with `fastrand` as the source of randomness for `HandleId::new()` in `bevy_asset`. This was the only crate with a dependency on `rand`, and now the dependency exists only as a dev-dependency. `fastrand` was already in the dependency tree, thanks to `futures-lite`, `async-executor`, and `tempfile` to name a few. ## Changelog Removed `rand` from dependencies in `bevy_asset` in favor of existing in-tree `fast-rand`
6 participants
This replaces
randwithfastrandas the source of randomness forHandleId::new()inbevy_asset. This was the only crate with a dependency onrand, and now the dependency exists only as a dev-dependency.fastrandwas already in the dependency tree, thanks tofutures-lite,async-executor, andtempfileto name a few.Changelog
Removed
randfrom dependencies inbevy_assetin favor of existing in-treefast-rand