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

fill_via_chunks: mutate src on BE (small optimisation) #1182

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Sep 15, 2021

This is an API-breaking change to rand_core but not too consequential, and results in a small performance boost on Big Endian:

From:
test gen_bytes_chacha12      ... bench:     367,282 ns/iter (+/- 265) = 2788 MB/s
test gen_bytes_chacha20      ... bench:     504,012 ns/iter (+/- 6,271) = 2031 MB/s
test gen_bytes_chacha8       ... bench:     296,971 ns/iter (+/- 1,062) = 3448 MB/s

To:
test gen_bytes_chacha12      ... bench:     334,430 ns/iter (+/- 2,287) = 3061 MB/s
test gen_bytes_chacha20      ... bench:     470,403 ns/iter (+/- 1,755) = 2176 MB/s
test gen_bytes_chacha8       ... bench:     262,490 ns/iter (+/- 1,313) = 3901 MB/s

LE is essentially unchanged:

test gen_bytes_chacha12      ... bench:     298,710 ns/iter (+/- 2,715) = 3428 MB/s
test gen_bytes_chacha20      ... bench:     436,912 ns/iter (+/- 4,788) = 2343 MB/s
test gen_bytes_chacha8       ... bench:     229,400 ns/iter (+/- 3,040) = 4463 MB/s

All numbers are smaller than expected due to #1181.

Since BE is not very popular and it's not that big a bump, we may choose not to merge this.

@dhardy dhardy added the B-API Breakage: API label Sep 15, 2021
@vks vks mentioned this pull request Sep 15, 2021
23 tasks
@dhardy
Copy link
Member Author

dhardy commented Nov 15, 2021

Updated with documentation and rebase.

The code is significantly nicer so probably worth merging this (when we accept breaking changes).

@vks
Copy link
Collaborator

vks commented Feb 22, 2022

Looks good! I think this only needs a CHANGELOG update.

@dhardy
Copy link
Member Author

dhardy commented Feb 22, 2022

This is a breaking change to rand_core, not rand. Are we planning a breaking release of that soon? There likely will be before 1.0 (at least @newpavlov has some ideas) but I've no idea what the schedule for that will be (or whether we want another breaking release of rand_core for rand v0.9).

@vks
Copy link
Collaborator

vks commented Feb 22, 2022

A breaking change to rand_core is a breaking change to rand, so it would make sense to synchronize them, wouldn't it?

@newpavlov
Copy link
Member

newpavlov commented Feb 22, 2022

One thing which I would like to do before releasing rand_core v1.0 is to merge BlockRng/BlockRng64 into a single wrapper type and changeBlockRngCore to:

pub trait BlockRngCore {
    type Item: SealedTrait;
    const ITEMS: usize;
    fn generate(&mut self) -> [Self::Item; Self::ITEMS];
}

It's possible today on Nightly with generic_const_exprs, but, unfortunately, it has no clear stabilization timeline.

Such trait still has a certain deficiency around target features, e.g. IIRC with ChaCha we have to use ChaCha block size multiplied by maximum number of blocks which could be generated in parallel (i.e. on non-AVX2 targets code will be a bit less efficient than possible), but we probably can overlook it. In RustCrypto we "solved" it using emulation of rank-2 closures, but such solution is somewhat complex and does not fit well into the rand use-case (when you use as a stream cipher you usually consume many blocks at once, while with RNG you often generate u32/u64s one by one).

@dhardy
Copy link
Member Author

dhardy commented Feb 23, 2022

so it would make sense to synchronize them, wouldn't it?

More to the point, a breaking release of rand_core requires a breaking release of rand, but not vice versa.

So we can't release rand_core v1.0 for a while yet (also dependant on getrandom); the question is whether we want to make a breaking release soon or hold off on it for now.

If we didn't make a breaking release of rand_core, we wouldn't need to bump the RNG crates either.

@dhardy
Copy link
Member Author

dhardy commented Dec 6, 2022

Breaking changes to rand_core are planned, so this will be rebased + merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants