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.

Sign up for GitHub

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 fill_via_chunks without using zerocopy or unsafe. #1575

Closed
wants to merge 1 commit into from
Closed

Implement fill_via_chunks without using zerocopy or unsafe. #1575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion rand_core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,3 @@ serde = ["dep:serde"] # enables serde for BlockRng wrapper
[dependencies]
serde = { version = "1", features = ["derive"], optional = true }
getrandom = { version = "0.3.0", optional = true }
zerocopy = { version = "0.8.0", default-features = false }
60 changes: 34 additions & 26 deletions rand_core/src/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
//! non-reproducible sources (e.g. `OsRng`) need not bother with it.

use crate::RngCore;
use core::cmp::min;
use zerocopy::{Immutable, IntoBytes};

/// Implement `next_u64` via `next_u32`, little-endian order.
pub fn next_u64_via_u32<R: RngCore + ?Sized>(rng: &mut R) -> u64 {
Expand Down Expand Up @@ -53,17 +51,22 @@ pub fn fill_bytes_via_next<R: RngCore + ?Sized>(rng: &mut R, dest: &mut [u8]) {
}
}

trait Observable: IntoBytes + Immutable + Copy {
fn to_le(self) -> Self;
trait Observable: Copy {
type Bytes: Sized + AsRef<[u8]>;
fn to_le_bytes(self) -> Self::Bytes;
}
impl Observable for u32 {
fn to_le(self) -> Self {
self.to_le()
type Bytes = [u8; 4];

fn to_le_bytes(self) -> Self::Bytes {
Self::to_le_bytes(self)
}
}
impl Observable for u64 {
fn to_le(self) -> Self {
self.to_le()
type Bytes = [u8; 8];

fn to_le_bytes(self) -> Self::Bytes {
Self::to_le_bytes(self)
}
}

Expand All @@ -72,32 +75,35 @@ impl Observable for u64 {
/// Returns `(n, byte_len)`. `src[..n]` is consumed (and possibly mutated),
/// `dest[..byte_len]` is filled. `src[n..]` and `dest[byte_len..]` are left
/// unaltered.
fn fill_via_chunks<T: Observable>(src: &mut [T], dest: &mut [u8]) -> (usize, usize) {
fn fill_via_chunks<T: Observable>(src: &[T], dest: &mut [u8]) -> (usize, usize) {
let size = core::mem::size_of::<T>();
let byte_len = min(core::mem::size_of_val(src), dest.len());
let num_chunks = (byte_len + size - 1) / size;

// Byte-swap for portability of results. This must happen before copying
// since the size of dest is not guaranteed to be a multiple of T or to be
// sufficiently aligned.
if cfg!(target_endian = "big") {
for x in &mut src[..num_chunks] {
*x = x.to_le();
}
}

dest[..byte_len].copy_from_slice(&<[T]>::as_bytes(&src[..num_chunks])[..byte_len]);
// Always use little endian for portability of results.

(num_chunks, byte_len)
let mut dest = dest.chunks_exact_mut(size);
let mut src = src.iter();

let zipped = dest.by_ref().zip(src.by_ref());
let num_chunks = zipped.len();
zipped.for_each(|(dest, src)| dest.copy_from_slice(src.to_le_bytes().as_ref()));

let byte_len = num_chunks * size;
if let (dest_tail @ [_, ..], Some(src)) = (dest.into_remainder(), src.next()) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I find the if let pattern here a little confusing, could we just do:

    if let Some(src) = src.next() {
        // We have consumed all full chunks of dest, but not src.
        let dest = dest.into_remainder();
        let n = dest.len();
        if n > 0 {
            dest.copy_from_slice(&src.to_le_bytes().as_ref()[..n]);
            return (num_chunks + 1, byte_len + n)
        }
    }
    (num_chunks, byte_len)

Copy link
Member

Choose a reason for hiding this comment

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

The usage of a slice match pattern to check for a non-empty tail is not so obvious, no.

This alternative compiles to identical assembly so I'll take it by preference.

let n = dest_tail.len();
dest_tail.copy_from_slice(&src.to_le_bytes().as_ref()[..n]);
(num_chunks + 1, byte_len + n)
} else {
(num_chunks, byte_len)
}
}

/// Implement `fill_bytes` by reading chunks from the output buffer of a block
/// based RNG.
///
/// The return values are `(consumed_u32, filled_u8)`.
///
/// On big-endian systems, endianness of `src[..consumed_u32]` values is
/// swapped. No other adjustments to `src` are made.
/// `src` is not modified; it is taken as a `&mut` reference for backward
/// compatibility with previous versions that did change it.
///
/// `filled_u8` is the number of filled bytes in `dest`, which may be less than
/// the length of `dest`.
Expand Down Expand Up @@ -125,6 +131,7 @@ fn fill_via_chunks<T: Observable>(src: &mut [T], dest: &mut [u8]) -> (usize, usi
/// }
/// ```
pub fn fill_via_u32_chunks(src: &mut [u32], dest: &mut [u8]) -> (usize, usize) {
// TODO(SemVer): src: `&[u32]` as we don't mutate it.
fill_via_chunks(src, dest)
}
Comment on lines 133 to 136
Copy link
Member

Choose a reason for hiding this comment

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

Does anyone know why this method is public at all? It is used in the impl of RngCore for BlockRng<R: BlockRngCore>, which is crate-local. I doubt RNGs would want to use it directly. It looks like this pub fn pre-dates BlockRng.

I suggest that we deprecate these two methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering that myself

Copy link
Collaborator

Choose a reason for hiding this comment

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

My guess is that it was intended to be used by other crates implementing RngCore.


Expand All @@ -133,8 +140,8 @@ pub fn fill_via_u32_chunks(src: &mut [u32], dest: &mut [u8]) -> (usize, usize) {
///
/// The return values are `(consumed_u64, filled_u8)`.
///
/// On big-endian systems, endianness of `src[..consumed_u64]` values is
/// swapped. No other adjustments to `src` are made.
/// `src` is not modified; it is taken as a `&mut` reference for backward
/// compatibility with previous versions that did change it.
///
/// `filled_u8` is the number of filled bytes in `dest`, which may be less than
/// the length of `dest`.
Expand All @@ -143,6 +150,7 @@ pub fn fill_via_u32_chunks(src: &mut [u32], dest: &mut [u8]) -> (usize, usize) {
///
/// See `fill_via_u32_chunks` for an example.
pub fn fill_via_u64_chunks(src: &mut [u64], dest: &mut [u8]) -> (usize, usize) {
// TODO(SemVer): src: `&[u64]` as we don't mutate it.
fill_via_chunks(src, dest)
}

Expand Down