-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
@@ -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>, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof 😬
There was a problem hiding this 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
.
@tarcieri |
@newpavlov done |
Thank you! |
Mostly changes around the new seek trait, plus some improvements of the code for
ctr
andsalsa20
/chacha20
.TODO:
aesni
chacha20
on AVX2@tarcieri
Can you please check the
salsa20
andchacha20
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 theOption
and instead code now assumes that if buffer position is equal to 0, then the cached buffer is invalid.