[WIP] Implement Fill for some MaybeUninit types
#1241
Conversation
It can be beneficial for users to avoid initialization in hot code. Also, init elimination is hard for LLVM because some of `Fill::try_fill` for some types marked as `#[inline(never)]`.
|
I want to add MaybeUninit impls for u16, u32, u64, usize, u128, etc but this would require some API changes for RngCore trait because How is better to implement this? Can I change existing APIs or should I just add new (especially for inner parts like rng_core which still available to end-users). Also, would need to add such method to Another option is to accept |
| // Safe to assume init because inner values have uninit type. | ||
| mem::MaybeUninit::uninit().assume_init() |
There was a problem hiding this comment.
Did you read the doc on assume_init? This is not safe.
It sounds like you want to cast [MaybeUninit<T>; N] to &mut [T]. This is explicitly not allowed by the Rust memory model.
There was a problem hiding this comment.
Nope, you are wrong.
This code is same as in MaybeUninit docs actually.
https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element
And I make an array of MaybeUninits which can be created from uninit array.
There was a problem hiding this comment.
Correct. It's just your comment that is wrong then: the reason it is safe is that the outer type is an array of MaybeUninit.
Benchmarks please. If you're introducing new |
This was discussed before, in #1080 I think (possibly elsewhere too). Summary: strong justification is required. But I agree that new API in |
AngelicosPhosphoros
changed the title
Fill for some MaybeUninit types (Need help)Fill for some MaybeUninit types
There was a problem hiding this comment.
I'm going to close this PR as "not very useful" (see individual comments).
Note that it doesn't really address #1080 anyway, which requires requires changes allowing RngCore fill an uninitialized byte-array directly (the element-wise fill used here probably negates any potential performance benefits).
| macro_rules! impl_fill_delegate_to_maybe_uninit{ | ||
| () => {}; | ||
| ($t:ty) => { | ||
| impl Fill for [$t] { | ||
| fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> { | ||
| Fill::try_fill( | ||
| unsafe { | ||
| slice::from_raw_parts_mut(self.as_mut_ptr() | ||
| as *mut mem::MaybeUninit<$t>, | ||
| self.len() | ||
| ) | ||
| }, | ||
| rng, | ||
| ) | ||
| } | ||
| } | ||
| }; | ||
| ($t:ty, $($tt:ty,)*) => { | ||
| impl_fill_delegate_to_maybe_uninit!($t); | ||
| impl_fill_delegate_to_maybe_uninit!($($tt,)*); | ||
| }; | ||
| } | ||
|
|
||
| impl_fill_delegate_to_maybe_uninit!(bool, char, f32, f64,); |
There was a problem hiding this comment.
This part doesn't make much sense to me: replacing what was a safe impl of Fill for [bool] (etc.) with an unsafe one which lowers to [MaybeUninit<bool>] (etc.).
| impl Fill for [$t] { | ||
| impl Fill for [mem::MaybeUninit<$t>] { | ||
| fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> { | ||
| for elt in self.iter_mut() { | ||
| *elt = rng.gen(); | ||
| *elt = mem::MaybeUninit::new(rng.gen()); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementing Fill for [MaybeUninit<bool>] (etc.) is okay in itself, but I don't think it very useful. In order to use it with uninitialized memory, assume_init is needed somewhere, which thus requires the user use unsafe code (and based on the assumption that fill succeeded). I mean, users can use this API correctly, but not easily, and it isn't difficult to implement equivalent functionality in userspace anyway.
It can be beneficial for users to avoid initialization in hot code.
Also, init elimination is hard for LLVM because some of
Fill::try_fillfor some types marked as#[inline(never)].