Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Random for array #136732

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.

library/core/src/array/mod.rs Outdated Show resolved Hide resolved
@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants