Use NonNull inside ThreadRng handles #827
Conversation
src/rngs/thread.rs
Outdated
| @@ -80,7 +78,9 @@ thread_local!( | |||
| /// | |||
| /// For more information see [`ThreadRng`]. | |||
| pub fn thread_rng() -> ThreadRng { | |||
| ThreadRng { rng: THREAD_RNG_KEY.with(|t| t.get()) } | |||
| let raw = THREAD_RNG_KEY.with(|t| t.get()); | |||
| let nn = unsafe { NonNull::new_unchecked(raw) }; | |||
There was a problem hiding this comment.
I would prefer to use NonNull::new(raw).unwrap() here, unless there is a benchmark demonstrating an unacceptable overhead.
There was a problem hiding this comment.
I don't think there's any situation where UnsafeCell::get() could return null, seems counter-productive to add a runtime check for a scenario that can never happen in order to enable an optimization somewhere else. I'll change it if you want though.
There was a problem hiding this comment.
If there is a very high performance cost of using safe code it might make sense to use the unsafe version. Did you run a benchmark for thread_rng()? If there is no performance difference, the safe version is preferable.
There was a problem hiding this comment.
I suppose this only matters if the user is calling random() or thread_rng().gen() in a tight loop, which isn't the recommended practice for performance anyway. Saving the ThreadRng handle and using it repeatedly will perform exactly the same either way.
|
Yeah, it's just a minor memory saving. There's probably no reason to have an |
Just some low hanging fruit to enable the null pointer optimization
Also removed an outdated comment regarding
Rc