Skip to content

Conversation

@newpavlov
Copy link
Member

@newpavlov newpavlov commented Nov 10, 2025

This change allows to remove the serde feature and reduce total amount of code in the crate. Additionally, the helper block functions now store the cursor position in the first buffer value, which allows us to remove the separate index field.

The new examples demonstrate implementation of the traits for the most common RNG classes.

TODO:

  • Check value stability in downstream impls.
  • Write integration tests with 100% coverage.

@newpavlov newpavlov marked this pull request as ready for review November 12, 2025 14:30
@newpavlov newpavlov requested a review from dhardy November 12, 2025 14:30
}

/// Create new block buffer.
pub fn new_buffer<W: Word, const N: usize>() -> [W; N] {
Copy link
Member Author

@newpavlov newpavlov Nov 12, 2025

Choose a reason for hiding this comment

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

One minor disadvantage of using the Word trait to de-duplicate code is that we can not currently mark this function const. Since this function i intended to be used in SeedableRng implementations it does not matter much, plus users can vendor the implementation into their const fn if necessary.

@dhardy
Copy link
Member

dhardy commented Nov 13, 2025

What does this look like at the use site? You could update rand_chacha at least (same repo; yes I am going to have to remake those PRs to move it to the rngs repo anyway).

@dhardy
Copy link
Member

dhardy commented Nov 13, 2025

Motivation regarding serde is good; the feature does not really belong in this crate.

lazy block generation

Not a change.

The le module is renamed to utils

I explained why I didn't want to do this in rust-random/rand#1667.

@newpavlov
Copy link
Member Author

newpavlov commented Nov 14, 2025

What does this look like at the use site?

See the new doc examples. I wanted to update the implementation crates after I got a confirmation that you approve the new approach.

Not a change.

True. I will edit the OP.

I explained why I didn't want to do this

The problem is that the new_buffer function and the Word trait technically have nothing to do with "litttle endian". And I think utils::fill_bytes_via_gen_block looks better in user code than le::fill_bytes_via_gen_block. But it's not a strong opinion, so I can rename it back, if you insist.

@newpavlov
Copy link
Member Author

newpavlov commented Nov 14, 2025

All potential panics in fill_bytes_via_gen_block get eliminated successfully: https://rust.godbolt.org/z/6E9Y7vW3o Unfortunately, codegen is a bit worse than expected. The compiler unrolls the head and tail processing loops instead of compiling them into one memcpy. I will look into whether it's possible to tweak the code to prevent it.

next_word_via_gen_block looks good: https://rust.godbolt.org/z/5qd7MaKx3 Same for fill_bytes_via_next_word: https://rust.godbolt.org/z/esYo49oKK

@newpavlov
Copy link
Member Author

newpavlov commented Nov 14, 2025

I improved codegen for fill_bytes_via_gen_block (see https://rust.godbolt.org/z/T8f77KjGc), but unfortunately it involves unsafe code. In future it could be removed with generic_const_exprs, but for now I couldn't find a way to do it on Rust 1.85 without returning to the bad codegen.

@dhardy
Copy link
Member

dhardy commented Nov 14, 2025

The problem is that the new_buffer function and the Word trait technically have nothing to do with "litttle endian".

It's fine to put those in a new utils module then. I don't like reformatting / moves getting mixed in with other changes, so moving le should be a different PR. (Exception: if commits are cleanly formatted for by-commit review.)

I wanted to update the implementation crates after I got a confirmation that you approve the new approach.

Isn't the best validation of a proof-of-concept API to use it? rand_chacha is in the same repo hence why I suggested to update that.

@dhardy dhardy mentioned this pull request Nov 14, 2025
7 tasks
Comment on lines 333 to 334
let pos = buf[0].into_usize();
match buf.get(pos) {
Copy link
Member

Choose a reason for hiding this comment

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

What if pos == 0? It looks like this shouldn't happen, but I'd prefer it be obvious to the compiler and reader that this cannot return the position counter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be debug_assert sufficient for that?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know: does it affect optimization?

@newpavlov
Copy link
Member Author

newpavlov commented Nov 14, 2025

It's fine to put those in a new utils module then.

I really do not want to introduce two separate utility modules. I returned the le name for now.

Isn't the best validation of a proof-of-concept API to use it?

I did use them in the examples. I plan to update chacha20 a bit later.

@dhardy
Copy link
Member

dhardy commented Nov 14, 2025

I really do not want to introduce two separate utility modules.

Better to punt the issue to a new PR than to worry overly about it here. More important that this PR is easy to review.

I did use them in the examples.

Yes, but examples don't always match up with real usage, so I strongly prefer seeing a new API in a real use-case than just in an example. Doc examples are also harder to review (no highlighting without building docs).

@dhardy
Copy link
Member

dhardy commented Nov 14, 2025

The approach looks broadly okay, not losing much from our previous code. But I cannot approve it without running benchmarks on actual block RNGs, so it does need some downstream implementation before this PR can be approved.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants