Skip to content

Conversation

@sorairolake
Copy link
Contributor

Implement Random for [T; N], where Random is implemented for T.

rust-lang/libs-team#393 (comment) states:

The trait Random will initially be implemented for all iN and uN integer types, isize/usize, and bool, as well as arrays** and tuples of such values.

This PR is also based on #130703 (comment).

Tracking issue: #130703

Implement `Random` for `[T; N]`, where `Random` is implemented for `T`.
@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 8, 2025
@sorairolake
Copy link
Contributor Author

r? @joboet

@rustbot rustbot assigned joboet and unassigned ibraheemdev Feb 8, 2025
@joboet
Copy link
Member

joboet commented Feb 8, 2025

I think it would be better to implement this with a dedicated method on the Random trait, or at least adding a specialization for the integer primitives. Otherwise generating e.g. a [u8; _] will call into the system for each element instead of just once to fill the entire buffer, which is very wasteful and slow.

@the8472
Copy link
Member

the8472 commented Feb 8, 2025

Otherwise generating e.g. a [u8; _] will call into the system for each element instead of just once to fill the entire buffer, which is very wasteful and slow.

That doesn't necessarily need specialization. We could add an internal BufferedRandomSource akin to a BufReader and then discard it at the end of the array filling.
We only want the OS source to be ~unbuffered to avoid fork/VM cloning issues. But if you're cloning a VM in the middle of array-filling you'll already end up with some duplicated "random" data anyway, so this should be fine I think.

@abgros
Copy link

abgros commented Feb 8, 2025

Calling random() for every element in the array is extremely wasteful because as @joboet said it does a separate system call for each element. I benchmarked the following functions using criterion:

fn random_array() -> [u8; 1_000] {
	array::from_fn(|_| u8::random(&mut DefaultRandomSource))
}

fn random_array2() -> [u8; 1_000] {
	let mut arr = [0; 1_000];
	DefaultRandomSource.fill_bytes(&mut arr);
	arr
}

Calling fill_bytes() a single time is over 50x faster.

random_array            time:   [42.631 µs 43.067 µs 43.875 µs]
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

random_array2           time:   [617.27 ns 626.50 ns 641.50 ns]
Found 11 outliers among 100 measurements (11.00%)
  6 (6.00%) high mild
  5 (5.00%) high severe

Comment on lines 433 to 438
let mut buf = [const { MaybeUninit::uninit() }; N];
for elem in &mut buf {
elem.write(T::random(source));
}
// SAFETY: all elements of the array were initialized.
unsafe { mem::transmute_copy(&buf) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a version that uses fill_bytes().

Suggested change
let mut buf = [const { MaybeUninit::uninit() }; N];
for elem in &mut buf {
elem.write(T::random(source));
}
// SAFETY: all elements of the array were initialized.
unsafe { mem::transmute_copy(&buf) }
let mut buf = [const { MaybeUninit::<T>::uninit() }; N];
// SAFETY: this initializes every element in the buffer with random bytes.
unsafe {
let slice = core::slice::from_raw_parts_mut(&raw mut buf as *mut u8, N * mem::size_of::<T>());
source.fill_bytes(slice);
mem::transmute_copy(&buf)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unsound for generic Ts. There's no guarantee that a Type's random impl shovels those bytes into their memory representation. A trivial example would be NonZero<u8> which could implement Random via rejection sampling. Setting it to 0 would be unsound.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@the8472 I did notice that, so we would need to restrict this to types that can hold arbitrary bytes (which currently includes every type implementing Random except bool). Otherwise, use a slower custom implementation with rejection sampling or modulo.

@rust-log-analyzer

This comment has been minimized.

Implements specialized `Random` for the array whose elements are
integer primitives. This reduces the number of system calls.
macro_rules! impl_random_for_integer_array {
($t:ty) => {
#[unstable(feature = "random", issue = "130703")]
impl<const N: usize> Random for [$t; N] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specializations should be hidden behind a private trait. https://std-dev-guide.rust-lang.org/policy/specialization.html

Alternatively you can avoid specialization by implementing a buffering random source wrapper as mentioned in #136732 (comment)

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too would prefer a buffering implementation (in the generic case, the specialization for primitives makes sense to me) – but you can leave that for another PR.

&raw mut buf as *mut u8,
N * (<$t>::BITS as usize / 8),
);
source.fill_bytes(bytes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unsound: you create a &mut [u8] and pass it to user code – but nothing guarantees that the user code won't try to read the slice, which would be undefined behaviour. My recommendation would be to use MaybeUninit::zeroed to create buf.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this needs a trait bound like zerocopy::FromBytes or bytemuck::Pod, but we don't have that in std, so... the macro should be renamed to unsafe_impl_... since safety depends on the macro caller?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2025
@dhardy
Copy link
Contributor

dhardy commented Mar 14, 2025

This uses specialization to do what we couldn't do with rand::Fill, which is great except:

Any non-default impls of Random will produce different values when sampled. This makes the introduction of new non-default impls potentially a breaking change, depending on how std wants to handle reproducibility guarantees for random generators.

(My view is still that random-value generation should be left to external crates like rand, partly to avoid this reproducibility-guarantee issue.)

@sorairolake sorairolake requested a review from joboet March 20, 2025 06:52
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2025
@joboet
Copy link
Member

joboet commented Sep 30, 2025

Sorry for not replying here for so long. This still exhibits the unsoundness I mentioned, and in any case is probably blocked on T-libs-api figuring out how they want to proceed with the random feature...

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@joboet joboet added the S-waiting-on-team DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ... label Sep 30, 2025
@Urgau Urgau added S-waiting-on-t-libs-api Status: Awaiting decision from T-libs-api and removed S-waiting-on-team DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ... labels Oct 6, 2025
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting.

This PR is based on the previous Random trait; we've rewritten to use a Distribution<T> trait that's closer to what rand uses.

But also, this uses specialization, which we'd prefer to avoid for this (and its current implementation is unsound).

@Amanieu proposed, and @the8472 and I agreed, that we should instead have a method sample_slice with a default implementation that's element-by-element, and the standard library can override that implementation to do the faster thing.

In order for that method to be performant, it would need to use something like BorrowedBuf/BorrowedCursor, in order to work with an uninitialized array. That, in turn, would require making BorrowedBuf/BorrowedCursor generic to work on something other than u8. In the meeting, we were broadly in favor of doing that, now that we have a use case in the standard library.

As a rough sketch of how this would look after we have the BorrowedBuf rework:

impl Distribution<[T; N]> {
    fn sample(&self, source: &mut (impl RandomSource + ?Sized)) -> T {
        // Set up an uninitialized BorrowedBuf
        T::sample_slice(buf.unfilled(), source);
        out
    }
    
    fn sample_slice(buf: BorrowedCursor<'_, [T; N]>, source: &mut (impl RandomSource + ?Sized)) {
        // Flatten into a cursor over `T`, and call `T::sample_slice`
        // This allows nested arrays like `[[u32; 8]; 8]` to Just Work.
    }
}

@the8472 the8472 removed the S-waiting-on-t-libs-api Status: Awaiting decision from T-libs-api label Oct 21, 2025
@bors
Copy link
Collaborator

bors commented Nov 10, 2025

☔ The latest upstream changes (presumably #135634) made this pull request unmergeable. Please resolve the merge conflicts.

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

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.