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

Upgrade to stream-cipher v0.7 #164

Merged
merged 9 commits into from
Aug 25, 2020
Merged

Upgrade to stream-cipher v0.7 #164

merged 9 commits into from
Aug 25, 2020

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Aug 25, 2020

Mostly changes around the new seek trait, plus some improvements of the code for ctr and salsa20/chacha20.

TODO:

  • upgrade aesni
  • fix chacha20 on AVX2

@tarcieri
Can you please check the salsa20 and chacha20 changes, especially the latter one? Tests do pass, but I am not 100% confident about changes (and with salsa one mistake almost got through, if not for the single "hello world" test). Note that I've removed the Option and instead code now assumes that if buffer position is equal to 0, then the cached buffer is invalid.

@@ -61,7 +62,7 @@ pub struct Cipher<R: Rounds> {
buffer: Buffer,

/// Position within buffer, or `None` if the buffer is not in use
buffer_pos: Option<u8>,
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this was a performance optimization for the AEAD use case, which is always able to work with ChaCha blocks-at-a-time until the last block.

It'd be good to double check how/if this impacts the performance of chacha20poly1305.

Copy link
Member Author

@newpavlov newpavlov Aug 25, 2020

Choose a reason for hiding this comment

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

The current API will not cache buffer if fed with data multiple of block size, so performance should be the same (though ofc worth checking out just to be safe). Note the if pos != 0 { .. } and if !rem.is_empty() { .. } parts. The Option<u8> (which I think you've borrowed from the ctr crate) approach was redundant, since buffer_pos will never be equal to Some(0).

Copy link
Member Author

Choose a reason for hiding this comment

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

After quick and dirty tests (i.e. without fiddling with hardware settings) the chacha20poly1305 performance is approximately the same on my CPU, with 2.605 cpb for decryption and 8.337 cpb for encryption (without enabled target-features).


#[cfg(features = "xchacha20")]
Copy link
Member

Choose a reason for hiding this comment

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

Oof 😬

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Overall this looks like an improvement to me.

It'd be good to double check this doesn't cause a performance regression in chacha20poly1305.

@newpavlov newpavlov merged commit dec7d26 into master Aug 25, 2020
@newpavlov newpavlov deleted the new_seek branch August 25, 2020 15:22
@newpavlov
Copy link
Member Author

newpavlov commented Aug 25, 2020

@tarcieri
Can you please add the stream-ciphers team as an owner of hc-128 and hc-256 crates?

@tarcieri
Copy link
Member

@newpavlov done

@newpavlov
Copy link
Member Author

Thank you!

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.

None yet

2 participants