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

Chacha: integrate c2-chacha dependency #931

Merged
merged 5 commits into from
Jan 20, 2020
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 13, 2020

This adds approx 250 lines of code to rand_chacha from the c2-chacha crate and reduces the dependency count by 1. It also means the c2-chacha crate no longer needs to feature-gate the "primary" API it implements. Closes #872.

Since there is no breaking change to rand_chacha, the version bump is merely a patch version.

Ideally I'd like to see @kazcw's approval. Licences are the same so no problem there.

@dhardy dhardy requested a review from vks January 13, 2020 18:09
rand_chacha/src/guts.rs Outdated Show resolved Hide resolved
}

#[inline(always)]
pub fn set_stream_param(&mut self, param: u32, value: u64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to cut all the *_stream_param stuff--it's unused and not verified against any official test vectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do use this in set_word_pos and set_stream methods on the RNGs.

We have a couple of our own tests using these methods, although only with three constants in total.

@kazcw
Copy link
Contributor

kazcw commented Jan 16, 2020

This makes sense. I'll be officially discontinuing and freezing my crate since rand turned out to be its only direct user (besides what I originally wrote it for, which is defunct). There's some simplification I plan to do to the ppv-lite86 API eventually, mostly consolidating traits. I'll probably do that when I add the next backend (avx512, NEON, WASM, and packed_simd are planned eventually, mostly waiting on std::arch implementing the intrinsics), so after I do that I can PR the corresponding update here.

You might want to annex some of my test cases too; they're closer to the implementation and able to cover some edge cases the rand_chacha tests probably don't.

I've also just noticed that I SIMD-accelerated a few top-level functions that aren't performance-sensitive, so I've written a patch to just do those bits with portable code. It's a minor change but to keep things simple I'll PR it separately after this lands.

@dhardy
Copy link
Member Author

dhardy commented Jan 17, 2020

I see no reason not to merge this now. All happy?

(Whether we replace this crate is another issue that doesn't need to influence this PR — I'll make a new issue.)

@dhardy dhardy merged commit d683f67 into rust-random:master Jan 20, 2020
@burdges
Copy link
Contributor

burdges commented Jan 21, 2020

In the end you merged c2-chacha into rand_chacha, so the chacha used here no longer exists stand alone? I suppose doing this sounds unavoidable because rand supplies its own BlockRng which differs from that supplied by stream ciphers.

I guess the chacha crate https://github.com/RustCrypto/stream-ciphers/tree/master/chacha20 is a dependency zoo until const generics land, and still significantly slower.

@dhardy
Copy link
Member Author

dhardy commented Jan 21, 2020

Less 'unavoidable' than 'an unnecessary extra step'.

This crate (c2-chacha) already had some work put into it to reduce dependencies. The same could be done for chacha20. The primary reason not to do so at the moment is that the latter is slower and doesn't auto-detect CPU features (#934).

@tarcieri
Copy link

I guess the chacha crate is a dependency zoo until const generics land, and still significantly slower.

It only has two mandatory dependencies: cfg-if and cpufeatures. We could probably get rid of the former if it's a big concern.

Regarding performance: RustCrypto/stream-ciphers#267 (comment)

[chacha20] doesn't auto-detect CPU features

It does now!

@vks
Copy link
Collaborator

vks commented Aug 29, 2021

Looks like the performance gap is closed, that's very nice! Migrating to chacha20 would require bumping the MSRV to 1.51.

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 29, 2021

It looks like chacha20 always tries to uses SSE2 on x86 and x86_64 even if it isn't available. It only checks if AVX2 exists. This is will cause issues on certain CPUs. While the doc comments says "the sse2 target feature is enabled-by-default on all x86(_64) CPUs", this is not true for the i586 targets. Only for the i686 and x86_64 targets.

@tarcieri
Copy link

@bjorn3 I opened RustCrypto/stream-ciphers#270 for that

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 29, 2021

Thanks!

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.

rand_chacha and reducing dependencies
6 participants